436069 - Unify history, bookmarks, awesomebar search and remote tabs into a single UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P3)

Description

[Christian Sejersen]

Comment 1

[Stuart Parmenter]

Christian says similar to Firefox's Show All History

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.1 - dirty wip

This wip just allow to have a new button next to "Show all bookmarks" in the awesomebar to display the history. 
This is not a real patch but just something that do the job.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.2

The wip is still dirty but add the basic behavior we want. There is still a few bugs into the selection of right popup and the style is ugly.

Code also needs to be cleaned.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.3

The wip still not: 
 * correctly update the navigation icons (reload, go, edit) when entering bookmarks or history
 * The way it is implement make it hard for extensions developers to add a new element on the row)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - part1

This patch change the top row for the awesome panel, bookmarks and also add an history view based on the display of bookmarks and the awesome results.

This top header row is extensible by addons developers to add their own easily by adding an entry in the row and a command to execute when opening this row, which can be to open a separated panel. this way the code of their custom panel could be separate from the logic of our awesome panel.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - part2

This patch add a basic support for remote tabs and correct a few bugs introduced in part 1.

Mark, if you prefer I can output one single patch?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - part3

This patch is on top of the two others and clean up HistoryList/BookmarkList/RemoteTabsList to have a single unified api.
It also fixes a few bugs discovered while testing with the two others patches applied.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460725 [details] [diff] [review]
Patch - part3

>diff -r fcbe60e5b5e3 chrome/content/bindings.xml
>               event.initEvent("BookmarkOpen", true, false);
>               this.dispatchEvent(event);
>               let func = new Function("event", this.getAttribute("onopen"));
>-              func.call(this, event);
>+              func.call(this, aEvent);

Why this change? Don't we want to send the "BookmarkOpen" event?

>diff -r fcbe60e5b5e3 chrome/content/browser.xul

>+    <!-- Awesome panels container -->
>+    <placelist id="bookmarks-items" type="bookmarks" onopen="BookmarkList.openLink(event);" onclose="BrowserUI.updateStar();" header="awesome_header_row" flex="1" hidden="true"/>
>+    <historylist id="history-items" onopen="HistoryList.openLink(event);" header="awesome_header_row" flex="1" hidden="true"/>
>+    <remotetabslist id="remotetabs-items" onopen="RemoteTabsList.openLink(event)" header="awesome_header_row" flex="1" hidden="true"/>

I'd like to see all of these wrapped in a parent, probably the <vbox id="popup_autocomplete">

I would also like to see the header placed inside the same parent and we can remove the "header" attribute from the lists. Unless I am missing something about how the header is added to the lists.

It is odd that there is no "all-pages" list here. Those items are in the XBL binding itself.

Overall, I think the patches are very good. Let's talk about potentially moving the XUL for the lists and what impact that could have. I also like the AwesomePanel idea.

Can you roll these 3 patch up into a single patch so I can review them all at once?

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

