1238313 - Implement missing browser.tabs events

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Kris Maglione [:kmag]]

These include:

 * onMoved
 * onAttached
 * onDetached
 * onReplaced (possibly)

Comment 1

[Kris Maglione [:kmag]]

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

Comment 2

[Kris Maglione [:kmag]]

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/

Comment 3

[Kris Maglione [:kmag]]

:/

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

https://reviewboard.mozilla.org/r/34635/#review31837

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/c893cd374dcd93adc833e3bcc05e725176bfd523
Bug 1238313: Part 1 - Implement tabs.onMoved event. r=billm

Comment 6

[Kris Maglione [:kmag]]

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

Comment 7

[Kris Maglione [:kmag]]

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.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Kris Maglione [:kmag]]

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

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/dae141b3af6af70909179599e1ad6441a772acfa
Bug 1238313: Part 1 - Implement tabs.onMoved event. r=billm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dae141b3af6a

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 13

[Kris Maglione [:kmag]]

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

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8720414 [details]
MozReview Request: Bug 1238313: Part 2 - Implement tabs.onReplaced event. r?billm

https://reviewboard.mozilla.org/r/35303/#review32345

Comment 16

[Kris Maglione [:kmag]]

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