1248267 - Right click on bookmark item of "Recently Bookmarked" should show regular places context menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Alice0775 White]

Steps To Reproduce:
1. Open Bookmarks menu
2. Right click on bookmark item of "last 5 Recently Bookmarked"

Actual Results:
Open it in tab
And Context menu has only 5 menu items

Expectedd Results:
Should not open it in tab.
Regular placescontext should pop up.
i.e. Context menu should be 12 items

Comment 2

[marcino245]

As for the "Context menu should be 12 items" - I already also poked an another related bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1248268

It seems that there is a mix of bookmarks related issues in the latest Nightly.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

probably for now right click should do nothing. Implementing the whole Places context menu is a complex task.

Comment 4

[Hussam Al-Tayeb]

Can you simply make it not open the link? It is poor user experience to open links when the user didn't intend to do so.

Comment 5

[Hussam Al-Tayeb]

Attachment: Screenshot from 2016-02-23 16-12-44.png

This is the current context menu in the "recently bookmarked" section. Those are the options Firefox normally shows for folders and NOT for links which makes this a bit confusing for users.
It would make sense to disable the context menu entirely in this section until a more appropriate one is added.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, the contextual menu on places menus static content refers to the root folder... It's confusing and we already evaluated removing contextual menus on static content many years ago (bug 429469)

Comment 8

[Hussam Al-Tayeb]

I don't understand the title of this bug. I think this is a reference to when right clicking used to open the link.
My bug 1261660 was about adding a proper context menu. Are you sure it is a duplicate?

Comment 9

[YF (Yang)]

(In reply to Hussam Al-Tayeb from comment #8)
> I don't understand the title of this bug. I think this is a reference to
> when right clicking used to open the link.
> My bug 1261660 was about adding a proper context menu. Are you sure it is a
> duplicate?

See the comment 0, although it is vague, but it means the type of context menu.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #6)
> yes, the contextual menu on places menus static content refers to the root
> folder... It's confusing and we already evaluated removing contextual menus
> on static content many years ago (bug 429469)

So when I set `item._placesNode = node` in `BookmarkingUI#_updateRecentBookmarks()`, I get half-way there. Of course, the drag-n-drop controller starts barfing because it can't find a parent node, yadayada, but is this remotely a direction worth pursuing nonetheless?

As a stop-gap measure, I'd like to disable all pointer interactions for these five items (including drag-n-drop), except left click. After that I'd like to learn how I can implement an interaction model properly.
Marco, can you give me a pointer where I should be looking?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Hi Mike, please check https://bugzilla.mozilla.org/show_bug.cgi?id=1248268#c48

I don't think we'll ever be able to have a complete places context menu here, since it would be hackish (lots of "simulated" nodes). Since these entries support a much smaller subset of actions than common bookmarks (atm), I'd rather suggest to remove the existing places contextual menu on anything that doesn't have a _placesNode attached and build a new contextual menu for these items.
I think you should coordinate with Dao about that.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Regarding drag & drop, places also supports unicode drag flavors that IIRC are something like "url\ntitle" so you may be able to create a datatransfer accepted by places views "easily".

Comment 13

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> As a stop-gap measure, I'd like to disable all pointer interactions for
> these five items (including drag-n-drop), except left click.

The patch in bug 1248267 basically does that. It makes "Hide Recently Bookmarked" the only context menu entry for these items.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

Dão, I tried to do it this way around by not disabling context menus after all. This can be extended to bug 1248616 for DnD.
It looks like the controller disables/ hides the correct items for us this way.
The setTimeout() needs to be introduced for the bookmarks menu as a subview (try it!)

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8745350 [details] [diff] [review]
Patch v1: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

Linux (and prolly Windows too) support context menus on items inside the main menu too. So this is not a proper fix.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8745403 [details] [diff] [review]
Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -1359,28 +1359,27 @@ var BookmarkingUI = {
>         footer: "panel-subview-footer"
>       },
>       insertionPoint: ".panel-subview-footer"
>     });
>   },
> 
>   _updateRecentBookmarks: function(aHeaderItem, extraCSSClass = "") {
>     const kMaxResults = 5;
>+    let parent = aHeaderItem.parentNode;
> 
>     let options = PlacesUtils.history.getNewQueryOptions();
>     options.excludeQueries = true;
>     options.queryType = options.QUERY_TYPE_BOOKMARKS;
>     options.sortingMode = options.SORT_BY_DATEADDED_DESCENDING;
>     options.maxResults = kMaxResults;
>     let query = PlacesUtils.history.getNewQuery();
> 
>-    while (aHeaderItem.nextSibling &&
>-           aHeaderItem.nextSibling.localName == "menuitem") {
>-      aHeaderItem.nextSibling.remove();
>-    }
>+    for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]")))
>+      item.remove();
> 
>     let onItemCommand = function (aEvent) {
>       let item = aEvent.target;
>       openUILink(item.getAttribute("targetURI"), aEvent);
>       CustomizableUI.hidePanelForNode(item);
>     };
> 
>     let fragment = document.createDocumentFragment();
>@@ -1392,26 +1391,29 @@ var BookmarkingUI = {
>       let title = node.title;
>       let icon = node.icon;
> 
>       let item =
>         document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>                                  "menuitem");
>       item.setAttribute("label", title || uri);
>       item.setAttribute("targetURI", uri);
>+      item.setAttribute("special-recents", true);

