Closed Bug 1413692 Opened 7 years ago Closed 7 years ago

Extensions page actions are pinned to address bar again and again

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Version:
58 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: mstanke, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1413692 - Extensions page actions are pinned to address bar again and again.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
I have unpinned all installed extensions page action from address bar to keep them in the page actions menu only. But after some time (and usually after restart) they move to the address bar again, probably after they are first shown in the code.

Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171101104430

The installed add-ons in questions are Awesome RSS 1.1.2 and Pontoon Tools 2.0.2.
Thanks for filing this, I'll take a look.
Assignee: nobody → adw
Status: NEW → ASSIGNED
I think I blew it with the onShutdown change in ext-pageAction.js.  We shouldn't call browserPageAction.remove() when simply quitting the app.
Priority: -- → P1
Whiteboard: [fxsearch]
Is bug 1413712 a dupe of this?
Flags: needinfo?(adw)
This fixes it.  I tested it manually.  Is there any way to write a test for this?  To send a mock webextension an APP_SHUTDOWN I guess.
Flags: needinfo?(adw)
(In reply to :Gijs (no reviews; PTO recovery mode) from comment #3)
> Is bug 1413712 a dupe of this?

Didn't mean to clear the needinfo without answering this, but I did answer in bug 1413712.
(In reply to Drew Willcoxon :adw from comment #5)
> This fixes it.  I tested it manually.  Is there any way to write a test for
> this?  To send a mock webextension an APP_SHUTDOWN I guess.

You can use promiseStartupManager and promiseShutdownManager in an xpcshell test. I'm not so certain however if you can test against CUI from xpcshell. The extension needs to be created with {useAddonManager: "temporary"}, you'll see that in most tests under toolkit/components/extensions/test/xpcshell   test_ext_browserSettings.js is a good example of using these.
Hmm, it may be possible to test PageActions.jsm outside of the browser, at least for this specific bug.  It might require it to be reworked a little.  I'll try.  (PageActions isn't related to CUI (unfortunately!).)  I should also take a closer look at how CUI is handling this...  Does it just never purge the placement of widgets from extensions?
This includes a test.  I found I had to use {useAddonManager: "permanent"} to get it to work.
Let's see if this fixes the test failures... https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32166c34192
Yes, try looks good now.
Comment on attachment 8924647 [details]
Bug 1413692 - Extensions page actions are pinned to address bar again and again.

https://reviewboard.mozilla.org/r/195882/#review201156

::: browser/components/extensions/test/xpcshell/test_ext_pageAction_shutdown.js:48
(Diff revision 4)
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +
> +  // Get the PageAction object and set its shownInUrlbar to false.  It's set to
> +  // true in ext-pageAction.js, when it's created.

Can we test across restart for both values (true and false)?

::: browser/modules/PageActions.jsm:394
(Diff revision 4)
>      } catch (ex) {
>        Cu.reportError(ex);
>      }
>    },
>  
> +  // For tests.

// For tests.  See Bug 1413692.
Attachment #8924647 - Flags: review?(mixedpuppy) → review+
Thanks Shane, this addresses your comments.  One more try push and then I'll land this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7e1b8930ef5
Comment on attachment 8924647 [details]
Bug 1413692 - Extensions page actions are pinned to address bar again and again.

https://reviewboard.mozilla.org/r/195882/#review202082

::: browser/components/extensions/test/xpcshell/test_ext_pageAction_shutdown.js:48
(Diff revision 5)
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +
> +  // Get the PageAction.Action object.  Its shownInUrlbar should have been
> +  // initialized to true in ext-pageAction.js, when it's created.

If we fix bug 1413574 will this still be true?  Typically we would run browser.pageAction.show|hide in the addon, with the default that it is not shown until show() is called.
(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> If we fix bug 1413574 will this still be true?  Typically we would run
> browser.pageAction.show|hide in the addon, with the default that it is not
> shown until show() is called.

Bug 1413574 renames shownInUrlbar to pinnedInUrlbar, and it maps the webextension show()/hide() methods to the PageActions disabled state.  Pinning is controlled by the user, and the disabled state is controlled by the extension.  An action is shown in the urlbar only if it's both pinned and not disabled.  All webextension actions start off pinned but disabled, so it's up to each extension to call show() as appropriate.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71a2c8b33ae0
Extensions page actions are pinned to address bar again and again. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/71a2c8b33ae0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
The patch works for me in Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171108110838. Unpinned icons stay in the menu and pinned ones in the address bar too.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: