1473720 - menus.onShown fails when context is a bookmark

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect)

Description

[Peter Simonyi]

Attachment: Testcase

STR: Install the attached testcase extension, which just sets a menus.onShown listener that prints to the console.

Right-click on tabs or in the content area; the extension prints to the console as expected.

Right-click on a bookmark (in the bookmarks menu or toolbar).  The extension code doesn't get to run; instead an error is printed to the console.

Comment 1

[Peter Simonyi]

Attachment: Bug 1473720: Fix firing menus.onShown for bookmark items

Review commit: https://reviewboard.mozilla.org/r/255116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255116/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

https://reviewboard.mozilla.org/r/255116/#review261980


Code analysis found 18 defects in this patch:
 - 18 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_menus.js:161
(Diff revision 1)
>    BrowserTestUtils.removeTab(tab);
>    await extension.unload();
>  });
>  
> +add_task(async function test_bookmarkContextMenu() {
> +  async function showBookmarksToolbar(visible=true) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/components/extensions/test/browser/browser_ext_menus.js:162
(Diff revision 1)
>    await extension.unload();
>  });
>  
> +add_task(async function test_bookmarkContextMenu() {
> +  async function showBookmarksToolbar(visible=true) {
> +    let bt = document.getElementById('PersonalToolbar');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:164
(Diff revision 1)
>  
> +add_task(async function test_bookmarkContextMenu() {
> +  async function showBookmarksToolbar(visible=true) {
> +    let bt = document.getElementById('PersonalToolbar');
> +    let transitionPromise =
> +      BrowserTestUtils.waitForEvent(bt, 'transitionend',

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:165
(Diff revision 1)
> +add_task(async function test_bookmarkContextMenu() {
> +  async function showBookmarksToolbar(visible=true) {
> +    let bt = document.getElementById('PersonalToolbar');
> +    let transitionPromise =
> +      BrowserTestUtils.waitForEvent(bt, 'transitionend',
> +                                    e => e.propertyName == 'max-height');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:172
(Diff revision 1)
> +    await transitionPromise;
> +  }
> +
> +  const ext = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ['menus', 'bookmarks'],

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:172
(Diff revision 1)
> +    await transitionPromise;
> +  }
> +
> +  const ext = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ['menus', 'bookmarks'],

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:175
(Diff revision 1)
> +  const ext = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ['menus', 'bookmarks'],
> +    },
> +    async background() {
> +      await browser.menus.create({title: 'blarg', contexts: ['bookmark']});

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:175
(Diff revision 1)
> +  const ext = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: ['menus', 'bookmarks'],
> +    },
> +    async background() {
> +      await browser.menus.create({title: 'blarg', contexts: ['bookmark']});

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:177
(Diff revision 1)
> +      permissions: ['menus', 'bookmarks'],
> +    },
> +    async background() {
> +      await browser.menus.create({title: 'blarg', contexts: ['bookmark']});
> +      browser.menus.onShown.addListener(() => {
> +        browser.test.sendMessage('hello');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:179
(Diff revision 1)
> +    async background() {
> +      await browser.menus.create({title: 'blarg', contexts: ['bookmark']});
> +      browser.menus.onShown.addListener(() => {
> +        browser.test.sendMessage('hello');
> +      });
> +      browser.test.sendMessage('ready');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:185
(Diff revision 1)
> +    },
> +  });
> +
> +  await showBookmarksToolbar();
> +  await ext.startup();
> +  await ext.awaitMessage('ready');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:187
(Diff revision 1)
> +
> +  await showBookmarksToolbar();
> +  await ext.startup();
> +  await ext.awaitMessage('ready');
> +
> +  let menu = await openChromeContextMenu('placesContext',

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:188
(Diff revision 1)
> +  await showBookmarksToolbar();
> +  await ext.startup();
> +  await ext.awaitMessage('ready');
> +
> +  let menu = await openChromeContextMenu('placesContext',
> +      '#PlacesToolbarItems .bookmark-item');

Error: Expected indentation of 41 spaces but found 6. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_menus.js:188
(Diff revision 1)
> +  await showBookmarksToolbar();
> +  await ext.startup();
> +  await ext.awaitMessage('ready');
> +
> +  let menu = await openChromeContextMenu('placesContext',
> +      '#PlacesToolbarItems .bookmark-item');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:191
(Diff revision 1)
> +
> +  let menu = await openChromeContextMenu('placesContext',
> +      '#PlacesToolbarItems .bookmark-item');
> +  let children = Array.from(menu.children);
> +  let item = children[children.length - 1];
> +  is(item.label, 'blarg', 'Menu item label is correct');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:191
(Diff revision 1)
> +
> +  let menu = await openChromeContextMenu('placesContext',
> +      '#PlacesToolbarItems .bookmark-item');
> +  let children = Array.from(menu.children);
> +  let item = children[children.length - 1];
> +  is(item.label, 'blarg', 'Menu item label is correct');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:192
(Diff revision 1)
> +  let menu = await openChromeContextMenu('placesContext',
> +      '#PlacesToolbarItems .bookmark-item');
> +  let children = Array.from(menu.children);
> +  let item = children[children.length - 1];
> +  is(item.label, 'blarg', 'Menu item label is correct');
> +  await ext.awaitMessage('hello'); // onShown listener fired

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/extensions/test/browser/browser_ext_menus.js:194
(Diff revision 1)
> +  let children = Array.from(menu.children);
> +  let item = children[children.length - 1];
> +  is(item.label, 'blarg', 'Menu item label is correct');
> +  await ext.awaitMessage('hello'); // onShown listener fired
> +
> +  closeChromeContextMenu('placesContext', item);

Error: Strings must use doublequote. [eslint: quotes]

Comment 3

[Peter Simonyi]

Whoops, meant to fix my quotes but forgot.
I'm not sure if the "infix operators" lint is intentional though, since a default argument is not really an operator... leaving as is because that's how I'm used to seeing default arguments and I don't see any nearby to follow local style.  Please let me know if spaces are really preferred.

Comment 4

[Peter Simonyi]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255116/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

https://reviewboard.mozilla.org/r/255116/#review262004


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_menus.js:188
(Diff revision 2)
> +  await showBookmarksToolbar();
> +  await ext.startup();
> +  await ext.awaitMessage("ready");
> +
> +  let menu = await openChromeContextMenu("placesContext",
> +      "#PlacesToolbarItems .bookmark-item");

Error: Expected indentation of 41 spaces but found 6. [eslint: indent]

Comment 6

[Peter Simonyi]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255116/diff/2-3/

Comment 7

[Peter Simonyi]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

I didn't mean to cancel the review request.  Not sure what happened.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

https://reviewboard.mozilla.org/r/255116/#review262180

nice.  I kicked off a try run, land given it passes.  As this is a simple fix for broken behavior, I think it's a good candidate for uplift.

Comment 9

[Peter Simonyi]

Try looks good, thanks.  I don't have permission to land it myself ("You must have scm_level_3 access to land").

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

Need approval before landing.

Comment 11

[Dorel Luca [:dluca]]

Needs proper mozreview approval before it can be landed

Comment 12

[Peter Simonyi]

Should be ready to go now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4561d6ed48b6
Fix firing menus.onShown for bookmark items r=mixedpuppy

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4561d6ed48b6

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

Approval Request Comment
[Feature/Bug causing the regression]: context menu on bookmarks
[User impact if declined]: some extension functionality may not work
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: very simple change, covered by tests
[String changes made/needed]: none

Comment 16

[Liz Henry (:lizzard)]

Comment on attachment 8990116 [details]
Bug 1473720: Fix firing menus.onShown for bookmark items

Has good automated test coverage, adds a new test, let's uplift for beta 10.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/fff218cba849