User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031323 Minefield/3.0b5pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031323 Minefield/3.0b5pre Now that JSON bookmark backup has landed, there are two potential bookmark file formats, JSON and HTML. The interface does not correctly handle a user selecting the wrong type of file. Reproducible: Always Steps to Reproduce: 1. Go to Bookmarks->Organize Bookmarks. 2. Go to Import and Backup->Backup and save a JSON backup somewhere. 3. Modify your bookmarks (e.g. add some new ones or delete some). 4. Go to Import and Backup->Import and choose From File then select the backup saved in step 2. Actual Results: I get a new folder called JavaScript%20Shell%201.4 (I have a bookmark called this that links to a javascript: url) and my changes made in step 3 above are not undone. Expected Results: My bookmarks should be restored to how they were before step 3 above. A similar effect can be seen in reverse by exporting your bookmarks to HTML then using Restore->Choose File (which expects a JSON file), nothing appears to happen. I think that "Restore->Choose File" should be removed and Import made to handle either type automatically. You could also combine Export and Backup into one option and allow picking the file type from the standard "Save as type" box in the save dialog.

Requesting blocking because this UI can appear plain broken when using the wrong filetype with the wrong import/restore option, and is otherwise pretty confusing.

Aside from having the options handle only the specific format, we need a clear indication in the UI that import/export means HTML (external use) and backup/restore means JSON (internal use). I think that explicit visual/textual communication is preferable over error messages (ie: forcing the user into using trial and error to see which menu option they actually need) or the current "just works" approach, which has unexpected results depending on which menu-option+file-format combination is selected.

Agree completely with comment 2, but I'm not sure what we can do at this point beyond restricting the file types and ..: - in the Import dialog, change the label of "From file" to "From HTML file" - in the Restore submenu, change label of "Choose file" to "From file"

