expose unique tab ids to the high-level tabs module #595

Merged
merged 2 commits into from Dec 28, 2012

Conversation

Projects
None yet
3 participants

@ghost ghost assigned erikvold Sep 29, 2012

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Oct 3, 2012

Member

Two things to consider before going ahead with this:

  1. Do we actually have a need for a properties like id. In chrome APIs they seem to be a primary mechanism for dealing with identity not the objects themselves. In our case tab instance itself serves this purpose. In other words in chrome you pass tab id across diff APIs in our case you pass object instead.
  2. Can we guarantee that we'll be able to support this in a future ? I guess answer is yes, if things will come to worst we could make our own ID's.
Member

Gozala commented Oct 3, 2012

Two things to consider before going ahead with this:

  1. Do we actually have a need for a properties like id. In chrome APIs they seem to be a primary mechanism for dealing with identity not the objects themselves. In our case tab instance itself serves this purpose. In other words in chrome you pass tab id across diff APIs in our case you pass object instead.
  2. Can we guarantee that we'll be able to support this in a future ? I guess answer is yes, if things will come to worst we could make our own ID's.
@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Oct 3, 2012

Member

After giving this a little bit more thought, I'm convinced that exposing tab.id is a good idea, in fact having such property would have made our lives a lot easier.

Member

Gozala commented Oct 3, 2012

After giving this a little bit more thought, I'm convinced that exposing tab.id is a good idea, in fact having such property would have made our lives a lot easier.

@Gozala

View changes

packages/api-utils/lib/tabs/utils.js
+ if (tab.browser) // fennec
+ return tab.id
+
+ return String.split(tab.linkedPanel, 'panel').pop();

This comment has been minimized.

@Gozala

Gozala Oct 3, 2012

Member

I'm not sure it's relable ?

@Gozala

Gozala Oct 3, 2012

Member

I'm not sure it's relable ?

This comment has been minimized.

@canuckistani

canuckistani Oct 3, 2012

Contributor

It's reliable for desktop as the linkedPanel property always returns a string 'panel1234567' for example. There would need to be a test written to ensure that the platform continues to produce that string format as the property.

I do this string manipulation out for aesthetic reasons, it looks wrong for something like 'tab.id' to have a value 'panel1234' - panels are something completely different in the SDK and we don't want to confuse people.

I think I have three action items here:

  1. write a test that will fail if the value of tab.linkedPanel changes
  2. investigate a cleaner way to re-format the linkedPanel value
  3. do some tests on fennec to see what their tab.id looks like
@canuckistani

canuckistani Oct 3, 2012

Contributor

It's reliable for desktop as the linkedPanel property always returns a string 'panel1234567' for example. There would need to be a test written to ensure that the platform continues to produce that string format as the property.

I do this string manipulation out for aesthetic reasons, it looks wrong for something like 'tab.id' to have a value 'panel1234' - panels are something completely different in the SDK and we don't want to confuse people.

I think I have three action items here:

  1. write a test that will fail if the value of tab.linkedPanel changes
  2. investigate a cleaner way to re-format the linkedPanel value
  3. do some tests on fennec to see what their tab.id looks like
@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Oct 3, 2012

Member

Now even though I think tab.id / window.id is a good idea, I would be more comfortable if we could check with someone on the platform side to make sure. All the APIs I could have though of tend to avoid using id's, making me wonder if there are intentional reasons. Take a look at https://developer.mozilla.org/en-US/docs/Session_store_API for example.

Also implementation does not seems to use any particular attribute as an identifier, which is concerning:
https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm

Maybe one of the contributors @andrez47 @ttaubert @iAke @MichaelKohler to session-store could tell use why ?

With this, I think we should avoid adding such attribute until we get more insights. Also if there is no particular reason for not having tab id's, I think we should just add them to the firefox similar to fennec and expose only after.

Member

Gozala commented Oct 3, 2012

Now even though I think tab.id / window.id is a good idea, I would be more comfortable if we could check with someone on the platform side to make sure. All the APIs I could have though of tend to avoid using id's, making me wonder if there are intentional reasons. Take a look at https://developer.mozilla.org/en-US/docs/Session_store_API for example.

Also implementation does not seems to use any particular attribute as an identifier, which is concerning:
https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm

Maybe one of the contributors @andrez47 @ttaubert @iAke @MichaelKohler to session-store could tell use why ?

With this, I think we should avoid adding such attribute until we get more insights. Also if there is no particular reason for not having tab id's, I think we should just add them to the firefox similar to fennec and expose only after.

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Oct 8, 2012

Member

I had some conversations with Gavin and others at jsconf.eu it looks like there is no specific reason why tab's should not have id. So I'm ok with landing as is, also we should note in the docs that this ID is not preserved across restarts. Also we should create a bug to implement actual tab.id in the platform, it would be best if we'll just adopt whatever Fennec does in a desktop FF

Member

Gozala commented Oct 8, 2012

I had some conversations with Gavin and others at jsconf.eu it looks like there is no specific reason why tab's should not have id. So I'm ok with landing as is, also we should note in the docs that this ID is not preserved across restarts. Also we should create a bug to implement actual tab.id in the platform, it would be best if we'll just adopt whatever Fennec does in a desktop FF

@canuckistani

This comment has been minimized.

Show comment
Hide comment
@canuckistani

canuckistani Dec 14, 2012

Contributor

@Gozala, @erikvold - I re-applied my patch to current master, should be good to land?

Contributor

canuckistani commented Dec 14, 2012

@Gozala, @erikvold - I re-applied my patch to current master, should be good to land?

@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Dec 14, 2012

Contributor

@canuckistani I sees no tests

Contributor

erikvold commented Dec 14, 2012

@canuckistani I sees no tests

@canuckistani

This comment has been minimized.

Show comment
Hide comment
@canuckistani

canuckistani Dec 27, 2012

Contributor

@erikvold @Gozala added some very basic tests, the tests pass. Okay to land? Or, advice on what more we could test here.

Contributor

canuckistani commented Dec 27, 2012

@erikvold @Gozala added some very basic tests, the tests pass. Okay to land? Or, advice on what more we could test here.

@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Dec 27, 2012

Contributor

do we have some confirmation that the tab.linkedPanel is globally unique?

Contributor

erikvold commented Dec 27, 2012

do we have some confirmation that the tab.linkedPanel is globally unique?

@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Dec 27, 2012

Contributor

I'd like to see a test for Firefox that opens two tabs in separate windows and checks that the ids are not equal. Also another test that confirms that the id is a string that only contains numbers.

Contributor

erikvold commented Dec 27, 2012

I'd like to see a test for Firefox that opens two tabs in separate windows and checks that the ids are not equal. Also another test that confirms that the id is a string that only contains numbers.

@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Dec 27, 2012

Contributor

Also another test that opens 2 tabs in the same window and confirms that those two ids are not equal (for fennec).

Contributor

erikvold commented Dec 27, 2012

Also another test that opens 2 tabs in the same window and confirms that those two ids are not equal (for fennec).

@canuckistani

This comment has been minimized.

Show comment
Hide comment
@canuckistani

canuckistani Dec 27, 2012

Contributor

Ok, I'll look into filling out those tests. FWIW, while the docs alude to the possible use of multiple panels per tab, etc, this does not seem to be how Firefox uses it:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1355

1352             var position = this.tabs.length - 1;
1353             var uniqueId = "panel" + Date.now() + position;
1354             notificationbox.id = uniqueId;
1355             t.linkedPanel = uniqueId;
1356             t.linkedBrowser = b;
1357             t._tPos = position;
Contributor

canuckistani commented Dec 27, 2012

Ok, I'll look into filling out those tests. FWIW, while the docs alude to the possible use of multiple panels per tab, etc, this does not seem to be how Firefox uses it:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1355

1352             var position = this.tabs.length - 1;
1353             var uniqueId = "panel" + Date.now() + position;
1354             notificationbox.id = uniqueId;
1355             t.linkedPanel = uniqueId;
1356             t.linkedBrowser = b;
1357             t._tPos = position;
@canuckistani

This comment has been minimized.

Show comment
Hide comment
@canuckistani

canuckistani Dec 28, 2012

Contributor

@erikvold I've added additional tests, so even if we changed the underlying implementation on Firefox these tests should be helpful.

Back to the underlying question though, based on that mxr link in my previous comment, I don't see how we could get duplicate tabs based on how they generate the linkedPanel ids ( timestamp + tab index. )

Contributor

canuckistani commented Dec 28, 2012

@erikvold I've added additional tests, so even if we changed the underlying implementation on Firefox these tests should be helpful.

Back to the underlying question though, based on that mxr link in my previous comment, I don't see how we could get duplicate tabs based on how they generate the linkedPanel ids ( timestamp + tab index. )

erikvold added a commit that referenced this pull request Dec 28, 2012

Merge pull request #595 from canuckistani/bug_795645
Fix Bug 795645 expose unique tab ids to the high-level tabs module r=@erikvold

@erikvold erikvold merged commit db4f668 into mozilla:master Dec 28, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment