Changes instant so that the newly created tab has a new id. Doing

Changes instant so that the newly created tab has a new id. Doing
otherwise causes subtle problems with extensions and breaks the
invariant that the tab id is unique.

BUG=68508
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71063

[sky, 2011-01-10 23:28:26]

Brett, could you review the change to NavigationController, and Rafael the rest?
The interesting bit in the rest is the change to ExtensionBrowserEventRouter,
the rest is the result of adding an additional arg to
TabStripModelObserver::TabReplacedAt so that we don't have to figure it out
ourselves.

Thanks!

  -Scott

[rafaelw, 2011-01-11 01:24:59]

lgtm.

Aaron/Erik: I think this means that "committing" a instant search on top of an
existing tab will look to the tabs api like a tab closed and a new one opened.

It also means that if there's a content script, it will have been hearing about
a sequence of new tab ids (that the tabs api won't know anything else about
because it's not a part of a tab strip), that never sent a tabCreated.

Finally, when the "commit" causes a tabCreated, it will occur after that script
got injected into the tabs content (different order from the usual progression).

[brettw, 2011-01-11 06:17:55]

NavController LGTM

[Erik does not do reviews, 2011-01-11 17:08:17]

On Mon, Jan 10, 2011 at 5:25 PM, <rafaelw@chromium.org> wrote:

> lgtm.
>
> Aaron/Erik: I think this means that "committing" a instant search on top of
> an
> existing tab will look to the tabs api like a tab closed and a new one
> opened.
>

Yes.  I think that's reasonable.



> It also means that if there's a content script, it will have been hearing
> about
> a sequence of new tab ids (that the tabs api won't know anything else about
> because it's not a part of a tab strip), that never sent a tabCreated.
>

Yes.  I chatted with Scott a bit about this yesterday.  This is my largest
area of concern.  However, given that the instant TabContents simply gets
swapped in, I think it's important that the content script actually run in
the page.  This means that we need to make sure that the tabs API (or
anything that uses tab ids) properly reacts to tabs without a window.  This
was already a potential problem in a number of scenarios (dragging tab, sync
login dialog, etc.).  Also, given that tab ids are guessable (monotonically
increasing integers), even if there aren't content scripts injected, we
still needed to protect against use of random tab ids.  This is especially
true as ChromeOS is using TabContents for more system things (screen saver,
html dialogs, etc.).



> Finally, when the "commit" causes a tabCreated, it will occur after that
> script
> got injected into the tabs content (different order from the usual
> progression).


Would it be better if there was no tabCreated in this case and we just got a
tabUpdated?  Maybe the better thing is to do a tabCreated right away when
the instant tab is first created (but without a windowId or an index).

Unfortunately, in all of these scenarios, I can imagine poorly written
extensions being surprised by something, so breakage seems inevitable.
 Anybody have any other ideas?

Erik

[rafaelw1, 2011-01-11 18:55:04]

On Tue, Jan 11, 2011 at 9:08 AM, Erik Kay <erikkay@chromium.org> wrote:

> On Mon, Jan 10, 2011 at 5:25 PM, <rafaelw@chromium.org> wrote:
>
>> lgtm.
>>
>> Aaron/Erik: I think this means that "committing" a instant search on top
>> of an
>> existing tab will look to the tabs api like a tab closed and a new one
>> opened.
>>
>
> Yes.  I think that's reasonable.
>
>
>
>> It also means that if there's a content script, it will have been hearing
>> about
>> a sequence of new tab ids (that the tabs api won't know anything else
>> about
>> because it's not a part of a tab strip), that never sent a tabCreated.
>>
>
> Yes.  I chatted with Scott a bit about this yesterday.  This is my largest
> area of concern.  However, given that the instant TabContents simply gets
> swapped in, I think it's important that the content script actually run in
> the page.  This means that we need to make sure that the tabs API (or
> anything that uses tab ids) properly reacts to tabs without a window.  This
> was already a potential problem in a number of scenarios (dragging tab, sync
> login dialog, etc.).  Also, given that tab ids are guessable (monotonically
> increasing integers), even if there aren't content scripts injected, we
> still needed to protect against use of random tab ids.  This is especially
> true as ChromeOS is using TabContents for more system things (screen saver,
> html dialogs, etc.).
>
>
>
>> Finally, when the "commit" causes a tabCreated, it will occur after that
>> script
>> got injected into the tabs content (different order from the usual
>> progression).
>
>
> Would it be better if there was no tabCreated in this case and we just got
> a tabUpdated?  Maybe the better thing is to do a tabCreated right away when
> the instant tab is first created (but without a windowId or an index).
>

I can see both sides to this, but I think my vote is for waiting to send a
tabCreated until the instant tab commits (status quo once this CL goes in).
(a) It seems least likely to break existing extensions that may be assuming
when they get a tabCreated, that it has a window, (b) I think it maps
conceptually to what's actually going on and (c) my gut says that most users
of the tabs API should ignore the instant tabs.


>
> Unfortunately, in all of these scenarios, I can imagine poorly written
> extensions being surprised by something, so breakage seems inevitable.
>  Anybody have any other ideas?
>
> Erik
>
>

[Erik does not do reviews, 2011-01-11 21:18:50]

+ben (FYI - this is the kind of thing I was talking about)

On Tue, Jan 11, 2011 at 10:54 AM, Rafael Weinstein <rafaelw@google.com>wrote:

>
>
> On Tue, Jan 11, 2011 at 9:08 AM, Erik Kay <erikkay@chromium.org> wrote:
>
>> On Mon, Jan 10, 2011 at 5:25 PM, <rafaelw@chromium.org> wrote:
>>
>>> lgtm.
>>>
>>> Aaron/Erik: I think this means that "committing" a instant search on top
>>> of an
>>> existing tab will look to the tabs api like a tab closed and a new one
>>> opened.
>>>
>>
>> Yes.  I think that's reasonable.
>>
>>
>>
>>> It also means that if there's a content script, it will have been hearing
>>> about
>>> a sequence of new tab ids (that the tabs api won't know anything else
>>> about
>>> because it's not a part of a tab strip), that never sent a tabCreated.
>>>
>>
>> Yes.  I chatted with Scott a bit about this yesterday.  This is my largest
>> area of concern.  However, given that the instant TabContents simply gets
>> swapped in, I think it's important that the content script actually run in
>> the page.  This means that we need to make sure that the tabs API (or
>> anything that uses tab ids) properly reacts to tabs without a window.  This
>> was already a potential problem in a number of scenarios (dragging tab, sync
>> login dialog, etc.).  Also, given that tab ids are guessable (monotonically
>> increasing integers), even if there aren't content scripts injected, we
>> still needed to protect against use of random tab ids.  This is especially
>> true as ChromeOS is using TabContents for more system things (screen saver,
>> html dialogs, etc.).
>>
>>
>>
>>> Finally, when the "commit" causes a tabCreated, it will occur after that
>>> script
>>> got injected into the tabs content (different order from the usual
>>> progression).
>>
>>
>> Would it be better if there was no tabCreated in this case and we just got
>> a tabUpdated?  Maybe the better thing is to do a tabCreated right away when
>> the instant tab is first created (but without a windowId or an index).
>>
>
> I can see both sides to this, but I think my vote is for waiting to send a
> tabCreated until the instant tab commits (status quo once this CL goes in).
> (a) It seems least likely to break existing extensions that may be assuming
> when they get a tabCreated, that it has a window, (b) I think it maps
> conceptually to what's actually going on and (c) my gut says that most users
> of the tabs API should ignore the instant tabs.
>

So in this world, "created" means "added to tab strip model"?  One thing
that this precludes is using executeScript to dynamically inject scripts
into these pages when they're first displayed.  I'm also worried that there
are going to be other cases where a tab is only ever created outside of this
context (and maybe never get put into a tabstrip), but will still need
content scripts injected.  For example, what about the "wicked fast" guys
that are preloading pages without displaying them?

Maybe for compatibility purposes, we need a new event?  onTabPreloaded?
 onTabInstantiated?  Maybe we should deprecate onTabCreated and rename it
onTabInserted?

Erik



>
>
>>
>> Unfortunately, in all of these scenarios, I can imagine poorly written
>> extensions being surprised by something, so breakage seems inevitable.
>>  Anybody have any other ideas?
>>
>> Erik
>>
>>
>