Closed Bug 854982 Opened 7 years ago Closed 4 years ago

Remove traits from window & tab implementations

Categories

(Add-on SDK Graveyard :: General, defect, P1, major)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

No description provided.
Priority: -- → P1
Assignee: nobody → rFobic
Duplicate of this bug: 854980
As it was impossible to remove trains from either windows or tabs independently I'm merging these bugs into one here.
Summary: Remove traits from windows implementation → Remove traits from window & tab implementations
Attachment #8340793 - Flags: review?(jsantell)
Attachment #8340798 - Flags: review?(evold)
Comment on attachment 8340793 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1313/files

Looks good, a few comments regarding `chainable` and adding additional docs/tests
Attachment #8340793 - Flags: review?(jsantell) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/fcaec2da6848bfcc8d40f0768fa40650a6a23134
Merge pull request #1313 from Gozala/bug/functional@854982

Bug 854982 - Extend functional utils. r=@jsantell
Comment on attachment 8340798 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

I mentioned a few missing tests and also the current test file seems to have at least one failure in it.
Attachment #8340798 - Flags: review?(evold) → review-
Comment on attachment 8344115 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Addressed all comments, requesting new review.
Attachment #8344115 - Flags: review?(evold)
Comment on attachment 8344115 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Looks good!
Attachment #8344115 - Flags: review?(evold) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d3fc4eafa02da4f37979f33fc0c2065956257940
Merge pull request #1314 from Gozala/bug/sequence@854982

Bug 854982  - Implement lazy sequence manipulation library. r=@erikvold
Can I help with this? This is blocking a few things for me, and I'll take it off of your hands, but not sure what the status of it is (as it seems to be in several PRs)
Flags: needinfo?(rFobic)
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13)
> Can I help with this? This is blocking a few things for me, and I'll take it
> off of your hands, but not sure what the status of it is (as it seems to be
> in several PRs)

Only remaining pull request is the containing all the changes. Other pull requests were just offspring's
to land parts of it separately. I can get you up to speed by talking over in person if you'd like.
Flags: needinfo?(rFobic)
What is the status of this work?
Flags: needinfo?(rFobic)
Flags: needinfo?(jsantell)
The status is following:

There is incomplete patch that is probably will require some work additional work and syncing with changes that happened in the meantime here:

https://github.com/mozilla/addon-sdk/pull/1207

It also must be noted that smaller chunks of that has being landed as part of fixes for other bugs. Bug 695143  would be one example that I can remember given that I have landed it today.

It also worth noticing that I believe our tab / window APIs are not going to work with e10s and we would likely have to switch to something more along the lines what chrome API looks like:

https://developer.chrome.com/extensions/tabs

Notice how there you obtain list of active tabs or state of individual tab via async API, that has a reason which is going to be relevant in our case with e10s as well. Given this is a case it may actually make sense to leverage tab actor stuff that devtools has built instead as it's E10S ready and would also reduce amount of code duplications we have to do with devtools team.

Given that I had to switch to a stuff with a higher prioritly I was not able to finish this. Also given the e10s notes above I'm not totally convinced it is worth perusing this task & it could make sense to just define a new API on top of devtools actors and also solve e10s compatibility along with it.
Flags: needinfo?(rFobic)
I can't really provide much information regarding this
Flags: needinfo?(jsantell)
Hey Irakli, I'm not sure if you want to work on this, if not could you please unassign yourself.
Flags: needinfo?(rFobic)
Severity: normal → major
Attached file pull request
This is a large patch so sorry about that, but it isn't as large as the existing pull requests here because it doesn't try to do as much. Details are in the pull request so I won't repeat them here.
Attachment #8584045 - Flags: review?(evold)
Comment on attachment 8584045 [details] [review]
pull request

Looks good!

I mentioned a few small things, it appears there is an issue with one test, and I don't think we should change what `this` is when using `browserWindows.open({ onOpen: ... ` if we can avoid it.
Attachment #8584045 - Flags: review?(evold)
Attachment #8584045 - Flags: review-
Attachment #8584045 - Flags: feedback+
Looks like Mossop is handling this one.
Assignee: rFobic → dtownsend
Flags: needinfo?(rFobic)
Comment on attachment 8584045 [details] [review]
pull request

Updated from the comments and added some more tests for the this property. Notes are in the pull request, not sure what you want to do about the tab.destroy function.
Attachment #8584045 - Flags: review- → review?(evold)
Comment on attachment 8584045 [details] [review]
pull request

Just mentioned why I don't want to remove the test of calling tab.destroy() twice in the pr.
Attachment #8584045 - Flags: review?(evold) → review-
Comment on attachment 8584045 [details] [review]
pull request

I've reimplemented tab.destroy and found a few other bugs in the course of testing for that. One behaviour of tab.destroy hasn't been implemented since it looks buggy to me and would complicate things but see what you think in the PR.
Attachment #8584045 - Flags: review- → review?(evold)
Comment on attachment 8584045 [details] [review]
pull request

After thinking about this more, you were right about the `tab.destroy()` method, I think we can remove that, I was probably over estimating the need to keep it.  Perhaps we can open a bug now to deprecate that just in case some add-on is using it, we can log deprecation warnings for a few releases before removing it.

Also the widget module was removed so there might be a small conflict.

Thanks!
Attachment #8584045 - Flags: review?(evold) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/53d1725c7f31544ae5c700a30d26ff6be9559b4f
Bug 854982: Reimplement tabs and windows APIs without traits. r=erikvold
BOOM!
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Woohoo!!! I'm excited for this! The tab/window APIs have needed some love. So glad you were able to do this! Do you know what version of FF this is slated to be in?
Assuming we get an uplift in in the next couple of weeks it should make Firefox 40.
Depends on: 1163816
You need to log in before you can comment on or make changes to this bug.