Last Comment Bug 611568 - Move "File->Import" to the Library
: Move "File->Import" to the Library
Status: VERIFIED FIXED
: user-doc-needed
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Steffen Wilberg
:
:
Mentors:
: 513167 (view as bug list)
Depends on: 668647 672459
Blocks: 607226 633716
  Show dependency treegraph
 
Reported: 2010-11-11 20:38 PST by Alex Faaborg [:faaborg] (Firefox UX)
Modified: 2013-11-11 23:58 PST (History)
27 users (show)
simona.marcu: in‑litmus+
hskupin: in‑moztrap? (simona.marcu)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
browser-menubar.inc patch for #611568 and #565564 (1.01 KB, patch)
2011-01-24 19:37 PST, Brian Carpenter [:geeknik]
no flags Details | Diff | Splinter Review
Patch for this bug and 4 others, see comment. (13.93 KB, patch)
2011-01-24 20:10 PST, Brian Carpenter [:geeknik]
no flags Details | Diff | Splinter Review
Follow up patch (1.26 KB, patch)
2011-01-24 20:21 PST, Brian Carpenter [:geeknik]
no flags Details | Diff | Splinter Review
Removes related information from browser.dtd (1.93 KB, patch)
2011-01-24 20:27 PST, Brian Carpenter [:geeknik]
no flags Details | Diff | Splinter Review
Removes Import from Browser Menu (2.20 KB, patch)
2011-01-24 21:34 PST, Brian Carpenter [:geeknik]
gavin.sharp: review-
Details | Diff | Splinter Review
Updated Patch (6.38 KB, patch)
2011-01-25 01:49 PST, Brian Carpenter [:geeknik]
dolske: review-
Details | Diff | Splinter Review
move Import to the Library, v1 (12.35 KB, patch)
2011-01-27 15:11 PST, Steffen Wilberg
no flags Details | Diff | Splinter Review
move "Import" to Library, v1.1 (12.29 KB, patch)
2011-01-27 15:41 PST, Steffen Wilberg
no flags Details | Diff | Splinter Review
v1.1, unbitrotted (12.43 KB, patch)
2011-02-01 13:16 PST, Steffen Wilberg
dolske: review+
Details | Diff | Splinter Review
v 1.2, new strings (16.03 KB, patch)
2011-03-31 12:44 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
v1.2, unbitrotted (15.99 KB, patch)
2011-04-25 02:39 PDT, Steffen Wilberg
faaborg: ui‑review+
Details | Diff | Splinter Review
patch v1.3, with new string from comment 42 (15.99 KB, patch)
2011-06-09 15:30 PDT, Steffen Wilberg
dao+bmo: review-
steffen.wilberg: ui‑review+
Details | Diff | Splinter Review
patch v2: menu item disabled in PB mode (20.69 KB, patch)
2011-06-19 16:46 PDT, Steffen Wilberg
dao+bmo: review-
Details | Diff | Splinter Review
patch v2.1 (20.38 KB, patch)
2011-06-21 15:43 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
patch v2.2 (20.33 KB, patch)
2011-06-21 16:30 PDT, Steffen Wilberg
dao+bmo: review+
Details | Diff | Splinter Review

Description Alex Faaborg [:faaborg] (Firefox UX) 2010-11-11 20:38:38 PST
This bug is to remove "File Import..."

The rationale is that this command had extremely low usage in our usability
metrics coming from test pilot, especially relative to the surrounding very
high usage menu items.

