854982 - Remove traits from window & tab implementations

Status: RESOLVED FIXED

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

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

As it was impossible to remove trains from either windows or tabs independently I'm merging these bugs into one here.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1207

Pointer to Github pull-request

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1313/files

Pointer to Github pull-request

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Pointer to Github pull-request

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 7

[[github robot]]

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 8

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Pointer to Github pull-request

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8344115 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Addressed all comments, requesting new review.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8344115 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1314

Looks good!

Comment 12

[[github robot]]

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

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

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)

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 15

[Dave Townsend [:mossop]]

What is the status of this work?

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

I can't really provide much information regarding this

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, I'm not sure if you want to work on this, if not could you please unassign yourself.

Comment 19

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

Looks like Mossop is handling this one.

Comment 22

[Dave Townsend [:mossop]]

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.

Comment 23

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 24

[Dave Townsend [:mossop]]

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.

Comment 25

[Erik Vold [:erikvold] (please needinfo? me)]

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!

Comment 26

[[github robot]]

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

Comment 27

[Dave Townsend [:mossop]]

BOOM!

Comment 28

[Ben McCann]

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?

Comment 29

[Dave Townsend [:mossop]]

Assuming we get an uplift in in the next couple of weeks it should make Firefox 40.