1282978 - Ignore the "minimum_chrome_version" manifest property

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Matthew Wein [:mattw]]

This property should just be ignored and perhaps a warning could be displayed to notify the user that it has been ignored.

Comment 1

[Matthew Wein [:mattw]]

A warning is already displayed.

Comment 2

[Kris Maglione [:kmag]]

We shouldn't display a warning, we should just ignore the property.

Comment 3

[Matthew Wein [:mattw]]

There are a number of chrome manifest properties, including update_url, author, minimum_chrome_version, options_page, optional_permissions, and storage, which generate warning in the browser console.

I'm re-purposing this bug to determine which properties we should ignore.

Kris, should we be ignoring all unrecognized properties or are there certain ones we should still be generating warnings for?

Comment 4

[Kris Maglione [:kmag]]

We already ignore unrecognized properties. Recognized but unsupported properties are a different matter.

"options_page", "optional_permissions", and "storage" should be errors. "author", we should just support. "update_url" and "minimum_chrome_version" can be silently ignored.

Comment 5

[drhen123]

At the moment if yoou are using linter it flags up the update_url as a being in the manifest but not being used.

Comment 6

[Andy McKay]

(In reply to drhen123 from comment #5)
> At the moment if yoou are using linter it flags up the update_url as a being
> in the manifest but not being used.

Yeah we thought it would be useful to tell you that while you've set this field, Firefox is going to ignore it. Trying to be helpful and all that :)

Comment 7

[drhen123]

It is useful yes. I agree Firefox should ignore it. If Firefox is going to ignore this and others should they all flag in linter to alert developers?

Comment 8

[Andy McKay]

(In reply to drhen123 from comment #7)
> It is useful yes. I agree Firefox should ignore it. If Firefox is going to
> ignore this and others should they all flag in linter to alert developers?

Maybe, I would save it for things that are important. If you want to file more bugs on the linter, then do so here: https://github.com/mozilla/addons-linter

Comment 9

[drhen123]

> Maybe, I would save it for things that are important. If you want to file
> more bugs on the linter, then do so here:
> https://github.com/mozilla/addons-linter

Fair enough, thanks

Comment 10

[Claas Augner [:claas]]

Could you outline what needs to be done to fix this bug? I could not find any occurrence of "minimum_chrome_version" in mozilla-central [0].

[0] https://dxr.mozilla.org/mozilla-central/search?q=%22minimum_chrome_version%22&redirect=false

Comment 11

[Kris Maglione [:kmag]]

(In reply to Claas Augner [:claas] from comment #10)
> Could you outline what needs to be done to fix this bug? I could not find
> any occurrence of "minimum_chrome_version" in mozilla-central [0].

Sorry for the delay. We're rushing to get things done in time for the next release at the moment.

All we need to do is add an entry to the manifest schema, for an optional string property called "minimum_chrome_version":

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json#9

And then an xpcshell test file like this one:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_incognito.js
http://searchfox.org/mozilla-central/rev/cb74fc1327028e13bb1f66817da07ca09e4edcec/toolkit/components/extensions/test/xpcshell/xpcshell.ini#14

to check that it's accepted without warnings or errors (see [1]).

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Comment 12

[Shubheksha Jalan]

I'd like to work on this bug. Can I be assigned to it?

Comment 13

[Kris Maglione [:kmag]]

(In reply to sj from comment #12)
> I'd like to work on this bug. Can I be assigned to it?

Sure. Let me know if you have any questions.

Comment 14

[Shubheksha Jalan]

(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to sj from comment #12)
> > I'd like to work on this bug. Can I be assigned to it?
> 
> Sure. Let me know if you have any questions.

I made the required changes in manifest.json as well as xpcshell.ini
However, I am unsure regarding the test cases as it's completely new to me.
The test file linked has a enum property which is being tested, however the "minimum_chrome_version" property just has just type and optional fields. I had a couple of questions relating to this:
1) What'll go here in the test file for this property: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_incognito.js#7
2) What's the expected behavior?
3) Do we just need to check the normalized.errors & normalized.errors variables as I don't see any tests for the optional or the type fields in any of the test files.

Comment 15

[Kris Maglione [:kmag]]

(In reply to sj from comment #14)
> I made the required changes in manifest.json as well as xpcshell.ini
> However, I am unsure regarding the test cases as it's completely new to me.
> The test file linked has a enum property which is being tested, however the
> "minimum_chrome_version" property just has just type and optional fields. I
> had a couple of questions relating to this:
>
> 1) What'll go here in the test file for this property:
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/xpcshell/test_ext_manifest_incognito.js#7

Anything you want, as long as it's a string. Any non-string value should still trigger an error.

> 2) What's the expected behavior?

Just that we don't get any warnings or errors.

> 3) Do we just need to check the normalized.errors & normalized.errors
> variables as I don't see any tests for the optional or the type fields in
> any of the test files.