(In reply to comment #3) > Agree completely with comment 2, but I'm not sure what we can do at this point > beyond restricting the file types and ..: > What do you think of this proposal? https://bugzilla.mozilla.org/show_bug.cgi?id=405936#c9

(In reply to comment #4) > https://bugzilla.mozilla.org/show_bug.cgi?id=405936#c9 I like it, but suggest the following tweaks: Import and Backup Backup... --> leads to file picker[1] Restore...> YYYY-MM-DD YYYY-MM-DD YYYY-MM-DD YYYY-MM-DD ---------- Choose file... --> leads to file picker[2] ------------ Import Bookmarks... --> leads to Import Wizard[3] Export Bookmarks... --> leads to file picker[4] [1] file type should be restricted to <blank> (as JSON isn't an option, aiui) [2] we should add "(HTML)" to the "From file" option in this wizard [3] file type should be restricted to HTM, HTML [4] file type should be restricted to HTM, HTML

Comment on attachment 311423 [details] [diff] [review] fix v1 this implements beltzner's suggestion in comment #5, so removing ui-r request.

Shouldn't that be "From a HTML File"?

It should be, yes. Also, bug 424534 points out that we don't need the ellipses on "Restore...". That's a good point.

Comment on attachment 311465 [details] [diff] [review] fix v2 (comments fixed) >Index: browser/locales/en-US/chrome/browser/places/places.dtd >-<!ENTITY cmd.restore.label "Restore…"> >+<!ENTITY cmd.restore.label "Restore"> That'd leave 47 ellipsisees unchanged, though, http://mxr.mozilla.org/l10n/search?string=cmd.restore.label&find=places%2Fplaces\.dtd%24&findi=\.dtd%24&filter=&tree=l10n

+ if (!f.isHidden() && f.leafName.match(/^bookmarks-.+json?$/)) why json? with optional n, is .jso a valid extension?

Comment on attachment 311465 [details] [diff] [review] fix v2 (comments fixed) r=mano

Comment on attachment 311465 [details] [diff] [review] fix v2 (comments fixed) Requesting driver approval for late-l10n.

(In reply to comment #12) > + if (!f.isHidden() && f.leafName.match(/^bookmarks-.+json?$/)) > > why json? with optional n, is .jso a valid extension? > no, i'll post a new patch.

Comment on attachment 312930 [details] [diff] [review] fix v3 (addresses comment #12 r=mano, don't you want to include the part I've reviewed on the other bug here?

(In reply to comment #17) > (From update of attachment 312930 [details] [diff] [review]) > r=mano, don't you want to include the part I've reviewed on the other bug here? > After looking at that, I think it might be better to move the error prompt in PlacesUtils.restoreBookmarksFromFile() to the front-end. This means that: 1. no UI being generated out of the back-end, toolkit consumers can handle errors in their own way 2. we can use the proper window for the prompt 3. no restrictions on file extension on the backend, toolkit consumers can make their own determination about what's supported If this sounds good, I'll combine these two patches and re-attach here.

implements solution in comment #18, in addition to a few issues found while testing this: - folds in fix for bug 424654 - moves error UI to the front-end - removes a couple of unused duplicate strings - removes latest backup when replacing (followup to bug 424389)

verification steps: - menu re-arrange per comment #5 - migration screen says "From a HTML File" - the file picker when importing, filters for HTML files - the restore menu only shows JSON backups from the bookmarksbackup dir - the file picker when restoring, filters for JSON files - when restoring from a non json file, get the format not supported error prompt - when restoring from a json file that's not parse-able, get the parse error prompt - the file picker when backing up, filters for JSON files - restore menu item does not have ellipsis - new profile, immediately open library, open restore menu, confirm that bookmarksbackup directory is a directory - back to a file on your desktop twice. confirm that the second backup replaces the first one in the bookmarksbackup directory

Comment on attachment 313144 [details] [diff] [review] fix v4 >Index: browser/components/places/content/places.js >=================================================================== >+ showErrorAlert: function PO_showErrorAlert(aMsg) { nit: prefix it with an underscore. >+ const BRANDING_BUNDLE_URI = "chrome://branding/locale/brand.properties"; >+ var brandShortName = Cc["@mozilla.org/intl/stringbundle;1"]. >+ getService(Ci.nsIStringBundleService). >+ createBundle(BRANDING_BUNDLE_URI). >+ GetStringFromName("brandShortName"); >+ I'd prefer using a <stringbundle/> >+ var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Ci.nsIWindowMediator); >+ var win = wm.getMostRecentWindow("Places:Organizer"); >+ >+ Cc["@mozilla.org/embedcomp/prompt-service;1"]. >+ getService(Ci.nsIPromptService). >+ alert(win, brandShortName, aMsg); you're already in the context of an organizer window, why do you use the window manger? >Index: browser/locales/en-US/chrome/browser/places/places.dtd >=================================================================== > <!ENTITY cmd.backup.label "Backup…"> > <!ENTITY cmd.backup.accesskey "B"> >-<!ENTITY cmd.restore.label "Restore…"> >+<!ENTITY cmd.restore.label "Restore"> reve the entity name please.

Comment on attachment 313203 [details] [diff] [review] fix v5 - comments addressed >-<!ENTITY importFromFile.label "From File"> >-<!ENTITY importFromFile.accesskey "F"> >+<!ENTITY importFromHTMLFile.label "From a HTML File"> >+<!ENTITY importFromHTMLFile.accesskey "F"> "From an HTML file"

Comment on attachment 313203 [details] [diff] [review] fix v5 - comments addressed can fix the a/an/ on checkin, it wouldn't be late-l10n anyway :)

Comment on attachment 313203 [details] [diff] [review] fix v5 - comments addressed r=mano

Comment on attachment 313203 [details] [diff] [review] fix v5 - comments addressed a1.9=beltzner

Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.322; previous revision: 1.321 done Checking in browser/components/migration/content/migration.xul; /cvsroot/mozilla/browser/components/migration/content/migration.xul,v <-- migration.xul new revision: 1.30; previous revision: 1.29 done Checking in browser/components/places/content/places.js; /cvsroot/mozilla/browser/components/places/content/places.js,v <-- places.js new revision: 1.152; previous revision: 1.151 done Checking in browser/components/places/content/places.xul; /cvsroot/mozilla/browser/components/places/content/places.xul,v <-- places.xul new revision: 1.126; previous revision: 1.125 done Checking in browser/components/places/tests/unit/test_384370.js; /cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v <-- test_384370.js new revision: 1.16; previous revision: 1.15 done Checking in browser/locales/en-US/chrome/browser/migration/migration.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/migration/migration.dtd,v <-- migration.dtd new revision: 1.8; previous revision: 1.7 done Checking in browser/locales/en-US/chrome/browser/places/places.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v <-- places.dtd new revision: 1.43; previous revision: 1.42 done Checking in browser/locales/en-US/chrome/browser/places/places.properties; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v <-- places.properties new revision: 1.39; previous revision: 1.38 done Checking in toolkit/components/places/src/utils.js; /cvsroot/mozilla/toolkit/components/places/src/utils.js,v <-- utils.js new revision: 1.11; previous revision: 1.10 done Checking in toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js,v <-- test_424958-json-quoted-folders.js new revision: 1.2; previous revision: 1.1 done Checking in toolkit/locales/en-US/chrome/places/places.properties; /cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v <-- places.properties new revision: 1.7; previous revision: 1.6 done

- s/a/an/ - s/restoreBookmarksFromFile/restoreBookmarksFromJSONFile/ in a test

> -<!ENTITY cmd.restore.label "Restore…"> > +<!ENTITY cmd.restore2.label "Restore"> > <!ENTITY cmd.restore.accesskey "R"> accesskey entity name should be changed too

now "Import" is changed to "Import HTML" "Export" is changed to "Export HTML" so these menus should be changed too ? "Backup" >> "Backup JSON" "Restore" >> "Restore JSON"

Checking in browser/components/places/content/places.xul; /cvsroot/mozilla/browser/components/places/content/places.xul,v <-- places.xul new revision: 1.127; previous revision: 1.126 done Checking in browser/locales/en-US/chrome/browser/places/places.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v <-- places.dtd new revision: 1.44; previous revision: 1.43 done

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031806 Minefield/3.0b5pre

Test case https://litmus.mozilla.org/show_test.cgi?id=6853 has been updated for regression testing this bug.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv