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

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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)
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)
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
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).
(Assignee)

Comment 6

3 years ago
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+
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
(Assignee)

Comment 9

3 years ago
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/
(Assignee)

Comment 10

3 years ago
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/
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Comment 12

3 years ago
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)
(Assignee)

Comment 13

3 years ago
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+
(Assignee)

Comment 17

3 years ago
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/
(Assignee)

Comment 18

3 years ago
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/
(Assignee)

Comment 19

3 years ago
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!
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Comment 22

3 years ago
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/
(Assignee)

Comment 23

3 years ago
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/
(Assignee)

Comment 24

3 years ago
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/
(Assignee)

Comment 25

3 years ago
(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)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
has conflicts when applying to fx-team, could you take a look, thanks!
Flags: needinfo?(lgreco)
Keywords: checkin-needed
(Assignee)

Comment 27

3 years ago
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/
(Assignee)

Comment 28

3 years ago
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/
(Assignee)

Comment 29

3 years ago
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/
(Assignee)

Comment 30

3 years ago
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/
(Assignee)

Comment 31

3 years ago
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/
(Assignee)

Comment 32

3 years ago
patches rebased to an updated fx-team team (with the conflict resolved).
Flags: needinfo?(lgreco)
Keywords: checkin-needed

Updated

3 years ago
Duplicate of this bug: 1279130

Updated

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