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

A warning is already displayed.

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

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?

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.

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

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

(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

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.

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

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

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 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 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 on attachment 8784973 [details] Bug 1282978 - Accept and ignore the minimum_chrome_version manifest property. Kris should finish the review here.

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

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

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.

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