Those changes all seem unrelated?

Marco will want to review the PlacesUIUtils.jsm, browserPlacesViews.js and controller.js changes, I don't have a very informed opinion there.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #17)
> Those changes all seem unrelated?

They are! But not obviously so :-) What I mean is that these changes are needed once I did `item._placesNode = node;`, because that opens up a couple of code paths that weren't trodden before.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > Those changes all seem unrelated?
> 
> They are! But not obviously so :-) What I mean is that these changes are
> needed once I did `item._placesNode = node;`, because that opens up a couple
> of code paths that weren't trodden before.

I don't understand. What part of what code path makes those changes necessary?

Comment 20

[Dão Gottwald [:dao]]

Also, if you could do this on top of the patch in bug 1248268 such that I don't have to rebase my older and larger patch, that would be appreciated.

Comment 21

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #20)
> Also, if you could do this on top of the patch in bug 1248268 such that I
> don't have to rebase my older and larger patch, that would be appreciated.

Of course. It's not a race. I was merely quite enthusiastic due to having the idea that I'm starting to grok the Places observer and controller code to the extent that I can make changes comfortably (whilst understanding what it's doing, that is).

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8745403 [details] [diff] [review]
Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

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

(In reply to Dão Gottwald [:dao] from comment #19)

::: browser/base/content/browser-places.js
@@ +1373,5 @@
>      options.maxResults = kMaxResults;
>      let query = PlacesUtils.history.getNewQuery();
>  
> +    for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]")))
> +      item.remove();

Making the removal of the 'old' items dependent on selecting them explicitly through the use of a unique property is handy, because we needn't rely on a menuseparator to be present in various XUL files.
This means we can keep the control flow in this single file.

@@ +1396,5 @@
>                                   "menuitem");
>        item.setAttribute("label", title || uri);
>        item.setAttribute("targetURI", uri);
> +      item.setAttribute("special-recents", true);
> +      item.setAttribute("outside-container", true);

`special-recents` means that these items are part of a special 'Recently Bookmarked' group. Useful for the items-selector above.
`outside-container` means that these items are not part of a (live) places container, so the menu view may know to leave them alone. See PlacesViewBase#_cleanPopup() in this patch.

@@ +1403,5 @@
>        item.addEventListener("command", onItemCommand);
>        if (icon) {
>          item.setAttribute("image", icon);
>        }
> +      item._placesNode = node;

This makes the item 'visible' to the Places controller. In other words: linking the item to the places db node allows for a context menu to be present and for other input control methods, like drag 'n drop.
Normally this would be done by the specific view, but since we're generating the items here custom, we need to take this step as well.

::: browser/components/places/PlacesUIUtils.jsm
@@ +614,5 @@
>      let parentNode = aNode.parent;
> +    if (!parentNode) {
> +      Cu.reportError(new Error("canUserRemove doesn't accept root nodes"));
> +      return false;
> +    }

When the controller selects which commands to disable for the context menu, it'll be rough and throw an error for root nodes here.
Since our recently bookmarked items and respective places nodes are outside their container, they have in fact become root nodes of sorts. But we do want a context menu. Hence I make this part friendlier whilst still disabling the command for root nodes.

