Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mossop, Assigned: erikvold)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
When australis finally ships we should deprecate widget and add warnings when it is used.
(Reporter)

Comment 1

4 years ago
We also need to update the docs to note the deprecation.

Updated

4 years ago
Assignee: nobody → evold
Priority: -- → P2
Created attachment 8349128 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1326

Pointer to Github pull-request
Attachment #8349128 - Flags: review?(zer0)
The deprecation process assumes we deprecate a module only when we provide alternatives, so I would wait to land this bug till Toolbar, Frame, and the new Buttons are landed.

Dave, is this bug intended to update the documentation as well, as it seems by your comment, or should be done in a different bug?
Depends on: 787390, 951317
(Reporter)

Comment 4

4 years ago
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> The deprecation process assumes we deprecate a module only when we provide
> alternatives, so I would wait to land this bug till Toolbar, Frame, and the
> new Buttons are landed.

Yep, agree with this, but they should all be landing imminently, right?

> Dave, is this bug intended to update the documentation as well, as it seems
> by your comment, or should be done in a different bug?

It doesn't really matter to me, if you want a second bug for the docs then file one.
They should all be landing these days. However, I will wait to land this patch until we didn't land the rest – that's why I set the two related bugs as blockers.

About the documentation I do not want a second bug, but because the patch doesn't provide any changes on documentation I thought it should be added.
Changed my mind, now that documentation is in MDN we shouldn't block PR for that – 'cause documentation can't be added in the same PR via MD files.
Comment on attachment 8349128 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1326

Now that we have both Button and Toolbar API we can land this bug.
Attachment #8349128 - Flags: review?(zer0) → review+
Created attachment 8358734 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1341/files#diff-4

Pointer to Github pull-request
Attachment #8358734 - Flags: review?(jsantell)
Comment on attachment 8358734 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1341/files#diff-4

Actually I'll give this to Matteo.
Attachment #8358734 - Flags: review?(jsantell) → review?(zer0)
(Reporter)

Updated

4 years ago
Depends on: 958454
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8358734 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1341/files#diff-4

review rejected basically because it seems to me that removes all the code that checks if we currently run Australis or not, and that will broke Holly.
Attachment #8358734 - Flags: review?(zer0) → review-
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #11)
> Comment on attachment 8358734 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/addon-sdk/pull/1341/files#diff-4
> 
> review rejected basically because it seems to me that removes all the code
> that checks if we currently run Australis or not, and that will broke Holly.

As far as I understood our last weekly meeting, we agreed to not land this warning until Australis lands, so does it make sense to change the patch?
Flags: needinfo?(zer0)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #12)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #11)
> > Comment on attachment 8358734 [details]
> > Pointer to Github pull request:
> > https://github.com/mozilla/addon-sdk/pull/1341/files#diff-4
> > 
> > review rejected basically because it seems to me that removes all the code
> > that checks if we currently run Australis or not, and that will broke Holly.
> 
> As far as I understood our last weekly meeting, we agreed to not land this
> warning until Australis lands, so does it make sense to change the patch?

So, during the last meeting we thought Australis was going to land on 30, but they changed their plans and they're trying to land on 29. It means that basically nothing is changed: we still need to support Holly (so, the code that checks if we're in australis or not needs to be present) and we have to land deprecation widget messages.
Flags: needinfo?(zer0)
Comment on attachment 8393013 [details]
https://github.com/mozilla/addon-sdk/pull/1436/files

r+ once the conflict is resolved and all tests are passing
Attachment #8393013 - Flags: review?(zer0) → review+
Comment on attachment 8393013 [details]
https://github.com/mozilla/addon-sdk/pull/1436/files

Hey I've got to get another review, there are updates due to failures.
Attachment #8393013 - Flags: review+ → review?(zer0)
Also the test examples fail, maybe we can make handling those a separate bug?  I suggest we remove them, rewrite them to use ui/stuff, or move them to a separate repo that will use jpm to run those tests (like how mozmill used to work.
Flags: needinfo?(zer0)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> Also the test examples fail, maybe we can make handling those a separate
> bug?

Sounds good to me.

> I suggest we remove them, rewrite them to use ui/stuff, or move them
> to a separate repo that will use jpm to run those tests (like how mozmill
> used to work.

Because are examples, probably is better update them to use new APIs.

Also, it seems to me that most of our examples doesn't have actually any tests (annotator, etc), and the one that have them – and doesn't work because the deprecation message – are really dummy tests. I'm totally in favor to remove them, and fill a bug to update our examples' code, and examples' test.
We could also fill multiple bugs (one per example), they could be good first bug too, and help contributors to get familiarity with new APIs.

However, r+ once you removed the test examples that are failing, and maybe add a comment in the tests file that points to the bug you filed.
Flags: needinfo?(zer0)
Attachment #8393013 - Flags: review?(zer0) → review+
Blocks: 987348

Comment 19

4 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f937e881f5104527e1587c4f9d07ded2b4be590a
Bug 936623 - Deprecate Widget

https://github.com/mozilla/addon-sdk/commit/7b75b2ae4ef68cf6b13f3be1aa95fcd434119f68
Bug 936623 fixing a 280 of 281 failure for test-widget.js due to the added deprecation warning

https://github.com/mozilla/addon-sdk/commit/5298fdb5f58cf81a006d1f4df9b57a45779e9a70
Bug 936623 commenting out the broken example add-on tests (due to widget dep)

https://github.com/mozilla/addon-sdk/commit/313abba9bdd6523c2b716d44b806f53cfb89b980
Merge pull request #1436 from erikvold/936623

Bug 936623 - Deprecate Widget r=@ZER0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.