Run event pages as background pages and log warning on persistent manifest attribute

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(5 attachments)

Event Pages [1] are basically Background pages which are not persistent (they can be closed at anytime and restarted when an event subscribed by the event page is available).

To help porting of the existing Chrome add-ons with the minimum "rewriting/tweaking" possible, it would be helpful if we relax the validation on the "persistent" manifest attribute (used to turn a background page into an event page) to just warn the add-on developer that event pages are not yet supported on Firefox and they will run like regular (persistent) background pages, instead of refusing to load the add-on.
Created attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

Review commit: https://reviewboard.mozilla.org/r/38189/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38189/
Attachment #8726680 - Flags: review?(kmaglione+bmo)

Updated

3 years ago
Assignee: nobody → lgreco
Comment on attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

https://reviewboard.mozilla.org/r/38189/#review34699

This looks good. I'd r+ it now, but I've been thinking about making extra properties warnings rather than errors in most manifest objects, rather than just at the top level and in specific cases, and I think it would be good to handle that along with this.

Can you add another type for "UnrecognizedProperty" and add `"additionalProperties": { "$ref": "UnrecognizedProperty" }` where it makes sense?

Thanks.

::: toolkit/components/extensions/schemas/manifest.json:270
(Diff revision 1)
> +        "id": "BackgroundPagePersistentPropertyUnsupported",

Maybe call this `PersistentBackgroundPage`?

::: toolkit/components/extensions/schemas/manifest.json:272
(Diff revision 1)
> +        "deprecated": "Event Pages are not currently supported. This will run as a persistent Background Page."

No need to capitalize "Background Page"
Attachment #8726680 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38189/diff/1-2/
Attachment #8726680 - Flags: review?(kmaglione+bmo)
Created attachment 8729120 [details]
MozReview Request: Bug 1253565 - [webext] reusable 'UnrecognizedProperty' type for manifest schema validation. r?kmag

Review commit: https://reviewboard.mozilla.org/r/39253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39253/
Attachment #8729120 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/39253/#review35949

::: toolkit/components/extensions/schemas/manifest.json:202
(Diff revision 1)
> +        "additionalProperties": { "$ref": "UnrecognizedProperty" },

Besides the background page, it seems that ContentScript is the only one that it could potentially contain any unrecognized property (e.g. the currently unsupported include_globs and exclude_globs).

Let me know if I'm wrong and we want to remove the UnrecognizedProperty from the ContentScript or if there is any other property that could make use of UnrecognizedProperty that I didn't notice.

NOTE: I added a test for the UnrecognizedProperty but currently it is checked only on the background page (if we decide to keep the UnsupportedProperty in the content script I will the remaining test cases).
https://reviewboard.mozilla.org/r/38189/#review34699

> Maybe call this `PersistentBackgroundPage`?

opted for "PersistentBackgroundProperty" instead, because it sounds more like the new "UnrecognizedProperty".
Comment on attachment 8729120 [details]
MozReview Request: Bug 1253565 - [webext] reusable 'UnrecognizedProperty' type for manifest schema validation. r?kmag

https://reviewboard.mozilla.org/r/39253/#review36049

We also need this in `browser_action.json`, `page_action.json`, and `commands.json`.
Attachment #8729120 - Flags: review?(kmaglione+bmo) → review+

Updated

3 years ago
Attachment #8726680 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

https://reviewboard.mozilla.org/r/38189/#review36051
Comment on attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38189/diff/2-3/
Comment on attachment 8729120 [details]
MozReview Request: Bug 1253565 - [webext] reusable 'UnrecognizedProperty' type for manifest schema validation. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39253/diff/1-2/
Created attachment 8730148 [details]
MozReview Request: Bug 1253565 - [webext] test "UnrecognizedProperty" on content_scripts manifest properties. r?kmag

Review commit: https://reviewboard.mozilla.org/r/39703/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39703/
Attachment #8730148 - Flags: review?(kmaglione+bmo)
Created attachment 8730149 [details]
MozReview Request: Bug 1253565 - [webext] use 'UnrecognizedProperty' on browser and page actions. r?kmag

Review commit: https://reviewboard.mozilla.org/r/39705/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39705/
Attachment #8730149 - Flags: review?(kmaglione+bmo)
Created attachment 8730150 [details]
MozReview Request: Bug 1253565 - [webext] add "UnrecognizedProperty" to commands manifest properties. r?kmag