(In reply to comment #8)
> Comment on attachment 460725 [details] [diff] [review]
> Patch - part3
> 
> >diff -r fcbe60e5b5e3 chrome/content/bindings.xml
> >               event.initEvent("BookmarkOpen", true, false);
> >               this.dispatchEvent(event);
> >               let func = new Function("event", this.getAttribute("onopen"));
> >-              func.call(this, event);
> >+              func.call(this, aEvent);
> 
> Why this change? Don't we want to send the "BookmarkOpen" event?

We're still sending it and on the "onopen" callback it makes sense to pass the original event so people can have the target of the click instead of using the activeItem property.
> 
> >diff -r fcbe60e5b5e3 chrome/content/browser.xul
> 
> >+    <!-- Awesome panels container -->
> >+    <placelist id="bookmarks-items" type="bookmarks" onopen="BookmarkList.openLink(event);" onclose="BrowserUI.updateStar();" header="awesome_header_row" flex="1" hidden="true"/>
> >+    <historylist id="history-items" onopen="HistoryList.openLink(event);" header="awesome_header_row" flex="1" hidden="true"/>
> >+    <remotetabslist id="remotetabs-items" onopen="RemoteTabsList.openLink(event)" header="awesome_header_row" flex="1" hidden="true"/>
> 
> I'd like to see all of these wrapped in a parent, probably the <vbox
> id="popup_autocomplete">
> 
> I would also like to see the header placed inside the same parent and we can
> remove the "header" attribute from the lists. Unless I am missing something
> about how the header is added to the lists.

I've done some tweaks about where they live into browser.xul, let me know what you think now.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 - minor corrections

Correct some minor bugs discovered while browsing.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460873 [details] [diff] [review]
Patch v0.2 - minor corrections

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>       </method>
>+       <method name="showHistoryPopup">

Why is this method in here? Don't we get it from:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#355

Also, <method> indent seems off by 1 space

>       <handler event="keypress" keycode="VK_RETURN" phase="capturing">
>         <![CDATA[
>-          if (this.popup.allBookmarksItemSelected) {
>-            this.popup.closePopup();
>+          if (this.popup.headerSelected) {
>+            BrowserUI.activePanel = null;
>             CommandUpdater.doCommand("cmd_bookmarks");
>           }
>+          else {
>+            setTimeout(function() { BrowserUI.activePanel = null }, 0);
>         ]]>

cuddle the "else" and you seem to be missing the final "}" for the else

>       <property name="selectedIndex"
>-                onget="return this._allBookmarksItem._hidden ? this._selectedIndex : this._selectedIndex - 1;">
>+                onget="return this._header.hidden ? this._selectedIndex : this._selectedIndex - 1;">

I was kind of hoping that if we re-organized the XUL for the "all pages" we would not need to handle the header mixed in with the results.

>@@ -615,26 +640,26 @@
>   </binding>
> 
>   <binding id="place-item" extends="chrome://browser/content/bindings.xml#place-base">

>-          <xul:label anonid="bookmark-url" class="bookmark-item-url" xbl:inherits="value=uri" crop="center" mousethrough="always"/>
>+          <xul:label anonid="bookmark-url" class="bookmark-item-url" xbl:inherits="value=url" crop="center" mousethrough="always"/>

Why the change from "uri" to "url"?

>       <xul:hbox anonid="bookmark-manage" class="bookmark-manage" hidden="true" flex="1">
>-            <xul:textbox anonid="uri" xbl:inherits="value=uri"/>
>+            <xul:textbox anonid="uri" xbl:inherits="value=url"/>

Same

>       <method name="_getChildren">
>+            if (this._mode == "folders" && node.type == node.RESULT_TYPE_FOLDER) {
>+              items.push(node);
>             }

Remove the {} for a single line

>+            else if (this._mode == "") {
>+              items.push(node);
>+            }

Remove the {} for a single line

>+      <method name="open">

>-            while (children.firstChild)
>-              children.removeChild(children.firstChild);
>+            let header = this._header;
>+            while (children.lastChild && children.lastChild != header)
>+              children.removeChild(children.lastChild);

Header affects this list too?

>       <method name="createItem">

>-            child.setAttribute("uri", aItem.uri);
>+            child.setAttribute("url", aItem.uri);

Same

>+  <binding id="history-list">

>+      <method name="createItem">

>+            if (aItem.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY) {
>+              child.setAttribute("class", "history-item-title");
>+            }
>+            else {

cuddle the "else"

>+  <binding id="remotetabs-list">

>+      <method name="createItem">

>+            if (aItem.name) {
>+              child.setAttribute("class", "remotetabs-item-title");
>+            }
>+            else {

cuddle the "else"

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  set activePanel(aPanel) {
>+    let container = document.getElementById("awesome-panels");
>+    if (aPanel) {
>+      container.hidden = false;
>+      aPanel.open();
>+    }
>+    else {

cuddle the "else"    } else {

>   showAutoComplete: function showAutoComplete() {

>     this._hidePopup();
>+    this.activePanel = AllPagesList;
>     this._edit.showHistoryPopup();

I wonder if the | this._edit.showHistoryPopup(); | call could be put into the AllPagesList.open method somehow? If so, we could file a followup bug for moving it.


>+var AwesomePanel = function(aElementId, aCommandId) {

>+  this.open = function aw_open() {

>+    if (items.hasAttribute("onshow")) {
>+      let func = new Function("event", items.getAttribute("onshow"));
>+      func.call(items);
>+    } else {
>+      if (items.open)
>+        items.open();
>+    }

Don't we want the "else" block called all the time?

>+  this.close = function aw_close() {

>+    if (items.hasAttribute("onhide")) {
>+      let func = new Function("event", items.getAttribute("onhide"));
>+      func.call(items);
>+    } else {
>+      if (items.close)
>+        items.close();
>+    }

Same here

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+    <vbox id="awesome-panels" hidden="true" header="awesome_header_row">
>+      <!-- Awesome header row -->
>+      <hbox id="awesome_header_row" hidden="true">

id="awesome-header"

Much nicer XUL organization in this patch! Thanks

>diff --git a/components/AboutRedirector.js b/components/AboutRedirector.js

>-  },
>-  "sync-tabs": {
>-    uri: "chrome://browser/content/aboutTabs.xhtml",
>-    privileged: true

What about AboutSyncTabs and the compoonents list?

http://mxr.mozilla.org/mobile-browser/source/components/AboutRedirector.js#148
http://mxr.mozilla.org/mobile-browser/source/components/AboutRedirector.js#155

You also have some other files to remove:
http://mxr.mozilla.org/mobile-browser/find?text=&string=aboutTabs
aboutTabs.css
aboutTabs.dtd

>diff --git a/themes/core/browser.css b/themes/core/browser.css

>+historylist placeitem:active:not([selected="true"]):not([class$="title"]),
>+remotetabslist placeitem:active:not([selected="true"]):not([class$="title"]),

I am a little worried about depending on the class$ selector. It might be harder to maintain when updating code. Easier to break things. It might be slower too.

>-/* special "no results" and "all bookmarks" items */
>+/* special "no results", "awesome header row" and "title rows" items */
>+autocompleteresult[class$="title"] {

You could just list the selectors here and it might be faster and clearer

autocompleteresult.history-item-title,
autocompleteresult.remotetabs-item-title {

>+autocompleteresult[class$="title"] .bookmark-item-url {
>+autocompleteresult[class$="title"] .bookmark-item-label {
>+autocompleteresult[class$="title"] image {

Same

>+#awesome_header_row {
>+  min-height: 70px; /* row size */
> }

#awesome-header

>+#awesome_header_row > toolbarbutton {

Same

>+@media (max-width: 499px) {
>+  #awesome_header_row > toolbarbutton {
>+    -moz-box-orient: vertical;
>+  }
>+
>+  #awesome_header_row > toolbarbutton .toolbarbutton-text {
>+    font-size: 18px !important;

Same

>+#awesome_header_row > toolbarbutton {
>+  -moz-appearance: none;
>+  color: white;
>+  font-weight: bold;
>+  border: 1px solid #36373b;
>+  border-left-width: 0;
>+  background-color: #797979;
>+  -moz-box-flex: 1;
> }

Isn't this rule | #awesome_header_row > toolbarbutton | above too?

This patch looks great! You are really close to r+

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3 - Adress comments

(In reply to comment #11)
> Comment on attachment 460873 [details] [diff] [review]
> Patch v0.2 - minor corrections
> 
> >diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
> >       </method>
> >+       <method name="showHistoryPopup">
> 
> Why is this method in here? Don't we get it from:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#355

This is here because I have add the header insertion here to prevent a flickering when going from bookmarks/history/desktop to "All Pages".


> >       <property name="selectedIndex"
> >-                onget="return this._allBookmarksItem._hidden ? this._selectedIndex : this._selectedIndex - 1;">
> >+                onget="return this._header.hidden ? this._selectedIndex : this._selectedIndex - 1;">
> 
> I was kind of hoping that if we re-organized the XUL for the "all pages" we
> would not need to handle the header mixed in with the results.
> 

> 
> >+      <method name="open">
> 
> >-            while (children.firstChild)
> >-              children.removeChild(children.firstChild);
> >+            let header = this._header;
> >+            while (children.lastChild && children.lastChild != header)
> >+              children.removeChild(children.lastChild);
> 
> Header affects this list too?

There is probably many stuffs to improve into the way the "header" is move between the fiels, I think we could enhance the "header" managing code into a followup if needed (to prevent these weird cases :))


> >@@ -615,26 +640,26 @@
> >   </binding>
> > 
> >   <binding id="place-item" extends="chrome://browser/content/bindings.xml#place-base">
> 
> >-          <xul:label anonid="bookmark-url" class="bookmark-item-url" xbl:inherits="value=uri" crop="center" mousethrough="always"/>
> >+          <xul:label anonid="bookmark-url" class="bookmark-item-url" xbl:inherits="value=url" crop="center" mousethrough="always"/>
> 
> Why the change from "uri" to "url"?

As said on IRC, this is to have a common way to access the url of an element just clicked into the openLink method of the AwesomePanel helper.
This could be revert if needed.


> 
> >   showAutoComplete: function showAutoComplete() {
> 
> >     this._hidePopup();
> >+    this.activePanel = AllPagesList;
> >     this._edit.showHistoryPopup();
> 
> I wonder if the | this._edit.showHistoryPopup(); | call could be put into the
> AllPagesList.open method somehow? If so, we could file a followup bug for
> moving it.

Done with the "onshow" attribute.


> >+var AwesomePanel = function(aElementId, aCommandId) {
> 
> >+  this.open = function aw_open() {
> 
> >+    if (items.hasAttribute("onshow")) {
> >+      let func = new Function("event", items.getAttribute("onshow"));
> >+      func.call(items);
> >+    } else {
> >+      if (items.open)
> >+        items.open();
> >+    }
> 
> Don't we want the "else" block called all the time?

Not really. The AwesomePanel helper act as a wrapper around xbl object that possibly does not have open/close.

> 
> >+  this.close = function aw_close() {
> 
> >+    if (items.hasAttribute("onhide")) {
> >+      let func = new Function("event", items.getAttribute("onhide"));
> >+      func.call(items);
> >+    } else {
> >+      if (items.close)
> >+        items.close();
> >+    }
> 
> Same here

Same response as above.

> >diff --git a/components/AboutRedirector.js b/components/AboutRedirector.js
> 
> >-  },
> >-  "sync-tabs": {
> >-    uri: "chrome://browser/content/aboutTabs.xhtml",
> >-    privileged: true
> 
> What about AboutSyncTabs and the compoonents list?
> 
> http://mxr.mozilla.org/mobile-browser/source/components/AboutRedirector.js#148
> http://mxr.mozilla.org/mobile-browser/source/components/AboutRedirector.js#155
> 
> You also have some other files to remove:
> http://mxr.mozilla.org/mobile-browser/find?text=&string=aboutTabs
> aboutTabs.css
> aboutTabs.dtd

oups! Done.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460909 [details] [diff] [review]
Patch v0.3 - Adress comments

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+      <method name="showHistoryPopup">

>+          this.popup._items.insertBefore(this.popup._header, this.popup._items.firstChild);

This is the reason for needing a copy of this method?

I am not a fan of the "uri" -> "url" change, but I won't let that block the patch

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+var AwesomePanel = function(aElementId, aCommandId) {
>+  let items = document.getElementById(aElementId);
>+  let command = document.getElementById(aCommandId);
>+
>+  this.open = function aw_open() {

>+    if (items.hasAttribute("onshow")) {
>+      let func = new Function("event", items.getAttribute("onshow"));
>+      func.call(items);

I don't think you want to use "event" as the argument name for the | items | parameter. "panel" ?

>+    } else if (items.open) {
>+        items.open();
>+    }

I'm still not sure I understand why we only call items.open() if there is no "onshow" handler. Wouldn't we always call items.open(), if it existed?

I'm using the patch in my local build to get a feel for it. The biggest usability issue I have is:
* The header scrolls out of view

I think the header should be stuck at the top of the list, unless it is hidden when I start typing to search for an item.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #13)
> Comment on attachment 460909 [details] [diff] [review]
> Patch v0.3 - Adress comments
> 
> >diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
> >+      <method name="showHistoryPopup">
> 
> >+          this.popup._items.insertBefore(this.popup._header, this.popup._items.firstChild);
> 
> This is the reason for needing a copy of this method?

It is. I need to put the node insertion here to prevent a flickering.

> 
> I am not a fan of the "uri" -> "url" change, but I won't let that block the
> patch

It is easy for me to revert back this change and just check for url|uri into the openLink method

> 
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+    } else if (items.open) {
> >+        items.open();
> >+    }
> 
> I'm still not sure I understand why we only call items.open() if there is no
> "onshow" handler. Wouldn't we always call items.open(), if it existed?

Right. I will correct that.

> 
> I'm using the patch in my local build to get a feel for it. The biggest
> usability issue I have is:
> * The header scrolls out of view
> 
> I think the header should be stuck at the top of the list, unless it is hidden
> when I start typing to search for an item.

As seen on IRC I think that's what expected. Thanks for the email sent to Madhava to clarify this point.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4

Address comments and change the header row to not move when panning (which allow to remove a lot of hacks) as asked by both Mark and Madhava.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461564 [details] [diff] [review]
Patch v0.4

Seems to be working great! I have rendering issues though. The content canvas is trying very hard to draw on top of the awesomescreen. See:

http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-awesomescreen-broken-01.png
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-awesomescreen-broken-02.png

We may need to wait for the Layers bug (bug 579349) to be fixed before landing this :(

Comment 17

[Tony Chung [:tchung]]

flagging for 2.0

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #16)
> Comment on attachment 461564 [details] [diff] [review]
> Patch v0.4
> 
> Seems to be working great! I have rendering issues though. The content canvas
> is trying very hard to draw on top of the awesomescreen. See:
> 
> http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-awesomescreen-broken-01.png
> http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-awesomescreen-broken-02.png
> 
> We may need to wait for the Layers bug (bug 579349) to be fixed before landing
> this :(

It works fine now bug 579349 has landed.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (faster history)

This is the same as Vivien's patch but I added code to limit the amount of history returned. | query.maxResults | is not used by the date query so I just check as we fill the items array and break when we exceed the limit.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461564 [details] [diff] [review]
Patch v0.4

Overall this patch looks good, but we can't land without making history faster (see other patch) and for some reason, remote tabs are not working correctly either.

r+ for the patch in general, but let's fix the history and remote tabs before landing.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #20)
> Comment on attachment 461564 [details] [diff] [review]
> Patch v0.4
> 
> Overall this patch looks good, but we can't land without making history faster
> (see other patch) and for some reason, remote tabs are not working correctly
> either.
> 
> r+ for the patch in general, but let's fix the history and remote tabs before
> landing.

Remote tabs is working for me, which error are you seeing?
Is the "Remote tabs" button is always disabled?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)

> Remote tabs is working for me, which error are you seeing?
> Is the "Remote tabs" button is always disabled?

The button seems to work fine. It starts "disabled", then becomes "enabled" when Sync auto connects. Clicking the button after it is enabled shows zero remote tabs.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.5

(In reply to comment #22)
> (In reply to comment #21)
> 
> > Remote tabs is working for me, which error are you seeing?
> > Is the "Remote tabs" button is always disabled?
> 
> The button seems to work fine. It starts "disabled", then becomes "enabled"
> when Sync auto connects. Clicking the button after it is enabled shows zero
> remote tabs.

WFM and Fabrice.



The new patch use a RESULTS_AS_URI instead of using DATE grouping to be faster and do a simple sort manually as you have suggested. This is way much faster! (even on a big profile)

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 466998 [details] [diff] [review]
Patch v0.5

> +  updateAwesomeHeader: function updateAwesomeHeader(aVisible) {
> +    document.getElementById('awesome-header').hidden = aVisible;
> +  },
> +

Use "

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Tests update based on bug 582745

This doesn't add new tests but make them green again with the changes in the way to access Bookmarks this patch do (and also add a few fixes discovered during testing).

The patch include a lot of whitespaces removal into tests and a few var -> let rename.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 467746 [details] [diff] [review]
Tests update based on bug 582745

> var BookmarkHelper = {
>   _panel: null,
>   _editor: null,
> 
>+  get mobileRoot() {

You move "mobileRoot" to BookmarkHelper, but I think bindings.xml expects "mobileRoot" to exist on the BookmarkList too. I found at least this usage:

  if (aRootFolder == this.mobileRoot && !this.isDesktopFolderEmpty())
    items.unshift(this._desktopFolder);

Any reason to move it?

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Tests update based on bug 582745 V0.2

(In reply to comment #26)
> Comment on attachment 467746 [details] [diff] [review]
> Tests update based on bug 582745
> 
> > var BookmarkHelper = {
> >   _panel: null,
> >   _editor: null,
> > 
> >+  get mobileRoot() {
> 
> You move "mobileRoot" to BookmarkHelper, but I think bindings.xml expects
> "mobileRoot" to exist on the BookmarkList too. I found at least this usage:
> 
>   if (aRootFolder == this.mobileRoot && !this.isDesktopFolderEmpty())
>     items.unshift(this._desktopFolder);
> 
> Any reason to move it?

Your comment makes me have a second look and I found that BookmarkList.mobileRoot is basically a close of what is done into bindings.xml and is unuseful, so I just removed it in the attached patch and allow "panel" to be accessed via AwesomePanel (tests and code has been updated for that)

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Tests update based on bug 582745 V0.3

Same but with a fix for the handleEscape case found while debugging

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/e32e61335714
http://hg.mozilla.org/mobile-browser/rev/b9a48885cb16

Comment 30

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100825 Namoroka/4.0b5pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100825 Namoroka/4.0b5pre Fennec/2.0a1pre

We're going to need to re-do on Location Bar testsuite. Aaron, you're leading the cause there.