843901 - observer notification for tab events

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

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

There are various tab related events dispatched by tabbrowser 
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml
on the tab instances. We should dispatch observer notifications for all of those with subject being a tab instance or unique identifier for it.

Comment 1

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

Should this be a platform bug?

Comment 2

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

Yes

Comment 3

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

Attachment: bug-843901.patch

This patch exposes tab events via observer notifications so add-on SDK (or generally add-ons) don't have to chase individual windows to set up listeners
and remove them.

Comment 4

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

Here is a try build with this changes: https://tbpl.mozilla.org/?tree=Try&rev=e2c655c09e79

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 832337 [details] [diff] [review]
bug-843901.patch

This will need super-review. Note that this could also be implemented as part of the SDK. If we want this as part of tabbrowser (which I'm not quite convinced of), we should refactor the code such that emitting a tab event also sends a notification automatically.

>-          var uniqueId = this._generateUniquePanelID();
>-          this.mPanelContainer.childNodes[0].id = uniqueId;
>-          this.mCurrentTab.linkedPanel = uniqueId;
>+          var uniqueId = this._generateUniqueID();
>+          var uniquePanelId = "panel-" + uniqueId;
>+          this.mPanelContainer.childNodes[0].id = uniquePanelId;
>+          this.mCurrentTab.linkedPanel = uniquePanelId;
>+          this.mCurrentTab.id = "tab-" + uniqueId;

What does this stuff have to do with this bug?

Comment 6

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

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 832337 [details] [diff] [review]
> bug-843901.patch
> 
> This will need super-review. 

Can you tell me who'm should I ask to do that ?


> Note that this could also be implemented as
> part of the SDK.

That is a case but that also means each add-on does same routine of adding bunch of listeners
and removing them every time, not to mention timing issues that come into play since add-ons
can be loaded at different moments in time.


> If we want this as part of tabbrowser (which I'm not quite
> convinced of), we should refactor the code such that emitting a tab event
> also sends a notification automatically.
>

I can refactor patch to do that, if that's preferable.

> 
> >-          var uniqueId = this._generateUniquePanelID();
> >-          this.mPanelContainer.childNodes[0].id = uniqueId;
> >-          this.mCurrentTab.linkedPanel = uniqueId;
> >+          var uniqueId = this._generateUniqueID();
> >+          var uniquePanelId = "panel-" + uniqueId;
> >+          this.mPanelContainer.childNodes[0].id = uniquePanelId;
> >+          this.mCurrentTab.linkedPanel = uniquePanelId;
> >+          this.mCurrentTab.id = "tab-" + uniqueId;
> 
> What does this stuff have to do with this bug?

Do you want me to submit that as a separate patch ? SDK uses `tab.linkedPanel` to
identify tab's which just feels wrong. Also on fennec we have `tab.id` having same
API is really helpful.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Comment on attachment 832337 [details] [diff] [review]
> > bug-843901.patch
> > 
> > This will need super-review. 
> 
> Can you tell me who'm should I ask to do that ?

:gavin is probably a good fit for this.

> > Note that this could also be implemented as
> > part of the SDK.
> 
> That is a case but that also means each add-on does same routine of adding
> bunch of listeners
> and removing them every time, not to mention timing issues that come into
> play since add-ons
> can be loaded at different moments in time.

I meant sending global notifications as part of the SDK that SDK-based add-ons use.

> > >-          var uniqueId = this._generateUniquePanelID();
> > >-          this.mPanelContainer.childNodes[0].id = uniqueId;
> > >-          this.mCurrentTab.linkedPanel = uniqueId;
> > >+          var uniqueId = this._generateUniqueID();
> > >+          var uniquePanelId = "panel-" + uniqueId;
> > >+          this.mPanelContainer.childNodes[0].id = uniquePanelId;
> > >+          this.mCurrentTab.linkedPanel = uniquePanelId;
> > >+          this.mCurrentTab.id = "tab-" + uniqueId;
> > 
> > What does this stuff have to do with this bug?
> 
> Do you want me to submit that as a separate patch ? SDK uses
> `tab.linkedPanel` to
> identify tab's which just feels wrong. Also on fennec we have `tab.id`
> having same
> API is really helpful.

The SDK (!= SDK-based add-ons) accessing tab.linkedPanel seems ok to me. In any case, this seems like fodder for a separate bug.

Comment 8

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

Achieving same without modifications to tabbrowser.xml would look more or less like this:
https://gist.github.com/Gozala/7476658

As you can see there is a much larger surface for bugs to occur, and a lot more overhead. That's
why I prefer to modify tabbrowser.xml instead.

Comment 9

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

Gavin I have made gist with how this can be achieved without modifying tabbrowser.xml itself. Could you please take a look and let me know what you think.

If you think approach of aggregating events rather than dispatching it from source is a way to go, maybe
you could also suggest what's the best place for putting such code ? I can only think of:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js

I assume that it loads at startup and only once per firefox session.

Comment 10

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

Attachment: bug-843901-2.patch

Comment 11

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

Gavin, I believe this patch addresses comments from Dao. I'm having hard time to run commit it to try right
now as moz-git-tools no longer seem to work for me. Once I manage to push to try I'll attach link here.

I'm going to submit a separate patch for tab.id.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8338782 [details] [diff] [review]
bug-843901-2.patch

There's again some unnecessary scope creep in this patch. Why are you making TabAttrModified a MutationEvent?

I don't think we should have a separate _notifyObservers method, as that encourages that the events and notifications get out of sync. Related to this, I don't think the TabStateChange notification makes much sense, and I'm not sure I see the point of the TabInitialOpen notification compared to e.g. delayed-startup-finished.

Comment 13

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

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8338782 [details] [diff] [review]
> bug-843901-2.patch
> 
> There's again some unnecessary scope creep in this patch. Why are you making
> TabAttrModified a MutationEvent?
>

Ok let me explain why are we doing this again:


Intent is to expose same observer notifications across different platforms (Desktop & Fennec)
so that SDK code can hook into and keep it's window & tab model representations synchronized.
Another intent is to make this interface consistent between these platforms so state changes
can be looked up in a same way. For that exact reason I'm going to submit second patch to
expose `tab.id` to match IMO Fennec's more intuitive API.

There for we need some way to expose state changes in a consistent manner. TabOpen / TabClose, Tab..
are all ok as subject.id will have info about model we need to create / dispose / ...

We also need notifications to update model.icon, model.status etc.. that's where `TabStateChange`
events come into play. We need to know what has changed which is attribute name and ideally what it
has changed to since Fennec and Desktop expose those values in quite different way. This is why I 
chose MutationEvent's as they exactly can represent what has state changes have being made. Alternative
could be to have different observer notification topics to encode that like in initial patch:

tab-busy, tab-unbusy, ....

Although that was not ideal since we'll have to deal platform specific state lookups!

Now given what we intend to achieve if you have better suggestions, I'm happy to discuss them.


> 
> I don't think we should have a separate _notifyObservers method, as that
> encourages that the events and notifications get out of sync. 

I don't see how they get out of sync ? They are dispatched by the same code as you requested.
The reason for observer notifications is so that we don't have to chase individual windows and
register / unregistered listeners on them. That approach has being prone to error and what we
really need is a single message but that add-on can hook on / off at startup / shutdown. That
eliminates most of the timing issues & memory leaks issues. Not to mention that it also helps
abstract platform differences since on Fennec there's only one window.


> Related to
> this, I don't think the TabStateChange notification makes much sense,

I hope with comments above this makes more sense.

> and
> I'm not sure I see the point of the TabInitialOpen notification compared to
> e.g. delayed-startup-finished.


Well use of `browser-delayed-startup-finished` does not gives us a tab for the very least,
which means we need to get it ourselfs from window, again different per platform. And even
then what if that window has multiple tabs etc.. etc... I'm sure there maybe a way to get
it all working, but that would miss the point. We just need to know every Tab opened and
doesn't really matter if it was initially there or not.

The less dependency we would be have on implementation details of things the better all of
us would be as we would be free to change those implementations without affecting each other.

BTW only reason I didn't just called it `TabOpen` is because tabbrowser seemed to treat it
specially otherwise it's no different for us from regular `TabOpen`.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #13)
> We also need notifications to update model.icon, model.status etc.. that's
> where `TabStateChange`
> events come into play.

Why is there a TabStateChange notification that is differently named and may have different semantics than the TabAttributeModified event?

> We need to know what has changed which is attribute
> name and ideally what it
> has changed to since Fennec and Desktop expose those values in quite
> different way.

Using CustomEvent gives you the ability to set event.detail to the attribute name, you don't need a MutationEvent for that. What the attribute has changed to you can get by calling getAttribute. Fennec may give you that directly, but that's somewhat irrelevant as long as the SDK can abstract away such differences (this is after all the primary point of the add-on SDK).

> > I don't think we should have a separate _notifyObservers method, as that
> > encourages that the events and notifications get out of sync. 
> 
> I don't see how they get out of sync ? They are dispatched by the same code
> as you requested.

They're already out of sync in your patch (see TabAttributeModified vs. TabStateChange and the odd TabInitialOpen notification), so you must be misunderstanding my concern.

> > and
> > I'm not sure I see the point of the TabInitialOpen notification compared to
> > e.g. delayed-startup-finished.
> 
> 
> Well use of `browser-delayed-startup-finished` does not gives us a tab for
> the very least,
> which means we need to get it ourselfs from window, again different per
> platform. And even
> then what if that window has multiple tabs etc.. etc... I'm sure there maybe
> a way to get
> it all working, but that would miss the point. We just need to know every
> Tab opened and
> doesn't really matter if it was initially there or not.

If you don't care whether the initial tab is really the initial tab, that makes it even simpler to do some manual legwork in the SDK in response to browser-delayed-startup-finished.

> The less dependency we would be have on implementation details of things the
> better all of
> us would be as we would be free to change those implementations without
> affecting each other.

TabInitialOpen actually exposes you to another implementation detail, which is the construction time of the tabbrowser binding. The binding being constructed too early has been a recurring pain point in the past.

Comment 15

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

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #13)
> > We also need notifications to update model.icon, model.status etc.. that's
> > where `TabStateChange`
> > events come into play.
> 
> Why is there a TabStateChange notification that is differently named and may
> have different semantics than the TabAttributeModified event?
>

I'm not sure I understand this question. On the SDK side we understand it as a state change
for tabs. If you just suggesting changing notification topic to `TabAttributeModified`, I'm
fine with that too, don't really wanna bikeshed on naming, just called whatever I thought made
most sense, happy to call whatever reviewer thinks it should be called.

> 
> > We need to know what has changed which is attribute
> > name and ideally what it
> > has changed to since Fennec and Desktop expose those values in quite
> > different way.
> 
> Using CustomEvent gives you the ability to set event.detail to the attribute
> name, you don't need a MutationEvent for that. 
>

Only reason I preferred MutationEvent over other is because it could carry all
the info I needed (including tab tab, which I'm not sure can be set manually
before dispatching event). If there's a specific reasons for not using MutationEvents
we can definitely do something else, although I'd prefer on format that would be
able to carry all the data we need so we don't have to chase it down.

Could you please let me know why are you against MutationEvents specifically ?

>
> What the attribute has
> changed to you can get by calling getAttribute. Fennec may give you that
> directly, but that's somewhat irrelevant as long as the SDK can abstract
> away such differences (this is after all the primary point of the add-on
> SDK).
>

Yes, but we would rather have one abstraction layer (observer notifications)
or even better fix platforms differences to reduce a glue needed for abstraction.
I actually consider such work an SDK effort.


> 
> > > I don't think we should have a separate _notifyObservers method, as that
> > > encourages that the events and notifications get out of sync. 
> > 
> > I don't see how they get out of sync ? They are dispatched by the same code
> > as you requested.
> 
> They're already out of sync in your patch (see TabAttributeModified vs.
> TabStateChange and the odd TabInitialOpen notification), so you must be
> misunderstanding my concern.
>

