580660 - Switch SeaMonkey browser UI to places bookmarks

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

The SeaMonkey browser UI should be switched over to use places bookmarks instead of the old bookmarks system.

This is a split-off of part 4 of bug 498596, for easier review (as now that
part is isolated here).

Comment 1

[Robert Kaiser]

Attachment: v2: switch over the in-browser UI parts

This should switch over all our in-browser UI parts to use the new places system for bookmarks.

Comment 2

[Robert Kaiser]

Comment on attachment 459043 [details] [diff] [review]
v2: switch over the in-browser UI parts

Requesting additional review from Ian on all patches but the actual core - I'll take reviews from whoever gets around to it first.

Comment 3

[Ian Neal]

Comment on attachment 459043 [details] [diff] [review]
v2: switch over the in-browser UI parts

Other than function BrowserHome soon to be bitrotted by Ratty's patch on bug 529240 looks okay on code inspection. Testing of all patches still to happen.

Comment 4

[Ian Neal]

Just as a note this patch has already been bitrotted by a number of other patches in navigator.xul (utilityOverlay.js passes with fuzz).

Comment 5

[Robert Kaiser]

Attachment: v2.1: in-browser UI, unbitrotted

Here's an unbitrotted patch that also applies without dependencies on doorhangers. I'm asking ui-r from Neil for now, not sure if and how deeply he wants to look into this before checkin, which I hope to be able to do before the weekend, core review permitting, so that we have a few nightlies before we cut a3 early next week with this in.

Comment 6

[[e-mail]]

Comment on attachment 462466 [details] [diff] [review]
v2.1: in-browser UI, unbitrotted

