Tim Nguyen :ntim Reporter
	Description • 7 years ago

The API introduced in bug 1370499 should work for the library window and the bookmarks sidebar

	Tim Nguyen :ntim Reporter
	Comment 3 • 6 years ago

Thanks for working on this Peter!

	Shane Caraveo (:mixedpuppy)
	Comment 4 • 6 years ago

Sorry for the delay.  I checked in with product to ensure that these sidebars were not going to have any significant changes in the foreseeable future.  mconcoa got me the ok.  I'll get to reviews soon.

	Shane Caraveo (:mixedpuppy)
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8993307 [details]
Bug 1419195: Show items from WebExtensions in Places Library context menu

https://reviewboard.mozilla.org/r/258078/#review268814

::: browser/components/extensions/parent/ext-menus.js:753
(Diff revision 1)
> +      }
> +    }
> +  },
> +
> +  uninit() {
> +    Services.ww.unregisterNotification(this);

Do we also need to remove the popupshowing listener during unregister?

	Shane Caraveo (:mixedpuppy)
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8993306 [details]
Bug 1419195: Show items from WebExtensions in bookmarks sidebar context menu

https://reviewboard.mozilla.org/r/258076/#review268816

::: browser/components/extensions/parent/ext-menus.js:752
(Diff revision 1)
> +    const browser = window.document.getElementById("sidebar");
> +    browser.addEventListener("load", menuTracker.onSidebarLoad,
> +                             {capture: true}); // Load events don't bubble.
> +    if (window.sidebar.document.readyState === "complete") {
> +      menuTracker.onSidebarLoad({currentTarget: browser});
> +    }

I'm not sure of the long term use of window.sidebar, just use use "window.SidebarUI.*" (see browser-sidebar.js)

::: browser/components/extensions/parent/ext-menus.js:765
(Diff revision 1)
> +  cleanupWindow(window) {
> +    for (const id of this.menuIds) {
> +      const menu = window.document.getElementById(id);
> +      menu.removeEventListener("popupshowing", this);
> +    }
> +    const browser = window.document.getElementById("sidebar");

window.SidebarUI.browser

::: browser/components/extensions/parent/ext-menus.js:768
(Diff revision 1)
> +    const URL = window.document.getElementById("viewBookmarksSidebar")
> +                               .getAttribute("sidebarurl");
> +    if (window.sidebar.location.href === URL) {

Just check window.SidebarUI.currentID == "viewBookmarksSidebar".  Ditto in onSidebarLoad.

	Peter Simonyi
	Comment 8 • 6 years ago

Depends on the sidebar patch

	Peter Simonyi
	Comment 9 • 6 years ago

Rebasing was a bit tricky, but I *think* I got everything.

(In reply to Tim Nguyen :ntim from comment #3)
> Thanks for working on this Peter!

Happy to :)

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Do we also need to remove the popupshowing listener during unregister?

Yes, you're right.  I just didn't think about the library window being open during unregister, but of course it could be.

	Shane Caraveo (:mixedpuppy)
	Comment 10 • 6 years ago

Comment on attachment 9006037 [details] [diff] [review]
sidebar.patch

I'm good with this, just going to ask rob to take a second look since he's heavily involved in the context menu code.

	Shane Caraveo (:mixedpuppy)
	Comment 11 • 6 years ago

Comment on attachment 9006038 [details] [diff] [review]
library.patch

I'm good with this, just going to ask rob to take a second look since he's heavily involved in the context menu code.

	Rob Wu [:robwu] Assignee
	Comment 12 • 6 years ago

Comment on attachment 9006037 [details] [diff] [review]
sidebar.patch

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

::: browser/components/extensions/parent/ext-menus.js
@@ +777,5 @@
>        const menu = window.document.getElementById(id);
>        menu.addEventListener("popupshowing", menuTracker);
>      }
> +
> +    const browser = window.SidebarUI.browser;

SidebarUI.browser is a lazy getter, so let's try to avoid touching it if not needed (not just this line, but in your whole patch):
https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/browser-sidebar.js#38-44

Instead of the "load" event on browser, use the SidebarShown event on the sidebar-switcher-target element to detect when the sidebar is shown, and then attach the popupshowing listener to the placesContext if needed.

@@ +806,5 @@
> +    const window = event.currentTarget.ownerGlobal;
> +    if (window.SidebarUI.currentID === "viewBookmarksSidebar") {
> +      const menu = window.SidebarUI.browser.contentDocument
> +                         .getElementById("placesContext");
> +      menu.addEventListener("popupshowing", menuTracker);

Use `menuTracker.handleSidebarCtxMenu` (please rename this, e.g. to handleBookmarksSidebarMenu). Then you don't need the extra check in the handleEvent method below.

	Rob Wu [:robwu] Assignee
	Comment 13 • 6 years ago

Comment on attachment 9006038 [details] [diff] [review]
library.patch

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

::: browser/components/extensions/parent/ext-menus.js
@@ +764,5 @@
> +    Services.ww.registerNotification(this);
> +
> +    // See WindowTrackerBase#*browserWindows in ext-tabs-base.js for why we
> +    // can't use the enumerator's windowtype filter.
> +    let e = Services.wm.getEnumerator("");

Please use a for-of loop. We recently added support for ES6 iterators in: https://bugzilla.mozilla.org/show_bug.cgi?id=1484496

@@ +781,5 @@
> +  // cleanupWindow is called on any library window that's open.
> +  uninit(cleanupWindow) {
> +    Services.ww.unregisterNotification(this);
> +
> +    let e = Services.wm.getEnumerator(this.libraryWindowType);

Also here, please use a for-of loop.

@@ +785,5 @@
> +    let e = Services.wm.getEnumerator(this.libraryWindowType);
> +    while (e.hasMoreElements()) {
> +      let window = e.getNext();
> +      try {
> +        cleanupWindow(window);

You should also unregister the "load" listeners that you've added in init/observe.

@@ +886,5 @@
>    },
>  
> +  onLibraryOpen(window) {
> +    const menu = window.document.getElementById("placesContext");
> +    menu.addEventListener("popupshowing", menuTracker);

Similarly to the other patch: If you pass the event (menuTracker.handlePlacesXXX) directly, then you don't need to have a special case in handleEvent.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +623,5 @@
> +        browser.contextMenus.create({
> +          title: "Get bookmark",
> +          contexts: ["bookmark"],
> +        }, resolve));
> +      browser.test.sendMessage("bookmark-created", newBookmark.id);

Move this after the browser.contextMenus.onClicked listener. Otherwise we will face intermittent test failures.

	Rob Wu [:robwu] Assignee
	Comment 14 • 6 years ago

Peter, have you seen the above review response? Just a couple of changes and then it's good to go.

	Tim Nguyen :ntim Reporter
	Comment 15 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #12)
> Comment on attachment 9006037 [details] [diff] [review]
> sidebar.patch
> 
> Review of attachment 9006037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/extensions/parent/ext-menus.js
> @@ +777,5 @@
> >        const menu = window.document.getElementById(id);
> >        menu.addEventListener("popupshowing", menuTracker);
> >      }
> > +
> > +    const browser = window.SidebarUI.browser;
> 
> SidebarUI.browser is a lazy getter, so let's try to avoid touching it if not
> needed (not just this line, but in your whole patch):
> https://searchfox.org/mozilla-central/rev/
> 5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/browser-
> sidebar.js#38-44
> 
> Instead of the "load" event on browser, use the SidebarShown event on the
> sidebar-switcher-target element to detect when the sidebar is shown, and
> then attach the popupshowing listener to the placesContext if needed.

This might not be needed to address with bug 1494103.

	Peter Simonyi
	Comment 16 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #14)
Thanks, Rob.  I'll have a chance to fix these up (and read bug 1494103) this weekend.  (It was going to be last weekend, but we've just had a major power outage that scrapped all my normal plans.)

	Rob Wu [:robwu] Assignee
	Comment 17 • 6 years ago

Hi Peter,

Firefox 64 will branch off soon (https://wiki.mozilla.org/Release_Management/Calendar), so to get this functionality in Firefox 64, the patches need to land next week.

Do you have time time finish these patches next week?
If not, then I am willing to take over and fix the last bits so they can land in 64.

	Worcester12345
	Comment 18 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #17)
> Hi Peter,
> 
> Firefox 64 will branch off soon
> (https://wiki.mozilla.org/Release_Management/Calendar), so to get this
> functionality in Firefox 64, the patches need to land next week.
> 
> Do you have time time finish these patches next week?
> If not, then I am willing to take over and fix the last bits so they can
> land in 64.
Does anyone know if this happened?

	Rob Wu [:robwu] Assignee
	Comment 19 • 6 years ago

^ That has not happened. This feature can land in 65, at the earliest.

	Tim Nguyen :ntim Reporter
	Comment 20 • 6 years ago

Hi Rob, Is this something you could finish up ?

	Rob Wu [:robwu] Assignee
	Comment 21 • 6 years ago

I'm awaiting Peter's response. In the interest of moving forwards: If there is no response within two weeks, I'll finish the patches.

	Rob Wu [:robwu] Assignee
	Comment 22 • 5 years ago

Nothing is happening here, and the patch slipped yet another release.
I'll take over the bug to ensure that the fixes become part of Firefox 66.

	Rob Wu [:robwu] Assignee
	Comment 24 • 5 years ago

Depends on D16413

	Pulsebot
	Comment 26 • 5 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/ee53bdc5b1d4
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/732184f122e3
Show items from WebExtensions in Places Library context menu r=mixedpuppy

	Pulsebot
	Comment 28 • 5 years ago

Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f97d93c3a2d
Backed out changeset 732184f122e3 for failing in browser_ext_contextMenus.js

	Pulsebot
	Comment 30 • 5 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/d9a81de35ac3
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c04c2376a213
Show items from WebExtensions in Places Library context menu r=mixedpuppy

	Pulsebot
	Comment 34 • 5 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/6b7a9a7afa56
Show items from WebExtensions in bookmarks sidebar context menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/8b35181c3ccc
Show items from WebExtensions in Places Library context menu r=mixedpuppy