Closed Bug 1404746 Opened 3 years ago Closed 3 years ago

Ignore minimum_opera_version in WebExtensions manifest

Categories

(WebExtensions :: Compatibility, defect, P5)

defect

Tracking

(firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox58 --- verified

People

(Reporter: cr0ydon, Assigned: apoorvasingh2811, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

I want to support Firefox and Opera in my WebExtension, therefore I have added minimum_opera_version to the manifest file to prevent breakages in older Opera version I don't support.


Actual results:

Firefox is throwing an error, complaining about the minimum_opera_version key in the manifest file.

> addons.webextension.<unknown>	WARN	Loading extension 'null': Reading manifest: Error processing minimum_opera_version: An unexpected property was found in the WebExtension manifest.


Expected results:

Firefox should just ignore the minimum_opera_version key in the manifest file.
Component: Untriaged → WebExtensions: Compatibility
Product: Firefox → Toolkit
Mentor: bob.silverberg
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P5
Hi, I'm a beginner. Can I take this up?
Flags: needinfo?(bob.silverberg)
(In reply to apoorvasingh2811 from comment #1)
> Hi, I'm a beginner. Can I take this up?

Sure, thanks for offering to take this on.

To fix this bug you'll need to add an entry to the manifest schema, for an optional string property called "minimum_opera_version", which will be very much like the existing property for "minimum_chrome_version" at:

http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/toolkit/components/extensions/schemas/manifest.json#17

You'll need to add a test to verify that this optional property is now ignored. It can be very much like the test we currently have at:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_manifest_minimum_chrome_version.js

except that you'll need to specify the "minimum_opera_version" property in the test instead of the "minimum_chrome_version" property.

Finally, you'll need to add a line for your new test to the xpcshell.ini file at:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/xpcshell.ini

Please let me know if you have any questions.
Flags: needinfo?(bob.silverberg)
Comment on attachment 8914505 [details] [diff] [review]
Added the entry to the manifest schema, added the test, and made the required change in xpcshell.ini

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

Looks great, thanks for the patch! I'm giving it an r+ and will pass it onto a peer for a final review.
Attachment #8914505 - Flags: review?(mixedpuppy)
Attachment #8914505 - Flags: review?(bob.silverberg)
Attachment #8914505 - Flags: review+
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Comment on attachment 8914505 [details] [diff] [review]
> Added the entry to the manifest schema, added the test, and made the
> required change in xpcshell.ini
> 
> Review of attachment 8914505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, thanks for the patch! I'm giving it an r+ and will pass it onto
> a peer for a final review.

Thank you so much for your guidance!
Since this is my contribution, I haven't been vouched by anyone yet. If you don't mind, can you please vouch for me!?
(In reply to apoorvasingh2811 from comment #5)
> (In reply to Bob Silverberg [:bsilverberg] from comment #4)
> > Comment on attachment 8914505 [details] [diff] [review]
> > Added the entry to the manifest schema, added the test, and made the
> > required change in xpcshell.ini
> > 
> > Review of attachment 8914505 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks great, thanks for the patch! I'm giving it an r+ and will pass it onto
> > a peer for a final review.
> 
> Thank you so much for your guidance!
> Since this is my contribution, I haven't been vouched by anyone yet. If you
> don't mind, can you please vouch for me!?

my first contribution*
(In reply to apoorvasingh2811 from comment #5)
> 
> Thank you so much for your guidance!
> Since this is my contribution, I haven't been vouched by anyone yet. If you
> don't mind, can you please vouch for me!?

Certainly. Vouch for you where?
(In reply to Bob Silverberg [:bsilverberg] from comment #7)
> (In reply to apoorvasingh2811 from comment #5)
> > 
> > Thank you so much for your guidance!
> > Since this is my contribution, I haven't been vouched by anyone yet. If you
> > don't mind, can you please vouch for me!?
> 
> Certainly. Vouch for you where?

Here : https://mozillians.org/en-US/u/apoorvasingh17/
Thanks!
(In reply to apoorvasingh2811 from comment #8)
> 
> Here : https://mozillians.org/en-US/u/apoorvasingh17/
> Thanks!

Done. Thanks for your contribution.
Attachment #8914505 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → apoorvasingh2811
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b0cdd852e9
Ignore minimum_opera_version in WebExtensions manifest. r=bsilverberg, r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9b0cdd852e9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Verified as fixed in FF58.0b6 in Win 7 64 bit, MacOS 10.13.1
I tried loading the extension as a temporary add-on and installing it as a submitted add-on from the AMO site. Firefox is now ignoring minimum_opera_version from the manifest file.
Marking bug as verified fixed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.