>+  var target = !gBrowser ? "window": !aEvent ? "current" : whereToOpenLink(aEvent);
whereToOpenLink already knows how to deal with a null event. (Also as you're touching that line you could add a space before the colon.)

>     case "tab":
>+    case "tabshifted":
>       tab = gBrowser.addTab(homePage[0]);
>-      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>+      if (target != "tabshifted")
[whereToOpenLink doesn't take the background preference into account, which I guess might not be so bad for the home button (compare goAbout)]

>+var gBookmarkAllTabsHandler = {
>+  init: function () {
>+    this._command = document.getElementById("Browser:BookmarkAllTabs");
>+    gBrowser.tabContainer.addEventListener("TabOpen", this, true);
>+    gBrowser.tabContainer.addEventListener("TabClose", this, true);
>+    this._updateCommandState();
>+  },
etc...

>-function updateGroupmarkCommand()
>-{
>-  const disabled = (!gBrowser || gBrowser.browsers.length == 1);
>-  document.getElementById("Browser:AddGroupmarkAs")
>-          .setAttribute("disabled", disabled);
>-}
Wow, all that overkill just to update one menuitem... Why 11 lines of gBookmarkAllTabsHandler when you can do it in two statements?

>+  // XXX: need to actually deal with the postData, see bug 553459
Actually it's only the new window case that's hard.

>+    <!-- Bookmarks and history tooltip -->
>+    <tooltip id="bhTooltip"/>
History?

>-                           event.stopPropagation();
>-                           HandleBookmarkIcon(this.src, true);"
>+                           event.stopPropagation();"
>                    onerror="gBrowser.addToMissedIconCache(this.src); HandleBookmarkIcon(this.src, false);"
This is the only place where favicon.ico icons are validated. Does the favicon service automatically fall back to favicon.ico? What about the error case?

>+        <hbox flex="1"
>+              id="PlacesToolbar"
>+              context="placesContext"
>+              onclick="BookmarksEventHandler.onClick(event);"
>+              oncommand="BookmarksEventHandler.onCommand(event);"
>+              tooltip="bhTooltip"
>+              popupsinherittooltip="true">
...

>+            <scrollbox orient="horizontal"
>+                       id="PlacesToolbarItems"
>+                       flex="1"/>
Do we get scrolling, or is this just a lazy way of cropping bookmarks?

>+            <toolbarbutton type="menu"
>+                           id="PlacesChevron"
>+                           class="chevron"
>+                           mousethrough="never"
>+                           collapsed="true"
>+                           tooltiptext="&bookmarksToolbarChevron.tooltip;"
>+                           onpopupshowing="document.getElementById('PlacesToolbar')
>+                                                   ._placesView._onChevronPopupShowing(event);">
document.getElementById('PlacesToolbar') == this.parentNode.parentNode, no?

>+    <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/>
This should be Ctrl+Shift+D, no? Ctrl+Alt+D is unsafe on Windows.
(I think Ctrl+Shift+D is now safe on Linux again.)

>-  <menu id="tasksMenu">
>+    <menu id="tasksMenu">
Nit: not strictly necessary as part of this bug.

>+window.addEventListener("SidebarFocused",
>+                        function()
>+                          document.getElementById("search-box").focus(),
>+                        false);
XPFE sidebar uses elementtofocus="search-box"

>diff --git a/suite/common/bookmarks/browser-places.js b/suite/common/bookmarks/browser-places.js
[Not reviewed yet]

>+    Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>+    Components.utils.import("resource:///modules/PlacesUIUtils.jsm");
Shouldn't these belong in one of the places JavaScript files?

>   addBookmarkForFrame: function() {
Why does this work differently to bookmarkThisPage?

>+var TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab";
???

>+      var topWindow = wm.getMostRecentWindow("navigator:browser");
What if there isn't one?

>+<!ENTITY bookmarksManager.width         "700">
>+<!ENTITY bookmarksManager.height        "500">
Why are these in the .dtd?

> <!ENTITY feedMessenger "News &amp; Blogs">
>+<!ENTITY feedLiveBookmarks "Live Bookmarks">
[What order are they in the menu?]

>+.bookmark-item[container][livemark] .bookmark-item,
> menupopup >

No Modern styles yet, I see...

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> [whereToOpenLink doesn't take the background preference into account, which I
> guess might not be so bad for the home button (compare goAbout)]

Is that a bug or feature?

> Wow, all that overkill just to update one menuitem... Why 11 lines of
> gBookmarkAllTabsHandler when you can do it in two statements?

Well, for one thing, their variant is more performant, for the other, I like their removing the attribute instead of setting it to "false" and creating yet another value it can be in.
For the other, it would be nice if some things I'm porting are friendly for merging future changes in the original - but oh, well, whatever.

> >+  // XXX: need to actually deal with the postData, see bug 553459
> Actually it's only the new window case that's hard.

Still, let's put that off into the followup, please?

> >+    <!-- Bookmarks and history tooltip -->
> >+    <tooltip id="bhTooltip"/>
> History?

It's called that, yes. And it will be used as that. Believe me. Please?

> >-                           event.stopPropagation();
> >-                           HandleBookmarkIcon(this.src, true);"
> >+                           event.stopPropagation();"
> >                    onerror="gBrowser.addToMissedIconCache(this.src); HandleBookmarkIcon(this.src, false);"
> This is the only place where favicon.ico icons are validated. Does the favicon
> service automatically fall back to favicon.ico? What about the error case?

I don't understand what you are saying here, sorry.
You're right that the error case also needs HandleBookmarkIcon() removed.
I think we need to handle fully using the faviconservice in a different bug anyhow, there's still a number of unclarities on our side about that icon handling, so far we only really tied places-related code and setIcon() into it. But let's investigate and clean that up in a followup, please. Thanks for bringing it up, though.

> >+            <scrollbox orient="horizontal"
> >+                       id="PlacesToolbarItems"
> >+                       flex="1"/>
> Do we get scrolling, or is this just a lazy way of cropping bookmarks?

Possibly the latter, from what I see, we are at least handling the appearance of the overflow chevron better than previously, where it could in some cases clash with the last displayed bookmark.

> >+            <toolbarbutton type="menu"
> >+                           id="PlacesChevron"
> >+                           class="chevron"
> >+                           mousethrough="never"
> >+                           collapsed="true"
> >+                           tooltiptext="&bookmarksToolbarChevron.tooltip;"
> >+                           onpopupshowing="document.getElementById('PlacesToolbar')
> >+                                                   ._placesView._onChevronPopupShowing(event);">
> document.getElementById('PlacesToolbar') == this.parentNode.parentNode, no?

Not that it would be more failsafe or shorter, but whatever you say.

> >+    <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/>
> This should be Ctrl+Shift+D, no? Ctrl+Alt+D is unsafe on Windows.
> (I think Ctrl+Shift+D is now safe on Linux again.)

I've struggled for some time to even find a variant that works somehow here. We have THREE key combinations with D here, Ctrl+Shift+D is already bookmarking a group of tabs. Probably, in a followup, we should just kill one or two of those options, but I'm out of options here (pun intended). :(

> >-  <menu id="tasksMenu">
> >+    <menu id="tasksMenu">
> Nit: not strictly necessary as part of this bug.

True, though the old indentation is wrong and confusing. Should I put it in a followup instead?

> >+window.addEventListener("SidebarFocused",
> >+                        function()
> >+                          document.getElementById("search-box").focus(),
> >+                        false);
> XPFE sidebar uses elementtofocus="search-box"

Sounds like another place where we finally should change interfaces to be compatible, whoever changes, we better make sure we do. Could file that as you seem to know things there that I don't, please?

> >+    Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> >+    Components.utils.import("resource:///modules/PlacesUIUtils.jsm");
> Shouldn't these belong in one of the places JavaScript files?

Logically not, IMHO, we could probably argue about that and find arguments in favor of either way, but I'd like to stick with at least matching Firefox here, if possible.

> >   addBookmarkForFrame: function() {
> Why does this work differently to bookmarkThisPage?

Because displaying the inline panel dropping from urlbar is probably wrong for a frame that has a different URI then the urlbar.

> >+var TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab";
> ???

See http://mxr.mozilla.org/comm-central/search?string=TAB_DROP_TYPE - the controller needs it. And we probably should get this stuff into tabbrowser as well some time anyhow.

> >+      var topWindow = wm.getMostRecentWindow("navigator:browser");
> What if there isn't one?

Do we even get here if that's the case?

> >+<!ENTITY bookmarksManager.width         "700">
> >+<!ENTITY bookmarksManager.height        "500">
> Why are these in the .dtd?

Because localizers may need to change them to make sure everything fits. Maybe this is overdoing things, not having this awkward toolbar that Firefox brings, but it usually doesn't harm to have a knob there for localizers.

> > <!ENTITY feedMessenger "News &amp; Blogs">
> >+<!ENTITY feedLiveBookmarks "Live Bookmarks">
> [What order are they in the menu?]

I hope that one, or I introduced a bug.

> >+.bookmark-item[container][livemark] .bookmark-item,
> > menupopup >
> 
> No Modern styles yet, I see...

Hmm, I should care about those, right. Given the size of this work, I'll put it in a followup as well, though. Thanks for the reminder.

Comment 8

[Robert Kaiser]

Attachment: v2.2: in-browser UI, updated for comments

Here's the updated patch for Neil's comments.

Comment 9

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/05e7e38f82ea after a formal "go" from Neil.

Comment 10

[[e-mail]]

Comment on attachment 463197 [details] [diff] [review]
v2.2: in-browser UI, updated for comments

I wanted to comment on (well, several) bugs, but none of the patches here match the bug number quoted on the changeset for the blamed line...

>+/* ::::: bookmarks toolbar ::::: */
>+
>+toolbarpaletteitem[place="palette"] > toolbaritem > hbox[type="places"] {
>+  display: none;
This doesn't hide the overflow chevron. Also it doesn't work in the palette. You also didn't remove the existing rules that hide the places items, which for some reason live in the skin.

Comment 11

[[e-mail]]

Comment on attachment 463197 [details] [diff] [review]
v2.2: in-browser UI, updated for comments

Aha, this one was here all along. Sorry for overlooking it.

>+      openDialog("chrome://communicator/content/bookmarks/bookmarksManager.xul",
>+                 "", "chrome,toolbar=yes,dialog=no,resizable", aLeftPaneRoot);
These chrome flags are incorrect. They should be "all,dialog=no".

Comment 12

[Robert Kaiser]

Comments addressed in attachment 465432 [details] [diff] [review] over in bug 585601.

Comment 13

[[e-mail]]

Comment on attachment 463197 [details] [diff] [review]
v2.2: in-browser UI, updated for comments

>+  _updateCommandState: function BATH__updateCommandState(aTabClose) {
>+    var numTabs = gBrowser.browsers.length;
Nit: gBrowser.tabs.length

>+    // The TabClose event is fired before the tab is removed from the DOM
>+    if (aTabClose)
>+      numTabs--;
>+
>+    if (numTabs > 1)
>+      this._command.removeAttribute("disabled");
>+    else
>+      this._command.setAttribute("disabled", "true");
In theory you only need to check numTabs when closing a tab; after opening a tab I would be very surprised if there was only one!

>     <!-- Bookmarks Menu -->
>+    <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/>
Unfortunately Ctrl+Alt key combos are reserved on Windows. (Also, why was Ctrl+Shift+D moved to bookmark all tabs?)