You right I don't understand your concerns, could you please be more specific ?
Maybe pointing out specifics in the review would help too. I'm sorry if it's
obvious, I don't have as much experience with this part of codebase.

> 
> > > and
> > > I'm not sure I see the point of the TabInitialOpen notification compared to
> > > e.g. delayed-startup-finished.
> > 
> > 
> > Well use of `browser-delayed-startup-finished` does not gives us a tab for
> > the very least,
> > which means we need to get it ourselfs from window, again different per
> > platform. And even
> > then what if that window has multiple tabs etc.. etc... I'm sure there maybe
> > a way to get
> > it all working, but that would miss the point. We just need to know every
> > Tab opened and
> > doesn't really matter if it was initially there or not.
> 
> If you don't care whether the initial tab is really the initial tab, that
> makes it even simpler to do some manual legwork in the SDK in response to
> browser-delayed-startup-finished.
>

I consider this SDK work. I don't really see why you think that listening to that
observer notification grabbing a tab and then dispatching `TabInitialOpen` notification
is any better. Could you please elaborate on that, would be also great if you can
suggest where that code should live. From my (probably naive) perspective tab related
notifications belong in the tab browser as a source of those notifications.

> 
> > The less dependency we would be have on implementation details of things the
> > better all of
> > us would be as we would be free to change those implementations without
> > affecting each other.
> 
> TabInitialOpen actually exposes you to another implementation detail, which
> is the construction time of the tabbrowser binding. 

