Implement `pageshow` event for Fennec tabs

RESOLVED FIXED

Status

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

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Created attachment 780075 [details]
GH PR 1132
Attachment #780075 - Flags: review?(evold)
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.
Attachment #780075 - Flags: feedback?(rFobic)
Comment on attachment 780075 [details]
GH PR 1132

Looks good to me.
Attachment #780075 - Flags: feedback?(rFobic) → feedback+
(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.
(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`.
Flags: needinfo?(rFobic)
(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..
Both `tab` and `persisted` are the arguments passed into the Firefox version of `pageshow` FYI
(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 on attachment 780075 [details]
GH PR 1132

We shouldn't open new browser windows on Fennec, it causes crashes.
Attachment #780075 - Flags: review?(evold) → review-
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
(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..
(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.
Flags: needinfo?(rFobic)
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?
Attachment #780075 - Flags: review- → review?(evold)

Updated

5 years ago
Priority: -- → P2
OS: Mac OS X → Android
Hardware: x86 → All
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.
Attachment #780075 - Flags: review?(evold) → review-
Comment on attachment 780075 [details]
GH PR 1132

Removed windows from new pageshow test!
Attachment #780075 - Flags: review- → review?(evold)
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.
Attachment #780075 - Flags: review?(evold) → review-
Implementing `onPageShow` is already in bug 896311
(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.
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.
(Assignee)

Updated

5 years ago
Attachment #780075 - Flags: review- → review?(evold)
Comment on attachment 780075 [details]
GH PR 1132

Awesome!
Attachment #780075 - Flags: review?(evold) → review+

Comment 21

5 years ago
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

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.