1289527 - Don't expose the PageAction namespace when the page_action manifest property is missing.

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Matthew Wein [:mattw]]

Attachment: bookmark_it_-1.0.xpi

Steps to reproduce:
Load the attached extension and navigate to a new tab.

Actual results:
The PageAction does not show up.

Expected results:
The PageAction should show up.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Matthew Wein [:mattw] from comment #0)
> Created attachment 8774818 [details]
> bookmark_it_-1.0.xpi

When the attached xpi is loaded, there is an error in the Browser Console that complains about the "pageAction" property in the manifest.json (which should be "page_action", https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/page_action)

Changing the property to "page_action" seems to be enought to make the attached xpi to work as expected.

Comment 2

[Kris Maglione [:kmag]]

Yeah, we really need to not expose the API when there's no page_action manifest property. I've been meaning to fix that.

Comment 3

[Matthew Wein [:mattw]]

Thanks Luca! I'm glad it was an issue with the manifest file and not with the API itself.

Comment 4

[Matthew Wein [:mattw]]

I think we can do better than hiding the namespace, although we should do this first.  

Ideally, whenever browser.pageAction is used without page_action existing in the manifest, we should add a console warning with something like "Did you forget to include 'page_action' in the manifest?" with a link to the documentation.

Comment 5

[Kris Maglione [:kmag]]

I think that hiding the API is the right solution. That's what we do for other APIs that the extension doesn't have access to.

Comment 6

[:shell escalante]

is chrome incompatibility we should fix - but not major blocker

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1289527: Hide browserAction and pageAction namespaces when relevant manifest entries are missing.

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8778607 [details]
Bug 1289527: Hide browserAction and pageAction namespaces when relevant manifest entries are missing.

https://reviewboard.mozilla.org/r/69852/#review67392

::: toolkit/components/extensions/test/xpcshell/xpcshell.ini:35
(Diff revision 1)
>  [test_ext_idle.js]
>  [test_ext_json_parser.js]
>  [test_ext_localStorage.js]
>  [test_ext_manifest_content_security_policy.js]
>  [test_ext_manifest_incognito.js]
> +[test_ext_manifest_permissions.js]

This looks like perhaps obsolete?

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8778607 [details]
Bug 1289527: Hide browserAction and pageAction namespaces when relevant manifest entries are missing.

https://reviewboard.mozilla.org/r/69852/#review67392

> This looks like perhaps obsolete?

How so?

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8778607 [details]
Bug 1289527: Hide browserAction and pageAction namespaces when relevant manifest entries are missing.

https://reviewboard.mozilla.org/r/69852/#review67392

> How so?

Oh. I see. Yeah, good catch.

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e18de5ad18969306334ed1f094ccb410b265b1
Bug 1289527: Hide browserAction and pageAction namespaces when relevant manifest entries are missing. r=aswan

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0e18de5ad18