Ignore the "minimum_chrome_version" manifest property

VERIFIED FIXED in Firefox 51

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: mattw, Assigned: jshubheksha, Mentored, NeedInfo)

Tracking

({good-first-bug})

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: triaged)

Attachments

(3 attachments, 1 obsolete attachment)

This property should just be ignored and perhaps a warning could be displayed to notify the user that it has been ignored.
Whiteboard: [webNavigation] triaged → triaged
A warning is already displayed.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
We shouldn't display a warning, we should just ignore the property.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Handle the "minimum_chrome_version" manifest property → Ignore the "minimum_chrome_version" manifest property
Whiteboard: triaged → triaged[good first bug]
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?
Flags: needinfo?(kmaglione+bmo)
Summary: Ignore the "minimum_chrome_version" manifest property → Ignore unrecognized chrome manifest properties
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.
Flags: needinfo?(kmaglione+bmo)
Summary: Ignore unrecognized chrome manifest properties → Ignore the "minimum_chrome_version" manifest property
At the moment if yoou are using linter it flags up the update_url as a being in the manifest but not being used.
(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 :)
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?
(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
> 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
Mentor: kmaglione+bmo
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
Flags: needinfo?(kmaglione+bmo)
(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
Flags: needinfo?(kmaglione+bmo)
Keywords: good-first-bug
Whiteboard: triaged[good first bug] → triaged
I'd like to work on this bug. Can I be assigned to it?
(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.
Assignee: nobody → jshubheksha
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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
Flags: needinfo?(kmaglione+bmo)
Attachment #8784688 - Flags: review?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
Attachment #8784688 - Flags: review?(kmaglione+bmo)
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.
Attachment #8784686 - Flags: review?(kmaglione+bmo)
Attachment #8784973 - Flags: review?(kmaglione+bmo) → review?(dtownsend)
Comment on attachment 8784973 [details]
Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property.

Kris should finish the review here.
Attachment #8784973 - Flags: review?(dtownsend) → review?(kmaglione+bmo)
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).
Attachment #8784973 - Flags: review?(kmaglione+bmo) → review+
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 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)
Attachment #8786058 - Flags: review?(kmaglione+bmo)
Flags: needinfo?(jshubheksha)
Attachment #8786058 - Attachment is obsolete: true
Attachment #8786058 - Flags: review?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe53c63024cb
Accept and ignore the minimum_chrome_version manifest property. r=kmag
https://hg.mozilla.org/mozilla-central/rev/fe53c63024cb
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.