Review commit: https://reviewboard.mozilla.org/r/39707/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39707/
Attachment #8730150 - Flags: review?(kmaglione+bmo)
Comment on attachment 8730150 [details]
MozReview Request: Bug 1253565 - [webext] add "UnrecognizedProperty" to commands manifest properties. r?kmag

https://reviewboard.mozilla.org/r/39707/#review36335
Attachment #8730150 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730148 [details]
MozReview Request: Bug 1253565 - [webext] test "UnrecognizedProperty" on content_scripts manifest properties. r?kmag

https://reviewboard.mozilla.org/r/39703/#review36333
Attachment #8730148 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730149 [details]
MozReview Request: Bug 1253565 - [webext] use 'UnrecognizedProperty' on browser and page actions. r?kmag

https://reviewboard.mozilla.org/r/39705/#review36331

::: browser/components/extensions/test/browser/browser_ext_pageAction_simple.js:1
(Diff revision 1)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

Can you use `hg cp browser_ext_browserAction_simple.js  browser_ext_pageAction_simple.js` for this?
Attachment #8730149 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730149 [details]
MozReview Request: Bug 1253565 - [webext] use 'UnrecognizedProperty' on browser and page actions. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39705/diff/1-2/
Comment on attachment 8730150 [details]
MozReview Request: Bug 1253565 - [webext] add "UnrecognizedProperty" to commands manifest properties. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39707/diff/1-2/
https://reviewboard.mozilla.org/r/39705/#review36331

> Can you use `hg cp browser_ext_browserAction_simple.js  browser_ext_pageAction_simple.js` for this?

I've used histedit to rework it to use hg cp before adapting the test for the pageAction and the diff now makes match more sense, Thanks!
Status: NEW → ASSIGNED
Keywords: checkin-needed
I had to back these out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7994579&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/21077a8abccf
Flags: needinfo?(lgreco)
Comment on attachment 8730148 [details]
MozReview Request: Bug 1253565 - [webext] test "UnrecognizedProperty" on content_scripts manifest properties. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39703/diff/1-2/
Comment on attachment 8730149 [details]
MozReview Request: Bug 1253565 - [webext] use 'UnrecognizedProperty' on browser and page actions. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39705/diff/2-3/
Comment on attachment 8730150 [details]
MozReview Request: Bug 1253565 - [webext] add "UnrecognizedProperty" to commands manifest properties. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39707/diff/2-3/
(In reply to Wes Kocher (:KWierso) from comment #21)
> I had to back these out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=7994579&repo=fx-team
> 
> https://hg.mozilla.org/integration/fx-team/rev/21077a8abccf

I rewritten an additional test case for the UnrecognizedProperty warning on content scripts as a mochitest-chrome, because I'm using the monitorConsole test helper to check the logged warning and it doesn't work from a content process (and it is the reason because the previous version of the test cases fails with e10s enabled)
Flags: needinfo?(lgreco)
Keywords: checkin-needed
has conflicts when applying to fx-team, could you take a look, thanks!
Flags: needinfo?(lgreco)
Keywords: checkin-needed
Comment on attachment 8726680 [details]
MozReview Request: Bug 1253565 - [webext] Run event pages as background pages and log warning on persistent manifest attribute. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38189/diff/3-4/
Comment on attachment 8729120 [details]
MozReview Request: Bug 1253565 - [webext] reusable 'UnrecognizedProperty' type for manifest schema validation. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39253/diff/2-3/
Comment on attachment 8730148 [details]
MozReview Request: Bug 1253565 - [webext] test "UnrecognizedProperty" on content_scripts manifest properties. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39703/diff/2-3/
Comment on attachment 8730149 [details]
MozReview Request: Bug 1253565 - [webext] use 'UnrecognizedProperty' on browser and page actions. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39705/diff/3-4/
Comment on attachment 8730150 [details]
MozReview Request: Bug 1253565 - [webext] add "UnrecognizedProperty" to commands manifest properties. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39707/diff/3-4/
patches rebased to an updated fx-team team (with the conflict resolved).
Flags: needinfo?(lgreco)
Keywords: checkin-needed

Updated

2 years ago
Duplicate of this bug: 1279130

Updated

3 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.