Well it's dispatched at the Tab construction time, the fact that tabbrowser is constructed
at the same time is really a side effect.

> The binding being
> constructed too early has been a recurring pain point in the past.

You clearly know all the ins & outs of this, sharing more details would really help us I think.
If you think timing of `TabInitialOpen` is too early and `browser-delayed-startup-finished` is a
better time to do it I can buy int that, although it's very awkward on the user end when one
can grab window and access it's tab, while tab open has not occurred yet. I would also wonder why
tabbrowser won't create it's initial tab at `browser-delayed-startup-finished` as well.

If `browser-delayed-startup-finished` is most appropriate time for `TabInitialOpen` then be it so,
although in that case I would still prefer to observe it and dispatch  `TabInitialOpen` notification
from tabbrowser itself.

Also are there any guarantees that other tabs won't be open before `browser-delayed-startup-finished`
is fired ? I suspect they might which would impose another out of sync issues.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #15)
> I'm not sure I understand this question. On the SDK side we understand it as
> a state change
> for tabs. If you just suggesting changing notification topic to
> `TabAttributeModified`, I'm
> fine with that too, don't really wanna bikeshed on naming, just called
> whatever I thought made
> most sense, happy to call whatever reviewer thinks it should be called.

The only notion of state changes in tabbrowser is from nsIWebProgressListener::onStateChange, so TabStateChange is a misnomer. The notification's name and details should be consistent with the TabAttributeModified event.

