Comment 133 for bug 214366

(In reply to <email address hidden> from comment #122)
> Comment on attachment 8374439
> WIP patch 4
>
> >+ bool containsChild = true;
> >+ uint32_t i;
> >+ for (i = 1; ; i++) {
> >+ newFolderName.Assign(folderName);
> >+ if (i > 1) {
> >+ // This could be localizable but Toolkit is fine without it, see
> >+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
> >+ newFolderName.Append('(');
> >+ newFolderName.AppendInt(i);
> >+ newFolderName.Append(')');
> >+ }
> >+ rv = ContainsChildNamed(newFolderName, &containsChild);
> >+ if (NS_WARN_IF(NS_FAILED(rv))) {
> >+ return rv;
> >+ }
> >+ if (!containsChild)
> >+ break;
> >+ }
> Did you mean for (i = 1; containsChild; i++) ? Note that if you do this then
> i ends the loop being at least 2, so you'd need to change the subsequent
> check. Alternatively, you would have to rewrite the loop something like this
> (sorry if this is what the code did in one of the earlier patches):
>
> uint32_t i = 1;
> newFolderName.Assign(folderName);
> for (;;) {
> bool containsChild;
> rv = ContainsChildNamed(newFolderName, &containsChild);
> NS_ENSURE_SUCCESS(rv, rv);
> if (!containsChild)
> break;
>
> i++;
> newFolderName.Assign(folderName);
> newFolderName.Append('(');
> newFolderName.AppendInt(i);
> newFolderName.Append(')');
> }
Yes, this is what the previous version did, just with a 'while'.
Is this issue really that important that we make tens of comments in such a long thread? It is not a perf critical path.

> >+ 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.

>
> >+ } catch (e) {
> >+ do_check_eq(e.result, parseInt("0x8055001a", 16));
> Why not just write 0x8055001a?
Because e.result returns the code in decimal. Is there other method of 'e' I can use?

> >+confirmDuplicateFolderRename=A folder with that name already exists under folder '%1$S'. Would you like to copy the folder under a new name of '%2$S'?
> A subfolder with that name already exists in the folder '%1$S'. Would you
> like to move the folder using the new name of '%2$S'?

Irving proposed some other wording with more arguments. So what should I do now?