943671 - Implement `tab.id`

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

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

As already mentioned in Bug 843901 on SDK side we would really appreciate to
have a consistent API for tracking state of tabs. Identifying those tab's is
the key in doing it. While there's `tab.linkedPanel` it feels wrong to depend
on such an intimate implementation details that may change at some point. It would
be great if we had a tab interface consistent with a Fennec's Tab:
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Tab

For the very least `tab.id`.

If I can get component owners to agree I would also be happy to implement rest of the APIs:
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/BrowserApp

Comment 1

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

Attachment: bug-943671.patch

Comment 2

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

To put it in formal list of things I would like to implement & prior to that get
feedback upon is:


Tab API: https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Tab

1. Implement `id` field on the tab instances (see https://bugzilla.mozilla.org/attachment.cgi?id=8338921)
2. Implement `browser` getter for `tabbrowser-tab` binding that would return
   `this.linkedBrowser`.

Browser API:

3. Implement `getTabForId` on `tabbrowser` binding that takes `tab.id` and returns tab.
4. Promote or alias existing `_getTabForBrowser` to as `getTabForBrowser`.
5. Implement `closeTab(tab)` on `tabbrowser` binding that simply closes given tab `tab.close()`.
6. Implement `selectTab(tab)` on `tabbrowser` that simply does `this.selectedTab = tab`.


I wish we also could normalize existing methods so that they would behave consistently 
across the platforms like:

7. loadURI
8. addTab

Maybe alternative for later two would be to have non conflicting name that would implement
same behavior across desktop and mobile.

Comment 3

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

Needinfo both Gavin and Mark to hopefully agree on something that can be consistent across desktop and mobile.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Why do you need a tab ID? Since we had WeakMaps I didn't feel the need to ever have an ID for a tab or browser. And if you for some reason need an arbitrary identifier you can totally put that in a WeakMap for every tab out there.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #2)
> 2. Implement `browser` getter for `tabbrowser-tab` binding that would return
>    `this.linkedBrowser`.

Fine by me.

> 4. Promote or alias existing `_getTabForBrowser` to as `getTabForBrowser`.

The reason why we hadn't done this so far is that _getTabForBrowser is relatively inefficient, so we don't want people to overuse it.

> 5. Implement `closeTab(tab)` on `tabbrowser` binding that simply closes
> given tab `tab.close()`.

We've had removeTab since about forever, adding an alias at this point just adds confusion. Maybe Fennec, being younger, can get its method renamed?

> 6. Implement `selectTab(tab)` on `tabbrowser` that simply does
> `this.selectedTab = tab`.

I see no value in this change either.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> > 4. Promote or alias existing `_getTabForBrowser` to as `getTabForBrowser`.
> 
> The reason why we hadn't done this so far is that _getTabForBrowser is
> relatively inefficient, so we don't want people to overuse it.

We could implement/maintain a WeakMap and then expose it maybe?

Comment 7

[Dão Gottwald [:dao]]

We could probably set browser.tab just like we set tab.linkedBrowser.

Comment 8

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

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Why do you need a tab ID? Since we had WeakMaps I didn't feel the need to
> ever have an ID for a tab or browser. And if you for some reason need an
> arbitrary identifier you can totally put that in a WeakMap for every tab out
> there.

We want to be able to identify tabs across the process boundaries (I believe that's fennec's reason for
having tab.id as well), unfortunately weak maps aren't ideal for this, as it would imply our manual ref-counting to avoid leaks, so I'd rather just expose tab.id for such use cases.

Comment 9

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

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #2)
> > 2. Implement `browser` getter for `tabbrowser-tab` binding that would return
> >    `this.linkedBrowser`.
> 
> Fine by me.
> 
> > 4. Promote or alias existing `_getTabForBrowser` to as `getTabForBrowser`.
> 
> The reason why we hadn't done this so far is that _getTabForBrowser is
> relatively inefficient, so we don't want people to overuse it.

How about if my patch would also optimize that. I can set `browser.id` along with `tab.id`
so that `_getTabForBrowser` would something like:

tab.ownerDocument.getElementById(tab.id.replace("tab", "browser"))

> 
> > 5. Implement `closeTab(tab)` on `tabbrowser` binding that simply closes
> > given tab `tab.close()`.
> 
> We've had removeTab since about forever, adding an alias at this point just
> adds confusion. Maybe Fennec, being younger, can get its method renamed?

Fare enough hope Mark will be ok with that.

> 
> > 6. Implement `selectTab(tab)` on `tabbrowser` that simply does
> > `this.selectedTab = tab`.
> 
> I see no value in this change either.

Well one reason is to have a compatible API with fennec. Also personally
I'm feel kind of awkward with APIs that side effects occur by assigning.

Comment 10

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

Attachment: Implement tab.browser

Patch to expose `tab.browser`

Comment 11

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

Attachment: Implement `gBrowser.getTabForId`

This depends on the patch that add's tab.id

Comment 12

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

Attachment: Implementation of `gBrowser.getTabForBrowser`

This patch inlines `tab.id` patch to illustrate how comment regarding
`getTabForBrowser` can be addressed.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I chatted with Irakli on IRC and we decided to see what sort of patches he needs to make tweaks to the Fennec APIs. I'd be OK with making some changes if it helps the cause.

Comment 14

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

Comment on attachment 8338921 [details] [diff] [review]
bug-943671.patch

This seems to change the format of linkedPanel (switching the order of localID and outerID). That seems unnecessary and undesirable in general (though it probably doesn't matter too much in practice).

Otherwise this seems fine, though someone should review it more closely.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8338921 [details] [diff] [review]
bug-943671.patch

Review of attachment 8338921 [details] [diff] [review]:
-----------------------------------------------------------------

> this._setupUniqueID(t);
> notificationbox.id = t.linkedPanel;

This isn't very clear in what it does, imo. The old version seemed more expressive to me, with a side-effect free function. This is also partly due to "linkedPanel" being an unfortunate property name but we will have to stick with that for now. How about doing something like:

> let uidPostfix = this._generateUniqueIDPostfix(t);
> t.id = "tab-" + uidPostfix;
> notificationbox.id = t.linkedPanel = "panel-" + uidPostfix;

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8341280 [details] [diff] [review]
Implement `gBrowser.getTabForId`

Review of attachment 8341280 [details] [diff] [review]:
-----------------------------------------------------------------

It bugs me a little that it would be too easy to break this for add-ons. I don't think it's very likely for an add-on to introduce elements with colliding IDs but we'd be much less fragile by maintaining an internal map. Not sure what others think about this, is that too much overhead?

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8341289 [details] [diff] [review]
Implementation of `gBrowser.getTabForBrowser`

>       <method name="_getTabForBrowser">
>         <parameter name="aBrowser"/>
>         <body>
>         <![CDATA[
>-          for (let i = 0; i < this.tabs.length; i++) {
>-            if (this.tabs[i].linkedBrowser == aBrowser)
>-              return this.tabs[i];
>-          }
>-          return null;
>+          let id = aBrowser.id.replace("browser", "tab");
>+          return document.getElementById(id);
>+        ]]>
>+        </body>
>+      </method>

How about implementing browser.tab instead?

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Dão Gottwald [:dao] from comment #17)

> How about implementing browser.tab instead?

In a way that doesn't add any code to browser.xml, right?

Comment 19

[Dão Gottwald [:dao]]

Right, it could just be an expando property set by tabbrowser.xml.

Comment 20

[Carsten Book [:Tomcat]]

can we get a try link for this change? thanks!

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Dao, is this something that we should still consider? It looks like it got forgotten and I don't think we have a need for it anymore considering it wouldn't have been forgotten otherwise.

Comment 22

[Dão Gottwald [:dao]]

There's now browser.permanentKey and tab.permanentKey.