Implement missing browser.tabs events

RESOLVED FIXED in mozilla47

Status

P2
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla47
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tabs] triaged)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
These include:

 * onMoved
 * onAttached
 * onDetached
 * onReplaced (possibly)

Updated

3 years ago
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [tabs] → [tabs] triaged

Updated

3 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Updated

3 years ago
Depends on: 1234086
(Assignee)

Comment 1

3 years ago
Created attachment 8718585 [details]
MozReview Request: Bug 1238313: Part 1 - Implement tabs.onMoved event. r?billm

Review commit: https://reviewboard.mozilla.org/r/34635/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34635/
Attachment #8718585 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

3 years ago
Comment on attachment 8718585 [details]
MozReview Request: Bug 1238313: Part 1 - Implement tabs.onMoved event. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34635/diff/1-2/
Attachment #8718585 - Attachment description: MozReview Request: Bug 1238313: Part 2 - Implement tabs.onReplaced event. r?billm → MozReview Request: Bug 1238313: Part 1 - Implement tabs.onMoved event. r?billm
(Assignee)

Comment 3

3 years ago
:/
Attachment #8718585 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8720414 [details]
MozReview Request: Bug 1238313: Part 2 - Implement tabs.onReplaced event. r?billm

Review commit: https://reviewboard.mozilla.org/r/35303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35303/
Attachment #8720414 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

3 years ago
Resubmitting the Part 2 that mozreview ate the first time. Turns out that you can create multiple patch groups per bug if you know the right push flags.
Keywords: leave-open
I had to back this out because either it or one of the other patches pushed together broke some tests: https://treeherder.mozilla.org/logviewer.html#?job_id=7363470&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/d318430a9a2d
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 9

3 years ago
Hrm. I saw that in one test on try, but it went away when I re-ran.

The problem seems to be that if we trigger a hover event on the new tab page, it throws an error. I guess this test just keeps the new tab page open long enough for it to happen more often. :/
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #6)
> Created attachment 8720414 [details]
> MozReview Request: Bug 1238313: Part 2 - Implement tabs.onReplaced event.
> r?billm
> 
> Review commit: https://reviewboard.mozilla.org/r/35303/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/35303/

Why would an add-on care about this event? I can understand if there are visible effects that would confuse the add-on. But I can't think of any. The <browser> and <tab> elements stay the same, so the tabId should remain the same. I think we'll treat a remoteness change as a normal navigation in all the events we fire.

The patch looks fine otherwise. If there's a reason to do it, I'm all for it.
(Assignee)

Comment 13

3 years ago
(In reply to Bill McCloskey (:billm) from comment #12)
> Why would an add-on care about this event? I can understand if there are
> visible effects that would confuse the add-on. But I can't think of any. The
> <browser> and <tab> elements stay the same, so the tabId should remain the
> same. I think we'll treat a remoteness change as a normal navigation in all
> the events we fire.
> 
> The patch looks fine otherwise. If there's a reason to do it, I'm all for it.

For the moment, I think the only difference an add-on will see is that the history of the tab gets wiped out, which is probably reason enough to do it. When we implement process IDs in the webnavigation/webrequest APIs, that will change too, which will probably matter more.
(In reply to Kris Maglione [:kmag] from comment #13)
> For the moment, I think the only difference an add-on will see is that the
> history of the tab gets wiped out, which is probably reason enough to do it.

That's not supposed to happen. Do you have an STR?

> When we implement process IDs in the webnavigation/webrequest APIs, that
> will change too, which will probably matter more.

I'm worried this will break some extensions that expect onReplaced to happen in a certain way, but we can try it.
Comment on attachment 8720414 [details]
MozReview Request: Bug 1238313: Part 2 - Implement tabs.onReplaced event. r?billm

https://reviewboard.mozilla.org/r/35303/#review32345
Attachment #8720414 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Bill McCloskey (:billm) from comment #14)
> (In reply to Kris Maglione [:kmag] from comment #13)
> > For the moment, I think the only difference an add-on will see is that the
> > history of the tab gets wiped out, which is probably reason enough to do it.
>
> That's not supposed to happen. Do you have an STR?

Hm. I can't actually reproduce this now, but I've definitely run into it
recently, including when I was testing this. I'll look into it some more.

> > When we implement process IDs in the webnavigation/webrequest APIs, that
> > will change too, which will probably matter more.
>
> I'm worried this will break some extensions that expect onReplaced to happen
> in a certain way, but we can try it.

I think I'm going to hold off on this until I have a good use case. We're
going to wind up sending a lot more than Chrome does because of how we handle
the new tab page, so most extensions probably won't be expecting it.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Target Milestone: --- → mozilla47

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.