Comment 136 for bug 214366

Comment on attachment 8377817
patch v5

Review of attachment 8377817:
-----------------------------------------------------------------

(In reply to <email address hidden> from comment #124)
> (In reply to aceman from comment #123)
> > (In reply to comment #122)
> > > (From update of attachment 8374439)
> > Is this issue really that important that we make tens of comments in such a
> > long thread? It is not a perf critical path.
>
> I'm after readability here, not perf, in particular I don't like breaking at
> the end of a for loop, because it's easy to miss that this skips the
> increment.
>
> > > >+ try {
> > > >+ root.copyFolderLocal(folderDeleted3, true, null, null);
> > > >+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
> > > I'm not sure why you're trying to throw from inside this try block, but I
> > > don't know enough about writing tests to know whether there's a better way.
> > I expect .copyFolderLocal to throw and be catched by my catch. But if it
> > does not throw (wrong behaviour), the next throw is not catched and fails
> > the test.
> Sure, but my question is is it safe to put do_throw in a try block?

You can call do_throw() inside a try; whether you catch it or not it still registers as a test failure (see https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#XPCShell_test_utility_functions)

> Sorry, I hadn't seen his wording. How about:
> A subfolder with the name '%$1S' already exists in the folder '%2$S'. Would
> you like to move this folder using the new name of '%3$S'?
> (I don't really want to use a / because that's an implementation detail.)

This is OK with me; whoever has ui-r? gets final say. r+ aside from that wording change.