897254 - Implement `pageshow` event for Fennec tabs

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: GH PR 1132

Comment 2

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

Comment on attachment 780075 [details]
GH PR 1132

Module owner should approve this second `persisted` param in `onPageShow(tab, persisted)`  I'd rather see an event ovject passed in that the tab, since the tab is the `this` anyhow and it could be available in `event.target` or something.

Comment 3

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

Comment on attachment 780075 [details]
GH PR 1132

Looks good to me.

Comment 4

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> Comment on attachment 780075 [details]
> GH PR 1132
> 
> Module owner should approve this second `persisted` param in
> `onPageShow(tab, persisted)`  I'd rather see an event ovject passed in that
> the tab, since the tab is the `this` anyhow and it could be available in
> `event.target` or something.

APIs intentionally didn't expose unwrapped representations & I would avoid changing at least as a part of this, note that firefox tabs do pass wrapped tab instances.

While `this` is indeed passed in, it's not going to work for example with arrow functions on which `this` can't be implied.

Comment 5

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

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> > Comment on attachment 780075 [details]
> > GH PR 1132
> > 
> > Module owner should approve this second `persisted` param in
> > `onPageShow(tab, persisted)`  I'd rather see an event ovject passed in that
> > the tab, since the tab is the `this` anyhow and it could be available in
> > `event.target` or something.
> 
> APIs intentionally didn't expose unwrapped representations & I would avoid
> changing at least as a part of this, note that firefox tabs do pass wrapped
> tab instances.

I don't know what you are referring to when you say `unwrapped representation`.

Comment 6

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #4)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> > > Comment on attachment 780075 [details]
> > > GH PR 1132
> > > 
> > > Module owner should approve this second `persisted` param in
> > > `onPageShow(tab, persisted)`  I'd rather see an event ovject passed in that
> > > the tab, since the tab is the `this` anyhow and it could be available in
> > > `event.target` or something.
> > 
> > APIs intentionally didn't expose unwrapped representations & I would avoid
> > changing at least as a part of this, note that firefox tabs do pass wrapped
> > tab instances.
> 
> I don't know what you are referring to when you say `unwrapped
> representation`.

I'm hoping you didn't think that I was suggesting we expose the unwrapped tab..

Comment 7

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

Both `tab` and `persisted` are the arguments passed into the Firefox version of `pageshow` FYI

Comment 8

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> Both `tab` and `persisted` are the arguments passed into the Firefox version
> of `pageshow` FYI

Ah I see I didn't realize this was added to Firefox already..

Comment 9

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

Comment on attachment 780075 [details]
GH PR 1132

We shouldn't open new browser windows on Fennec, it causes crashes.

Comment 10

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

Fennec doesn't open new browser windows:
https://github.com/mozilla/addon-sdk/pull/1132/files#L2R63

Probably a more generic name for this would be better, like setup or something. Right now most of our tests live in Fennec or Firefox and they don't overlap, which when we can, we should have them overlap

Comment 11

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #10)
> Fennec doesn't open new browser windows:
> https://github.com/mozilla/addon-sdk/pull/1132/files#L2R63
> 
> Probably a more generic name for this would be better, like setup or
> something. Right now most of our tests live in Fennec or Firefox and they
> don't overlap, which when we can, we should have them overlap

I'm fine with overlapping tests when we can, I don't think the tests should open browser windows when they don't need to though, which would make the overlapping much easier..

Comment 12

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #4)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> > > Comment on attachment 780075 [details]
> > > GH PR 1132
> > > 
> > > Module owner should approve this second `persisted` param in
> > > `onPageShow(tab, persisted)`  I'd rather see an event ovject passed in that
> > > the tab, since the tab is the `this` anyhow and it could be available in
> > > `event.target` or something.
> > 
> > APIs intentionally didn't expose unwrapped representations & I would avoid
> > changing at least as a part of this, note that firefox tabs do pass wrapped
> > tab instances.
> 
> I don't know what you are referring to when you say `unwrapped
> representation`.

Probably I misunderstand what you were suggesting, I thought you were suggesting to pass `event.target` which is XUL tab or equivalent in Fennec. To that I object as we we intentionally did not expose them in high level APIs and only way to obtain those is by requiring a low level API. I don't mind reconsidering that either, it's just if we do we should have a broader discussion and it definitely should not be part of this change.


Either way at this point only reasonable thing is to make API consistent with desktop FF, which I believe what this patch does.

Comment 13

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

Comment on attachment 780075 [details]
GH PR 1132

Refactoring the tabs tests to not open new windows when unnecessary will probably need to be done for more Fennec tests -- that can be handled as I get there (and I already am doing that in pieces). Either way, are we good for this feature?

Comment 14

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

Comment on attachment 780075 [details]
GH PR 1132

We shouldn't use `openBrowserWindow` even on Firefox for this test, so there is no need to move it.

Comment 15

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

Comment on attachment 780075 [details]
GH PR 1132

Removed windows from new pageshow test!

Comment 16

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

Comment on attachment 780075 [details]
GH PR 1132

There should be a test that the pageshow event happens in the expected order relative to the open and ready events too.  We also need a test that `tabs.open({url: .., onPageshow: function() {...}})` works too.

Comment 17

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

Implementing `onPageShow` is already in bug 896311

Comment 18

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> Implementing `onPageShow` is already in bug 896311

I think we ought to do these together, it's very odd having one without the other, but I understand we're already there, so I won't block on it.

Comment 19

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

Changed the assertations to assertEqual, and the ordering of open/ready/pageshow events. Once this is good, wrapping up onPageShow in the declaration will be cake.

Comment 20

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

Comment on attachment 780075 [details]
GH PR 1132

Awesome!

Comment 21

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/dddf08bea89b19cc3d95101c67e99e1d76561e85
Bug 897254 Add pageshow event to fennec tabs

https://github.com/mozilla/addon-sdk/commit/c6d7162fd0326a5c86d3beee8d106f15390c5da8
Merge pull request #1132 from jsantell/897254-pageshow-tab-event-fennec

Fix Bug 897254 Implement 'pageshow' event for tabs for fennec, r=@erikvold