The user has multiple other ways to perform this command, including:
-The wizard that opens when Firefox first launches from a fresh installation
-A toolbar button in the Library window (note this patch also needs to adjust that string from "Import HTML" to just "Import..." since the user has both options in the wizard anyway).
Comment 1 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2010-11-11 20:53:50 PST
The importer in the Library doesn't allow the user to import passwords, cookies, history, and preferences. We would loose an important import mechanism here.
Comment 2 Alex Faaborg [:faaborg] (Firefox UX) 2010-11-11 21:06:25 PST
Then let's just remap this import over there (I actually always thought they were the same).
Comment 3 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2010-11-11 21:08:11 PST
Cc'ing Dietrich and Marco, so we can make sure that gets addressed.
Comment 4 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-24 11:25:58 PST
The UX team is very eager to get this bug landed over the next few days in order to make Beta 11.  If anyone can get a patch for this bug posted soon, we will push hard for reviews and approval (even though this isn't blocking).

You can view all of the related bugs to clean up the traditional menu bar here: http://areweprettyyet.com/4/traditionalMenu/
Comment 5 Brian Carpenter [:geeknik] 2011-01-24 19:37:36 PST
Created attachment 506633 [details] [diff] [review]
browser-menubar.inc patch for #611568 and #565564

Is this what you're looking for? If so it knocks out two bugs, this one and Bug #565564.
Comment 6 Brian Carpenter [:geeknik] 2011-01-24 19:49:43 PST
I just realized that in the Library window "this patch also needs to adjust that string from "Import HTML" to just "Import..." since the user has both options in the wizard anyway). However, I'm having trouble finding where this is at. I mean, I think it's in http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul, but...
Comment 7 Brian Carpenter [:geeknik] 2011-01-24 20:10:20 PST
Created attachment 506639 [details] [diff] [review]
Patch for this bug and 4 others, see comment.

Partial for Bug #611568, total for #565564, #611569, #607227, #607230. I did an all in one for simplicity. I will knock out #607232 and #607234 next.
Comment 8 Brian Carpenter [:geeknik] 2011-01-24 20:21:35 PST
Created attachment 506641 [details] [diff] [review]
Follow up patch

This is a followup patch which removes the the Bookmark All Tabs and Page Info key from browser-sets.inc.
Comment 9 Brian Carpenter [:geeknik] 2011-01-24 20:27:28 PST
Created attachment 506642 [details] [diff] [review]
Removes related information from browser.dtd

This final patch removes the related information from the browser.dtd.
Comment 10 Brian Carpenter [:geeknik] 2011-01-24 21:34:24 PST
Created attachment 506648 [details] [diff] [review]
Removes Import from Browser Menu

This patch removes the Import from the browser menu (browser-menubar.inc) and from the browser.dtd.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-01-24 23:54:48 PST
Comment on attachment 506648 [details] [diff] [review]
Removes Import from Browser Menu

This seems to be the only caller to BrowserImport(), so you should probably remove that too (though perhaps not if this is actually going to land for 4.0). There are two references to this menu item in browser.js, and also a reference in browser_privatebrowsing_import.js .
Comment 12 :Ehsan Akhgari 2011-01-25 00:14:49 PST
(In reply to comment #11)
> Comment on attachment 506648 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=506648
> Removes Import from Browser Menu
> 
> This seems to be the only caller to BrowserImport(), so you should probably
> remove that too (though perhaps not if this is actually going to land for 4.0).
> There are two references to this menu item in browser.js, and also a reference
> in browser_privatebrowsing_import.js .
Actually, this patch should remove the browser_privatebrowsing_import.js test completely, since its only purpose is to make sure that menu item gets disabled inside the private browsing mode.
Comment 13 Brian Carpenter [:geeknik] 2011-01-25 01:49:06 PST
Created attachment 506689 [details] [diff] [review]
Updated Patch

This patch is the same as the original, but also removes the browser_privatebrowsing_import.js test and updates the Makefile.in. As far as the browser.js, which one? I counted quite a few. =)
Comment 14 Steffen Wilberg 2011-01-25 03:20:12 PST
This doesn't address comments 1, 2, and 11:
If we want to keep the possibility to import passwords,
cookies, history, and preferences, we need to make that available through Library --> Import and Backup button --> Import.

Both that and main menu File -> Import open chrome://browser/content/migration/migration.xul, the latter with "bookmarks" as its final argument, thus only migrating bookmarks, but also allowing to import an HTML file:
http://hg.mozilla.org/mozilla-central/annotate/6a097e294828/browser/components/places/content/places.js#l368
http://hg.mozilla.org/mozilla-central/annotate/6a097e294828/browser/base/content/browser.js#l2537

We need some input from the UX team here. I'd suggest to
1. relabel "Import as HTML..." to "Import Bookmarks as HTML...", which should directly open a file open dialog,
2. add a new menu item "Import...", opening the full migration wizard like File-->Impoort currently does.
Requires a new string though.


> As far as the browser.js, which one? I counted quite a few. =)
http://mxr.mozilla.org/mozilla-central/search?string=menu_import&find=browser.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 15 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-25 10:58:02 PST
I know we've already discussed various options in bugzilla somewhere, but I'm not entirely sure which bug now (looks like it wasn't this one, which would have been handy).

In addition to removing the menu item, we should rename "Import as HTML..." to just "Import..." and (same string), and only give users access to the full migration wizard.  That work can be handled in a separate bug.

This somewhat hides the feature, but the feature doesn't really work in the current marketplace (we don't support chrome, or delicious, etc.)
Comment 16 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-25 10:59:36 PST
Marco: do you know if we have a bug for adjusting the library to invoke the full wizard (and changing which string we use there)?
Comment 17 Steffen Wilberg 2011-01-25 11:15:32 PST
(In reply to comment #15)
> we should rename "Import as HTML..." to
> just "Import..." and (same string), and only give users access to the full
> migration wizard.
Besides the label change, that's a one-liner, see comment 14.
But we'd lose the ability to import/export the bookmarks file as HTML, as the full migration wizard doesn't support that. Do we want to do that?
Comment 18 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-25 11:20:47 PST
Wasn't aware of that, yeah, let's go with two import commands in the library window.
Comment 19 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-25 11:21:55 PST
The fact that "Import HMTL" just means bookmarks isn't ideal, but probably isn't worth breaking string freeze.  We need to significantly revisit our import UI at some point soon anyway.
Comment 20 Steffen Wilberg 2011-01-25 13:43:32 PST
Brian, do you want to take that as well?
Comment 21 Justin Dolske [:Dolske] 2011-01-26 20:20:31 PST
Comment on attachment 506689 [details] [diff] [review]
Updated Patch

Still need to figure out what where the "import from another browser" functionality should move to and strings impact. The Places organizer doesn't seem like a very good place for it, but between this not being likely to be used and already of marginal use in the current marketplace (comment 15), I'm kind of meh on where it ends up. A button somewhere under Options --> Advanced might be a nice dumping ground, but I don't see a very good fit there either (and it's cramped).