> > Using CustomEvent gives you the ability to set event.detail to the attribute
> > name, you don't need a MutationEvent for that. 
> >
> 
> Only reason I preferred MutationEvent over other is because it could carry
> all
> the info I needed (including tab tab, which I'm not sure can be set manually
> before dispatching event). If there's a specific reasons for not using
> MutationEvents
> we can definitely do something else, although I'd prefer on format that
> would be
> able to carry all the data we need so we don't have to chase it down.

I don't think "chase it down" is a fair characterization for an ordinary getAttribute call in order to get the current attribute value.

> Could you please let me know why are you against MutationEvents specifically
> ?

Whenever we add something, it's hard to correct that later, since a later rollback or change in behavior may impair add-on compatibility. Therefore I'm against letting the API grow opportunistically whenever somebody comes along and thinks something might be handy.

> > > > I don't think we should have a separate _notifyObservers method, as that
> > > > encourages that the events and notifications get out of sync. 
> > > 
> > > I don't see how they get out of sync ? They are dispatched by the same code
> > > as you requested.
> > 
> > They're already out of sync in your patch (see TabAttributeModified vs.
> > TabStateChange and the odd TabInitialOpen notification), so you must be
> > misunderstanding my concern.
> >
> 
> You right I don't understand your concerns, could you please be more
> specific ?
> Maybe pointing out specifics in the review would help too. I'm sorry if it's
> obvious, I don't have as much experience with this part of codebase.