Yup, just `normalized.error` and `normalized.errors`. The normalized value
itself doesn't matter, since we don't actually use it.

Comment 16

[Shubheksha Jalan]

Attachment: Made required changes to manifest.json and xpcshell.ini

Comment 17

[Shubheksha Jalan]

Attachment: test_ext_manifest_minimum_chrome_version.js

I added the required test case in a new file. It passes right now. But if I include a string like "test" over here[1], the test fails. I thought it's expecting a key-value pair, but even that doesn't seem to work. Am I missing something here?

[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_incognito.js#7

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8784688 [details]
test_ext_manifest_minimum_chrome_version.js

(In reply to sj from comment #17)
> I added the required test case in a new file. It passes right now. But if I
> include a string like "test" over here[1], the test fails. I thought it's
> expecting a key-value pair, but even that doesn't seem to work. Am I missing
> something here?

You should have something like this to test for a pass:

  let normalized = yield ExtensionTestUtils.normalizeManifest({
    "minimum_chrome_version": "test",
  });

And something like this to test for a failure:

  let normalized = yield ExtensionTestUtils.normalizeManifest({
    "minimum_chrome_version": 42,
  });

This looks like a good start, but please roll it into the other patch.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8784686 [details] [diff] [review]
Made required changes to manifest.json and xpcshell.ini

Review of attachment 8784686 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, aside from the missing test file. It needs a relevant commit summary, though, in this format:

Bug 1282978 - Accept and ignore the "minimum_chrome_version" manifest property.

Comment 20

[Shubheksha Jalan]

Attachment: Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

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

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

Kris should finish the review here.

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

https://reviewboard.mozilla.org/r/74306/#review72914

r=me with the formatting fixed.

::: toolkit/components/extensions/test/xpcshell/test_ext_manifest_minimum_chrome_version.js:8
(Diff revision 1)
> +"use strict";
> +
> +
> +add_task(function* test_manifest_minimum_chrome_version() {
> +  let normalized = yield ExtensionTestUtils.normalizeManifest({
> +  	"minimum_chrome_version":"42",

Nit: Please expand tabs, use 2-space indentation, and add a space after the ":" (ESLint won't accept this formatting).

Comment 23

[Shubheksha Jalan]

Attachment: Bug 1282978 - Fixed tabs and spacing issues

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

Comment 24

[Shubheksha Jalan]

Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

https://reviewboard.mozilla.org/r/74306/#review72914

> Nit: Please expand tabs, use 2-space indentation, and add a space after the ":" (ESLint won't accept this formatting).

Really sorry about this! I forgot to run eslint before committing. It should be fixed now!

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

https://reviewboard.mozilla.org/r/74306/#review72914

> Really sorry about this! I forgot to run eslint before committing. It should be fixed now!

Can you please fold these into a single commit and push again? (The `histedit` extension can help you with this)

Comment 26

[Shubheksha Jalan]

Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe53c63024cb
Accept and ignore the minimum_chrome_version manifest property. r=kmag

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this initial issue on Firefox 50.0a2 (2016-09-12) under Windows 10 64-bit.

Verified fixed on Firefox 51.0a1 (2016-09-12) under Windows 10 64-bit. The “Error processing minimum_chrome_version: An unexpected property was found in the WebExtension manifest.” warning is no longer thrown in Browser Console.

Comment 30

[Amy Tsay [:amyt]]

Thanks! Contribution added here: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#September_2016