::: browser/components/places/content/browserPlacesViews.js
@@ +206,5 @@
>          // In all other cases the insertion point is before that node.
>          container = selectedNode.parent;
> +        // ...except when the node is placed outside its root!
> +        if (!container)
> +          return null;

Since our recently bookmarked items and their respective places nodes are placed outside their container(s), there is no container. (Enter Jedi hand wave gesture.)
Therefore bailing out here is the right thing to do in this case.

@@ +239,5 @@
>      // Remove Places nodes from the popup.
>      let child = aPopup._startMarker;
>      while (child.nextSibling != aPopup._endMarker) {
>        let sibling = child.nextSibling;
> +      let isPlacesNode = (sibling._placesNode && !sibling.hasAttribute("outside-container"));

This code extends the `._placesNode` property detection with the `outside-container` property, because we want to be in full control of the recently bookmarked items and not have the view remove the nodes from the menu!

@@ +676,5 @@
>    containerStateChanged:
>    function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) {
> +    // We may safely ignore root nodes here.
> +    if (!aPlacesNode.parent)
> +      return;

This is all about live bookmarks, so container-less nodes don't have any value here.
In fact, if we let this through, `this.invalidateContainer(...)` will throw an error.

::: browser/components/places/content/controller.js
@@ -455,5 @@
>            nodeData["link"] = true;
>            uri = NetUtil.newURI(node.uri);
>            if (PlacesUtils.nodeIsBookmark(node)) {
>              nodeData["bookmark"] = true;
> -            PlacesUtils.nodeIsTagQuery(node.parent)

Someone forgot to remove this statement during the last refactor here.
Since our nodes are parent-less, this obviously throws.
Removing this line does _not_ have side-effects, because it's also invoked three lines down (line 463 here).

Comment 23

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #19)
> 
> ::: browser/base/content/browser-places.js
> @@ +1373,5 @@
> >      options.maxResults = kMaxResults;
> >      let query = PlacesUtils.history.getNewQuery();
> >  
> > +    for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]")))
> > +      item.remove();
> 
> Making the removal of the 'old' items dependent on selecting them explicitly
> through the use of a unique property is handy, because we needn't rely on a
> menuseparator to be present in various XUL files.
> This means we can keep the control flow in this single file.

The menuseparator is there to stay, so this sounds like a solution looking for a problem. Anyway, this change is just completely unrelated to this bug.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #23)
> The menuseparator is there to stay, so this sounds like a solution looking
> for a problem. Anyway, this change is just completely unrelated to this bug.

Nope, it's not. In fact, the other places view code messes up and draws duplicate items for us if we don't take of this. In other words: remove this code and the patch won't work. :(

Comment 25

[Dão Gottwald [:dao]]

Then I still don't get how exactly this change helps places view code. Does the loop without querySelector remove items it shouldn't remove, or leave back items it should remove? What's going on?

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8745403 [details] [diff] [review]
Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

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

Please rebase on top of bug 1248268 and let's see where we are.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

I left the 'hideRecentlyBookmarked' in there, because it's still handy for the header text context menu.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #25)
> Then I still don't get how exactly this change helps places view code. Does
> the loop without querySelector remove items it shouldn't remove, or leave
> back items it should remove? What's going on?

Can you please explain this?

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Screenshot: while vs. for

(In reply to Dão Gottwald [:dao] from comment #25)
> Then I still don't get how exactly this change helps places view code. Does
> the loop without querySelector remove items it shouldn't remove, or leave
> back items it should remove? What's going on?

I'm afraid I don't have to 100% grasp on the situation, but here's what I've found out so far:
As we're now setting `._placesNode` on the menuitem, they are now considered proper bookmark nodes for `PlacesViewBase` in browserPlacesView.js.
This means that `_rebuildPopup` is called for our recent bookmark items as well, which invokes `_cleanPopup`, which is where we added the guarding code for items with an 'outside-container' attribute set.
But _rebuildPopup does lots more and I believe `_insertNewItemToPopup` is what ultimately causes the situation in the screenshot: it shows the state of the 'Recently Bookmarked' items when you open the popup for the _second_ time - it's the same for the main menu on any platform; complete duplicate items.
Thus the insertion point can't be deduced from the places node correctly anymore, since they live outside their respective containers.

Comment 30

[Dão Gottwald [:dao]]

Are you sure the while loop ran at all? Have you accidentally removed or otherwise broken it? Because it looks like all old items remained and then the new ones were added on top of them.

Comment 31

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #30)
> Are you sure the while loop ran at all? Have you accidentally removed or
> otherwise broken it? Because it looks like all old items remained and then
> the new ones were added on top of them.

I'm _very_ sure. And the small patch is all the code I have applied.

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8747045 [details] [diff] [review]
Patch v3: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

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

Sorry for late, my priorities are in a good shape, so I'm now looking into this, and will follow you until this is merged.

I think your problem with the menu rebuilding is that by adding _placesNode you confused the code that is inserting the _startMarker and _endMarker in the menu.
To better explain this, each places menu has a _startMarker and _endMarker special menuseparators that are hidden and indicate the beginning and the end of the dinamically generated content in the menu. These are used to properly draw drop indicators.
the _startMarker is found by walking the menu until the first node with a _placesNode is found. And this is where your patch fails.

See _ensureMarkers at http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#864
and in particular
897       if (child._placesNode && !firstNonStaticNodeFound) {

I think you want to add an hasAttribute check here and skip your fake nodes. The attribute should be more generically named than special-recents, so we can reuse it, something like "static-places-node" or "simulated-places-node"

This should help reducing the reach of your patch.

Comment 33

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #32)
> Sorry for late, my priorities are in a good shape, so I'm now looking into
> this, and will follow you until this is merged.

I didn't expect any different from you! I know your queue is long nowadays, so I calculated the waiting time in... here's to hoping for better days.

> See _ensureMarkers at
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/browserPlacesViews.js#864
> and in particular
> 897       if (child._placesNode && !firstNonStaticNodeFound) {

Awesome, thanks! I think this info will help finish things here. I was on PTO yesterday and not feeling well today, but next week for sure.

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8750265 [details] [diff] [review]
Patch v4: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

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

::: browser/components/places/PlacesUIUtils.jsm
@@ +611,5 @@
>     * @return true if the aNode represents a removable entry, false otherwise.
>     */
>    canUserRemove: function (aNode) {
>      let parentNode = aNode.parent;
> +    if (!parentNode) {

could you please tell me the stack trace reaching this point? Is it just used by commands updating?
If so, probably we should not reportError, or it would happen at every contextual menu opening.

::: browser/components/places/content/browserPlacesViews.js
@@ +206,5 @@
>          // In all other cases the insertion point is before that node.
>          container = selectedNode.parent;
> +        // ...except when the node is placed outside its root!
> +        if (!container)
> +          return null;

to maintain the current behavior, we should instead end up in the previews branch

198       if (!popup._placesNode || popup._placesNode == this._resultNode ||
199           popup._placesNode.itemId == -1) {
200         // If a static menuitem is selected, or if the root node is selected,
201         // the insertion point is inside the folder, at the end.
202         container = selectedNode;
203         orientation = Ci.nsITreeView.DROP_ON;

Ideally these nodes should not have a _placesNode, thus we would enter this branch. you should add a check for the attribute here and take this path rather than the else.

@@ +239,5 @@
>      // Remove Places nodes from the popup.
>      let child = aPopup._startMarker;
>      while (child.nextSibling != aPopup._endMarker) {
>        let sibling = child.nextSibling;
> +      let isPlacesNode = (sibling._placesNode && !sibling.hasAttribute("simulated-places-node"));

Looks like these changes should not be needed anymore, now that we fixed _startMarker.

This code starts cleaning from the startMarker so it should not even touch our nodes.

@@ +676,5 @@
>    containerStateChanged:
>    function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) {
> +    // We may safely ignore root nodes here.
> +    if (!aPlacesNode.parent)
> +      return;

Could you please tell me the stack trace reaching this point?

Comment 36

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #35)
> ::: browser/components/places/PlacesUIUtils.jsm
> @@ +611,5 @@
> >     * @return true if the aNode represents a removable entry, false otherwise.
> >     */
> >    canUserRemove: function (aNode) {
> >      let parentNode = aNode.parent;
> > +    if (!parentNode) {
> 
> could you please tell me the stack trace reaching this point? Is it just
> used by commands updating?
> If so, probably we should not reportError, or it would happen at every
> contextual menu opening.

TypeError: aNode is null
PU_nodeIsQuery()PlacesUtils.jsm:333
this.PlacesUIUtils.canUserRemove()PlacesUIUtils.jsm:630
_hasRemovableSelection()controller.js:336
PC_isCommandEnabled()controller.js:162
updatePlacesCommand()controller.js:1683
goUpdatePlacesCommands()controller.js:1694
oncommandupdate()browser.xul:1
PVB_buildContextMenu()browserPlacesViews.js:230
onpopupshowing()browser.xul:1

... I've removed the Cu.reportError and added a comment there instead.

> 
> ::: browser/components/places/content/browserPlacesViews.js
> @@ +206,5 @@
> >          // In all other cases the insertion point is before that node.
> >          container = selectedNode.parent;
> > +        // ...except when the node is placed outside its root!
> > +        if (!container)
> > +          return null;

So true! I changed it.

> @@ +676,5 @@
> >    containerStateChanged:
> >    function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) {
> > +    // We may safely ignore root nodes here.
> > +    if (!aPlacesNode.parent)
> > +      return;
> 
> Could you please tell me the stack trace reaching this point?

I'm not hitting this anymore with the changes applied that you requested, so it works just fine now.

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: Patch v5: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

Comment 38

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8750727 [details] [diff] [review]
Patch v5: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

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

I'm forwarding a feedback to dao, cause I think we don't need anymore 2 separate context menus and I hope we could remove the "reduced" one now
Since he added it, maybe I'm missing some context.

::: browser/base/content/browser-places.js
@@ +1483,5 @@
>        fragment.appendChild(item);
>      }
>      root.containerOpen = false;
>      aHeaderItem.parentNode.insertBefore(fragment, aHeaderItem.nextSibling);
>      aHeaderItem.setAttribute("context", "hideRecentlyBookmarked");

this thing still confuses me, the contextual menu on "Recently Bookmarked" header is exotic and different from anything else. I think it should look the same as the menu on any other static item.
Shouldn't we remove the "hideRecentlyBookmarked" menupopup and stop setting it as a context attribute here?
I'm not sure what you decided in this regard, but it looks an over-complication we don't need anymore...

::: browser/components/places/PlacesUIUtils.jsm
@@ +612,5 @@
>     */
>    canUserRemove: function (aNode) {
>      let parentNode = aNode.parent;
> +    if (!parentNode) {
> +      // canUserRemove doesn't accept root nodes.

"root or simulated nodes"

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8750727 [details] [diff] [review]
Patch v5: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

(In reply to Marco Bonardo [::mak] from comment #38)
> I'm forwarding a feedback to dao, cause I think we don't need anymore 2
> separate context menus and I hope we could remove the "reduced" one now
> Since he added it, maybe I'm missing some context.

It depends on whether it's feasible to use the places context menu for the "Recently Bookmarked" header menu item. If so, I'd prefer getting rid of the custom context menu.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think it will just work as it works on Show all bookmarks or any other menuitem there.

Comment 41

[Mike de Boer [:mikedeboer]]

Attachment: Patch v6: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions

With custom menupopup removal. Carrying over r=mak.

Comment 42

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e8b47b007d

Comment 43

[Mike de Boer [:mikedeboer]]

Attachment: Patch 6 ad.: fix unit test to deal with recent bookmarks static items

Comment 44

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b7b2c4ef71

Comment 45

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/ec7fe414e5892563cd7343836431d662222a4068
Bug 1248267 - allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions. r=mak,dao

https://hg.mozilla.org/integration/fx-team/rev/e8112fba80d64c7c6615c9019178e68f60b5d718
Bug 1248267 - fix unit test to deal with recent bookmarks static items. r=dao

Comment 46

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #44)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b7b2c4ef71

Well, Windows seems to be busted in its own spectacular way that I find hard to relate to my patches. Giving it a shot on integration.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec7fe414e589
https://hg.mozilla.org/mozilla-central/rev/e8112fba80d6

Comment 48

[Azmina]

I have reproduced this bug with 47.0a1 (2016-02-14) on Windows 8.1 64 bit!

This bug's fix is verified on Latest Aurora 49.0a2.

Build ID : 20160723004004
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160722]

Comment 49

[Bogdan Maris, Desktop QA]

Thanks Azmina for the testing!
I also verified this using latest DevEdition 49.0a2 on Mac OS X 10.10.5 and Ubuntu 16.04 64-bit. Marking as verified fixed on 49 and also changing the status of the bug to Verified since the Target Milestone has been reached.