I'm not sure how I can be more specific, but I'll try. TabAttributeModified and TabStateChange shouldn't be differently named, because that's inconsistent and confusing. TabInitialOpen shouldn't exist without a corresponding event, because that's inconsistent and confusing as well. Having a _notifyObservers method as in your patch directly enables such inconsistencies. Therefore, we should not have that method.

> > The binding being
> > constructed too early has been a recurring pain point in the past.
> 
> You clearly know all the ins & outs of this, sharing more details would
> really help us I think.

The binding is constructed whenever some code touches the tabbrowser element (e.g. via gBrowser). This can happen even before the load event. Add-ons sometimes do that.

> If you think timing of `TabInitialOpen` is too early and
> `browser-delayed-startup-finished` is a
> better time to do it I can buy int that, although it's very awkward on the
> user end when one
> can grab window and access it's tab, while tab open has not occurred yet.

This all happens too fast and before the first paint, so the user won't notice.

> I would also wonder why
> tabbrowser won't create it's initial tab at
> `browser-delayed-startup-finished` as well.

Because that would break code that assumes it can use tabbrowser before that point.

> If `browser-delayed-startup-finished` is most appropriate time for
> `TabInitialOpen` then be it so,
> although in that case I would still prefer to observe it and dispatch 
> `TabInitialOpen` notification
> from tabbrowser itself.

I don't think this makes much sense, since tabbrowser has no internal dependency on browser-delayed-startup-finished.

> Also are there any guarantees that other tabs won't be open before
> `browser-delayed-startup-finished`
> is fired ? I suspect they might which would impose another out of sync
> issues.

No, there's no such guarantee. In particular, "delayed startup" itself can open multiple tabs before sending the notification. Here's some pseudo code showing how you can deal with that:

> Services.obs.addObserver(observeTabOpen, "TabOpen", false);
> Services.obs.addObserver(notifyInitialTabOpen, "browser-delayed-startup-finished", false);
> 
> function observeTabOpen(tab) {
>   let window = tab.ownerDocument.defaultView;
>   notifyInitialTabOpen(window);
> 
>   notifyTabOpen(tab);
> }
> 
> let notifiedInitialTabOpen = new WeakMap;
> 
> function notifyInitialTabOpen(window) {
>   if (notifiedInitialTabOpen.has(window))
>     return;
>   notifiedInitialTabOpen.set(window, true);
> 
>   notifyTabOpen(window.gBrowser.tabs[0]);
> }
> 
> function notifyTabOpen(tab) {
>   ...
> }

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8338782 [details] [diff] [review]
bug-843901-2.patch

As I understand it this is waiting on a new patch that addresses dao's concerns.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

If the concerns aren't understood, please don't hesitate to reach out and we can discuss further. But discussing specific proposals in the form of a patch is probably the easiest way to reach agreement here.

Comment 19

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

Yes I'm going to write up a new patch, I just was pulled to do some other work.