765683 - livemarkChild metaData never set for place context menus

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[martin]

The Places controller builds metaDatas, related to the current selected places nodes, when displaying the context menu.
One metaData is "livemarkChild". But this one is never setted, I suppose since the "Load Livemarks on demand" plan (bug 613588).

in https://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#475

The test to determine if the node is a livemark child, is inside another test, 
"if (PlacesUtils.nodeIsBookmark(node))".

The cause is :
- PlacesUtils.nodeIsBookmark(node) rejects a node with an itemId = -1
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#165

- the itemId of a livemark child is always -1
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsLivemarkService.js#962

Steps to reproduce:

In short, add a new menuitem element in the placesContext menupopup, displayed only for livemark children, and try the context menu.

detailled:
- open scratchpad, and select the Browser option in the Environment menu (so, the pref devtools.chrome.enabled must be enabled)
- copy/past the following code inside scratchpad, and run it

function createNewMenuitem() {
    let menuitem = document.createElement("menuitem");
    menuitem.setAttribute("label", "I'm a Livemark Child :)");
    menuitem.setAttribute("selection", "livemarkChild");
    
    let menupopup = document.getElementById("placesContext");
    if (menupopup)
        menupopup.appendChild(menuitem);
}

createNewMenuitem();

- now try the context menu (right-click) on a livemark child, inside the Personal Toolbar or the Bookmarks Menu.
A menuitem labelled "I'm a Livemark Child :)" should appears.

actual result:
the created menuitem isn't displayed

expected result:
the created menuitem should be displayed

a patch will follow

Comment 1

[martin]

Attachment: proposed patch

Comment 2

[martin]

Note that the livemarkChild metaData is currently used only in https://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/placesOverlay.xul , for 2 menuitems, and as a livemark child isn't considered as a bookmark for now, another possibility is to remove the usage of this metaData.
But, this solution will annoyed me (for an extension) ;) . Nevertheless, I can find a solution for my extension in this case.

Comment 3

[Marco Bonardo [:mak]]

hm, yeah this is clearly a valid concern.
Well, the patch should be updated cause I think in the meanwhile we changed that code http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#475

I see that we only use livemarkChild to hide context menu options, though those options are already hidden by the fact those are not bookmarks (both cut and info check itemId), so yeah, your supposition in comment 2 is correct, we can get rid of "livemarkChild" completely.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 633985 [details] [diff] [review]
proposed patch

clearing review both cause should be updated, and cause we should probably get rid of the "livemarkChild" stuff.

Comment 5

[Mark Banner (:standard8)]

This was for a legacy style add-on and the code here has changed significantly now. So I think we don't need to do anything here unless an issue is found in the current FF implementation.