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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

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.

Comment 1

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

Attachment: 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/

Comment 2

[Kris Maglione [:kmag]]

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"

Comment 3

[Luca Greco [:rpl] [:luca] [: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

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

Comment 4

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

Attachment: 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/

Comment 5

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

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).

Comment 6

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

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

> Maybe call this `PersistentBackgroundPage`?

opted for "PersistentBackgroundProperty" instead, because it sounds more like the new "UnrecognizedProperty".

Comment 7

[Kris Maglione [:kmag]]

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`.

Comment 8

[Kris Maglione [:kmag]]

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 9

[Luca Greco [:rpl] [:luca] [: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

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

Comment 10

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

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/

Comment 11

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

Attachment: 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/

Comment 12

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

Attachment: 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/

Comment 13

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

Attachment: 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/

Comment 14

[Kris Maglione [:kmag]]

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

Comment 15

[Kris Maglione [:kmag]]

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

Comment 16

[Kris Maglione [:kmag]]

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?

Comment 17

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

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 18

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

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/

Comment 19

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

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!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/398160abc223
https://hg.mozilla.org/integration/fx-team/rev/c02c1dc34180
https://hg.mozilla.org/integration/fx-team/rev/1e47cefe3b9b
https://hg.mozilla.org/integration/fx-team/rev/8a78c738b882
https://hg.mozilla.org/integration/fx-team/rev/d50f024f55d8

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 22

[Luca Greco [:rpl] [:luca] [: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 23

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

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 24

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

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/

Comment 25

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

(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)

Comment 26

[Carsten Book [:Tomcat]]

has conflicts when applying to fx-team, could you take a look, thanks!

Comment 27

[Luca Greco [:rpl] [:luca] [: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

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

Comment 28

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

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 29

[Luca Greco [:rpl] [:luca] [: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/2-3/

Comment 30

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

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 31

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

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/

Comment 32

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

patches rebased to an updated fx-team team (with the conflict resolved).

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9891272b3f09
https://hg.mozilla.org/integration/fx-team/rev/46794fe72b80
https://hg.mozilla.org/integration/fx-team/rev/96c02999683f
https://hg.mozilla.org/integration/fx-team/rev/229b9d4e1396
https://hg.mozilla.org/integration/fx-team/rev/c715f7a51827

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9891272b3f09
https://hg.mozilla.org/mozilla-central/rev/46794fe72b80
https://hg.mozilla.org/mozilla-central/rev/96c02999683f
https://hg.mozilla.org/mozilla-central/rev/229b9d4e1396
https://hg.mozilla.org/mozilla-central/rev/c715f7a51827