>--- a/browser/base/content/browser-menubar.inc	Mon Jan 24 19:47:52 2011 -0800
...
>                 <menuitem id="menu_print"
>                           label="&printCmd.label;"
>                           accesskey="&printCmd.accesskey;"
>                           key="printKb"
>                           command="cmd_print"/>
>-                <menuseparator/>

Should keep this menuseparator, so the printing related items are demarked from the items above and below them.

>-                <menuitem id="menu_import"

Need to fix onEnterPrivateBrowsing() and onExitPrivateBrowsing() [in browser.js], to not attempt to disable/enable this item.
Comment 22 Justin Dolske [:Dolske] 2011-01-26 21:09:01 PST
Talked with Faaborg jsut to be clear, current recommendation is for the places organizer to have both "Import From HTML" (does what it does right now), and "Import" (does what File --> Import did).

So, sigh, that would mean keeping the existing strings, making the places organizer use them for the new command, and fixing the private browsing stuff to ensure this command is disabled while in PB mode.

How about just getting a driver-level call on flat out removing this (and maybe someone write a tiny Jetpack to allow reexpose it)? I think most of us agree that the import functionality is already very lacking, and isn't likely to be missed much.
Comment 23 :Ehsan Akhgari 2011-01-26 21:16:34 PST
(In reply to comment #22)
> So, sigh, that would mean keeping the existing strings, making the places
> organizer use them for the new command, and fixing the private browsing stuff
> to ensure this command is disabled while in PB mode.

The command inside the places should not be disabled inside the private browsing mode, as it's not right now.  The original rationale for this was that the command is not available through the primary UI.
Comment 24 Steffen Wilberg 2011-01-27 15:11:31 PST
Created attachment 507673 [details] [diff] [review]
move Import to the Library, v1

"Import HTML..." used to let you choose between either importing from another browser or and importing an HTML file. If you chose the latter, it would open a file picker. Now it directly opens the file picker to let you import the HTML file.

The new "Import..." in the Library does the same as the old File->Import, letting you choose which sorts of data to import, including bookmarks.
I moved BrowserImport() to places.js, added a comment, used the Cc/Ci shorthands, but left it untouched otherwise.

Using the existing string from browser.dtd (which is already referenced by places.xul), there's no string change required. Ideally, "Import HTML..." would be relabeled to "Import Bookmarks HTML...", but that's not worth breaking string freeze, see comment 19.

(In reply to comment #21)
> Should keep this menuseparator, so the printing related items are
> demarked from the items above and below them.
Done, and removed the #ifndef XP_MACOSX <menuseparator/> instead.

> >-                <menuitem id="menu_import"
> 
> Need to fix onEnterPrivateBrowsing() and onExitPrivateBrowsing() [in
> browser.js], to not attempt to disable/enable this item.
Done.

(In reply to comment #23)
> The command inside the places should not be disabled inside the private
> browsing mode, as it's not right now.  The original rationale for this was
> that the command is not available through the primary UI.
Didn't replicate the menu item disabling in PB mode, and removed the test.
Comment 25 Steffen Wilberg 2011-01-27 15:41:55 PST
Created attachment 507686 [details] [diff] [review]
move "Import" to Library, v1.1

Removed debug leftover :-/
Comment 26 Steffen Wilberg 2011-02-01 13:16:36 PST
Created attachment 508871 [details] [diff] [review]
v1.1, unbitrotted

Unbitrotted (context has changed).
Comment 27 Justin Dolske [:Dolske] 2011-02-11 17:02:59 PST
Beta 12 will be closing soon, please land ASAP.
Comment 28 Dão Gottwald [:dao] 2011-02-12 01:17:19 PST
http://hg.mozilla.org/mozilla-central/rev/9698ac3f1c61
Comment 29 u60234 2011-02-12 02:51:17 PST
This has localization impact. In en-US and sv-SE, and probably other locales, there are now two menu items under "Import and Backup" that has "I" as access key. Fix or notify localizers?
Comment 30 Dão Gottwald [:dao] 2011-02-12 02:58:36 PST
-> backed out
Comment 31 Steffen Wilberg 2011-02-12 03:50:19 PST
Ah crap, I tried to fix this using existing strings, but didn't notice the double accesskey.

String changes are out of the question after "the last string landed".
So the only way to fix this bug for Firefox 4 would be to tolerate the double accesskey in this rather hidden place. You could still reach the menu item by pressing Alt+i to open the menu, and pressing "i" twice: once for "Import HTML..." and once for "Import..." (tested on Windows and Linux).

Either way, I'll fix this properly for the next release.
Comment 32 Dão Gottwald [:dao] 2011-02-12 04:03:16 PST
Apparently this killed mochitest-other on OS X, as well.
Comment 33 Dão Gottwald [:dao] 2011-02-12 04:09:42 PST
*** Bug 513167 has been marked as a duplicate of this bug. ***
Comment 34 Steffen Wilberg 2011-02-12 04:44:09 PST
Comment on attachment 508871 [details] [diff] [review]
v1.1, unbitrotted

Right, I messed that up as well. ;-(
I uncommented the #ifdef XP_MACOSX, resulting in a broken Library window, and the syntax error pointing right at it:

>+  browserImport: function PO_browserImport() {
>+#ifdef XP_MACOSX
>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             .getService(Ci.nsIWindowMediator);
The double "." before getService causes the syntax error breaking the Mac tests.

I then searched the log.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out

is not helpful at all. But several lines earlier, I finally found it:

TEST-INFO | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Console message: [JavaScript Error: "syntax error" {file: "chrome://browser/content/places/places.js" line: 373 column: 13 source: "             .getService(Ci.nsIWindowMediator);
"}]

Why do tests not fail when a syntax error occurs, but time out later on?
Comment 35 Steffen Wilberg 2011-02-15 10:32:35 PST
Dao or Dolske, can we do without the accesskey instead of duplicating it?

I just noticed that the entire History and Bookmarks menus lack accesskeys altogether, and I can't even find a bug about that...

Users can still navigate the menu using the arrow keys. You don't import bookmarks + passwords + cookies + history + preferences every day.
Comment 36 Dão Gottwald [:dao] 2011-02-15 10:44:31 PST
History and Bookmark menus don't have access key in order to allow history entries and bookmarks to be accessed with a key stroke.
Comment 37 Steffen Wilberg 2011-02-15 11:01:02 PST
Ah, I see.

So how about adding "Import" without an accesskey?
Comment 38 Dão Gottwald [:dao] 2011-02-15 11:12:25 PST
I think the window of opportunity for this bug has closed. It can be fixed properly after Firefox 4 has branched.
Comment 39 Steffen Wilberg 2011-03-31 12:44:28 PDT
Created attachment 523399 [details] [diff] [review]
v 1.2, new strings

Suggested new strings are:
Import Bookmarks from HTML… (accesskey I)
Export Bookmarks to HTML… (E)
Import Bookmarks, History, Passwords… (m)
Comment 40 Steffen Wilberg 2011-04-25 02:39:35 PDT
Created attachment 528069 [details] [diff] [review]
v1.2, unbitrotted

Alex, please comment on the strings I proposed in comment 39.
Comment 41 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 10:56:30 PDT
We can't really use commas in a menu command (at least I can't think of any existing menu commands that use commas)  How about:

Suggested new strings are:
Import Bookmarks from HTML… (accesskey I)
Export Bookmarks to HTML… (E)
Import from Another Browser (A)
Comment 42 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 10:57:58 PDT
Comment on attachment 528069 [details] [diff] [review]
v1.2, unbitrotted

ui-r+ with the strings tweaked from the last comment
Comment 43 Steffen Wilberg 2011-06-09 15:30:53 PDT
Created attachment 538365 [details] [diff] [review]
patch v1.3, with new string from comment 42

"Import from Another Browser…" (accesskey A) per comment 42.
Comment 44 Dão Gottwald [:dao] 2011-06-13 22:58:38 PDT
Comment on attachment 538365 [details] [diff] [review]
patch v1.3, with new string from comment 42

It looks like this would regress bug 464736... comment 23 seems wrong, since importing history and cookies actually didn't work in PB mode.

>   /**
>-   * Show the migration wizard for importing from a file.
>+   * Show the migration wizard for importing passwords,
>+     cookies, history, preferences, and bookmarks.
>    */

nit: add another asterisk

>-<!ENTITY cmd.exportHTML.label           "Export HTML…">
>+<!ENTITY cmd.exportHTML2.label          "Export Bookmarks to HTML…">

Can you change the entity name to something more sensible like exportBookmarksToHTML?
Comment 45 :Ehsan Akhgari 2011-06-14 14:14:04 PDT
(In reply to comment #44)
> Comment on attachment 538365 [details] [diff] [review] [review]
> patch v1.3, with new string from comment 42
> 
> It looks like this would regress bug 464736... comment 23 seems wrong, since
> importing history and cookies actually didn't work in PB mode.

Ah, right, yes.  This menu item should continue to be disabled in private browsing mode.  Sorry for my mistake.
Comment 46 Steffen Wilberg 2011-06-19 16:46:37 PDT
Created attachment 540359 [details] [diff] [review]
patch v2: menu item disabled in PB mode

Added the code to disable the menu item in private browsing mode, and rewrote the test.

The test actually tests the #OrganizerCommand_browserImport command instead of the #browserImport menuitem; the latter somehow failed some of the tests.

(In reply to comment #44)
> >   /**
> >-   * Show the migration wizard for importing from a file.
> >+   * Show the migration wizard for importing passwords,
> >+     cookies, history, preferences, and bookmarks.
> >    */
> 
> nit: add another asterisk
Done.

> >-<!ENTITY cmd.exportHTML.label           "Export HTML…">
> >+<!ENTITY cmd.exportHTML2.label          "Export Bookmarks to HTML…">
> 
> Can you change the entity name to something more sensible like
> exportBookmarksToHTML?
Done: importBookmarksFromHTML.label, exportBookmarksToHTML.label, importOtherBrowser.label.
Comment 47 Dão Gottwald [:dao] 2011-06-21 03:27:31 PDT
Comment on attachment 540359 [details] [diff] [review]
patch v2: menu item disabled in PB mode

>+  browserImport: function PO_browserImport() {
>+#ifdef XP_MACOSX
>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+    var win = wm.getMostRecentWindow("Browser:MigrationWizard");

use Services.wm

>+let gPOPrivateBrowsingUI = {

"gPO" is an irritatingly obscure prefix. "PO" seems redundant, too, since this is places.js.

>+  _privateBrowsingService: null,
>+  _cmd_import: null,
>+  _obs: null,
>+
>+  init: function PO_PB_init() {
>+    this._cmd_import = document.getElementById("OrganizerCommand_browserImport");
>+
>+    this._privateBrowsingService = Cc["@mozilla.org/privatebrowsing;1"].
>+                                   getService(Ci.nsIPrivateBrowsingService);
>+    if (this._privateBrowsingService.privateBrowsingEnabled)
>+      this.updateUI(true);

You're not using this._privateBrowsingService outside of this method.

>+    this._obs = Cc["@mozilla.org/observer-service;1"].
>+                getService(Ci.nsIObserverService);
>+    this._obs.addObserver(this, "private-browsing", false);
>+  },
>+
>+  uninit: function PO_PB_uninit() {
>+    this._obs.removeObserver(this, "private-browsing");

use Services.obs
Comment 48 Steffen Wilberg 2011-06-21 15:43:51 PDT
Created attachment 540918 [details] [diff] [review]
patch v2.1

Addressed comment 47.
Comment 49 Marco Bonardo [::mak] 2011-06-21 16:17:41 PDT
Comment on attachment 540918 [details] [diff] [review]
patch v2.1

Review of attachment 540918 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for taking care of this, just some drive-by comments

::: browser/components/places/content/places.js
@@ +373,2 @@
>     */
> +  browserImport: function PO_browserImport() {

since we have importFromFile, while is not this called importFromBrowser?

@@ +378,5 @@
> +      win.focus();
> +    else {
> +      window.openDialog("chrome://browser/content/migration/migration.xul",
> +                        "migration", "centerscreen,chrome,resizable=no");
> +    }

if you brace one part of an if/elseif/else you want to brace all of them.

@@ +382,5 @@
> +    }
> +#else
> +    window.openDialog("chrome://browser/content/migration/migration.xul",
> +                      "migration", "modal,centerscreen,chrome,resizable=no");
> +#endif

Btw, I'd rather (pseudocode)
ifdef MAC
// On Mac the window is not modal.
if (win) {
  win.focus();
  return;
}
let features = "..."
#else
let features = "..."
#endif
window.openDialog("chrome://browser/content/migration/migration.xul",
		  "migration", features);

@@ +1352,5 @@
> +/**
> + * Disables the "Import and Backup->Import From Another Browser" menu item
> + * in private browsing mode.
> + */
> +let gPrivateBrowsingUI = {

this name makes no sense, this is not a private browsing ui.
gPrivateBrowsingListener or gPrivateBrowsingHandler sounds better to me.

@@ +1372,5 @@
> +  },
> +
> +  observe: function PO_PB_observe(aSubject, aTopic, aData) {
> +    if (aTopic != "private-browsing")
> +      return;

this seems useless, tests would fail if this would ever happen.
Comment 50 Steffen Wilberg 2011-06-21 16:30:37 PDT
Created attachment 540928 [details] [diff] [review]
patch v2.2

Thanks for the comments.
Comment 51 Dão Gottwald [:dao] 2011-06-26 14:58:07 PDT
Comment on attachment 540928 [details] [diff] [review]
patch v2.2

>+<!ENTITY importBookmarksFromHTML.label     "Import Bookmarks from HTML…">
>+<!ENTITY importBookmarksFromHTML.accesskey "I">
>+<!ENTITY exportBookmarksToHTML.label       "Export Bookmarks to HTML…">
>+<!ENTITY exportBookmarksToHTML.accesskey   "E">
>+<!ENTITY importOtherBrowser.label          "Import from Another Browser…">

The last one should probably be called "Import Data from Another Browser…" or similar for it to contrast with "Bookmarks" from above.
Comment 52 Steffen Wilberg 2011-06-27 12:41:28 PDT
I went with "Import Data from Another Browser…".
http://hg.mozilla.org/integration/mozilla-inbound/rev/08734613c1ab
Comment 53 Marco Bonardo [::mak] 2011-06-28 02:26:46 PDT
http://hg.mozilla.org/mozilla-central/rev/08734613c1ab
Comment 54 Simona B [:simonab ] 2011-07-01 02:55:34 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110630 Firefox/7.0a1

Verified that "Import" from the File menu is now moved to the Library, on Windows XP, Windows 7, MacOS X 10.6 and Ubuntu.
Comment 55 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2011-07-01 06:03:28 PDT
Once we have merged to mozilla-aurora we will have to update the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=16699
Comment 56 Simona B [:simonab ] 2012-07-24 01:49:16 PDT
https://litmus.mozilla.org/show_test.cgi?id=16699 - the test case is updated
Comment 57 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-24 02:01:41 PDT
(In reply to Simona B [QA] from comment #56)
> https://litmus.mozilla.org/show_test.cgi?id=16699 - the test case is updated

This should not only be updated for Aurora. Given that this bug has been fixed for Firefox 7, we should also update the beta, latest release, and the esr10 branch.
Comment 58 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-24 02:02:17 PDT
I wonder if this even needs an update in our new manual testcase management system.
Comment 59 Simona B [:simonab ] 2012-07-24 02:40:18 PDT
The test case was updated in Litmus long time ago (2011-07-22 02:21:07) for all the branches, just forgot to change the Litmus flag.

The test case has not been imported to MozTrap yet, as most of the test cases from Litmus. I'm not sure yet if all the test cases will be imported. I shall investigate further on this.

Note You need to log in before you can comment on or make changes to this bug.