Make an inserted selected tab selected before calling TabInsertedAt on observers.

Use active information fromTabInsertedAt in browser event router so that the tabs.onCreated callback result has the correct active ("selected" in the API) state.

BUG=59194
TEST=in ExtensionApiTest.TabEvents, reenabled tabsOnCreated


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

[sky, 2011-06-09 17:13:00]

http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
I'm nervous of the effects of doing this on the TabStrip implementations. dpapad
is also in the middle of changing this code:
http://codereview.chromium.org/7033048/ .

Why can't the extension code use the boolean passed to TabInsertedAt?

[dpapad, 2011-06-09 17:30:37]

On 2011/06/09 17:13:00, sky wrote:
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> File chrome/browser/tabs/tab_strip_model.cc (right):
> 
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
> I'm nervous of the effects of doing this on the TabStrip implementations.
dpapad
> is also in the middle of changing this code:
> http://codereview.chromium.org/7033048/ .
> 
> Why can't the extension code use the boolean passed to TabInsertedAt?

BTW, we should enforce the distinction between "selected" and "active" within
the code from now on and make it clear that selected != active. I have done some
renaming to achieve this, but there is still plenty of renaming to do.

[Yoyo Zhou, 2011-06-09 17:32:56]

On 2011/06/09 17:30:37, dpapad wrote:
> On 2011/06/09 17:13:00, sky wrote:
> >
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> > File chrome/browser/tabs/tab_strip_model.cc (right):
> > 
> >
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> > chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
> > I'm nervous of the effects of doing this on the TabStrip implementations.
> dpapad
> > is also in the middle of changing this code:
> > http://codereview.chromium.org/7033048/ .
> > 
> > Why can't the extension code use the boolean passed to TabInsertedAt?
> 
> BTW, we should enforce the distinction between "selected" and "active" within
> the code from now on and make it clear that selected != active. I have done
some
> renaming to achieve this, but there is still plenty of renaming to do.

Oh no... Can you please clarify what they each should mean?

[sky, 2011-06-09 17:37:16]

On Thu, Jun 9, 2011 at 10:30 AM,  <dpapad@chromium.org> wrote:
> On 2011/06/09 17:13:00, sky wrote:
>
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
>>
>> File chrome/browser/tabs/tab_strip_model.cc (right):
>
>
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
>>
>> chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
>> I'm nervous of the effects of doing this on the TabStrip implementations.
>
> dpapad
>>
>> is also in the middle of changing this code:
>> http://codereview.chromium.org/7033048/ .
>
>> Why can't the extension code use the boolean passed to TabInsertedAt?
>
> BTW, we should enforce the distinction between "selected" and "active"
> within
> the code from now on and make it clear that selected != active. I have done
> some
> renaming to achieve this, but there is still plenty of renaming to do.

Agreed. I appreciate your efforts here.

On the extensions side is harder though as they have used 'selected'
in the api. http://code.google.com/p/chromium/issues/detail?id=81411
is against updating extensions for multi-selection.

  -Scott

[dpapad, 2011-06-09 17:38:34]

On 2011/06/09 17:30:37, dpapad wrote:
> On 2011/06/09 17:13:00, sky wrote:
> >
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> > File chrome/browser/tabs/tab_strip_model.cc (right):
> > 
> >
>
http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
> > chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
> > I'm nervous of the effects of doing this on the TabStrip implementations.
> dpapad
> > is also in the middle of changing this code:
> > http://codereview.chromium.org/7033048/ .
> > 
> > Why can't the extension code use the boolean passed to TabInsertedAt?
> 
> BTW, we should enforce the distinction between "selected" and "active" within
> the code from now on and make it clear that selected != active. I have done
some
> renaming to achieve this, but there is still plenty of renaming to do.

Active is the tab that is being displayed and there can be only one active tab
at a time. An active tab is selected also. 

But there can be selected tabs that are not active (since we added multi-tab
selection, allowing to perform operations on many tabs at the same time). Try
shift+clicking or ctrl+clicking on a tab and then right click (use the windows
canary build) to understand it better.

[Yoyo Zhou, 2011-06-09 18:08:53]

http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
File chrome/browser/tabs/tab_strip_model.cc (right):

http://codereview.chromium.org/7134035/diff/4/chrome/browser/tabs/tab_strip_m...
chrome/browser/tabs/tab_strip_model.cc:158: if (active) {
On 2011/06/09 17:13:01, sky wrote:
> I'm nervous of the effects of doing this on the TabStrip implementations.
dpapad
> is also in the middle of changing this code:
> http://codereview.chromium.org/7033048/ .
> 
> Why can't the extension code use the boolean passed to TabInsertedAt?

It gets discarded in ExtensionBrowserEventRouter::TabCreatedAt. I'm not sure
why; we could certainly use it.

Actually, we seem to have related bugs with tab selection/activeness and window
focus, where the callback result value from creating a tab or window is derived
directly from the TabContents or Browser, which may not have actually become
active or focused yet.

It sounds like you're suggesting that in the result value, we should override
the TabContents or Browser's active/focused state with what it will become.

I'm not sure about the implications of telling the callback function that a tab
is active when it might not yet be in the UI. +asargent who might know more
about the timing of the tabs API.

[jstritar, 2011-06-09 18:27:19]

On 2011/06/09 18:08:53, Yoyo Zhou wrote:
>
> It gets discarded in ExtensionBrowserEventRouter::TabCreatedAt. I'm not sure
> why; we could certainly use it.
> 
> Actually, we seem to have related bugs with tab selection/activeness and
window
> focus, where the callback result value from creating a tab or window is
derived
> directly from the TabContents or Browser, which may not have actually become
> active or focused yet.
> 
> It sounds like you're suggesting that in the result value, we should override
> the TabContents or Browser's active/focused state with what it will become.
> 
> I'm not sure about the implications of telling the callback function that a
tab
> is active when it might not yet be in the UI. +asargent who might know more
> about the timing of the tabs API.
>

I remember looking into this before. I think the tab model updates its active
tab based on the GTK / window manager events, which are fired later (after
rendering?), but the tab strip model observer fires when the model itself
changes (like when you call the TabStripModel methods), so the two are out of
sync. Using the value passed into the observers may work better since although
it may not have finished rendering, it will be in the tab strip.

[sky, 2011-06-09 18:30:05]

On Thu, Jun 9, 2011 at 11:27 AM,  <jstritar@chromium.org> wrote:
> On 2011/06/09 18:08:53, Yoyo Zhou wrote:
>
>> It gets discarded in ExtensionBrowserEventRouter::TabCreatedAt. I'm not
>> sure
>> why; we could certainly use it.
>
>> Actually, we seem to have related bugs with tab selection/activeness and
>
> window
>>
>> focus, where the callback result value from creating a tab or window is
>
> derived
>>
>> directly from the TabContents or Browser, which may not have actually
>> become
>> active or focused yet.
>
>> It sounds like you're suggesting that in the result value, we should
>> override
>> the TabContents or Browser's active/focused state with what it will
>> become.
>
>> I'm not sure about the implications of telling the callback function that
>> a
>
> tab
>>
>> is active when it might not yet be in the UI. +asargent who might know
>> more
>> about the timing of the tabs API.
>
>
> I remember looking into this before. I think the tab model updates its
> active
> tab based on the GTK / window manager events, which are fired later (after
> rendering?), but the tab strip model observer fires when the model itself
> changes (like when you call the TabStripModel methods), so the two are out
> of
> sync. Using the value passed into the observers may work better since
> although
> it may not have finished rendering, it will be in the tab strip.
>
> http://codereview.chromium.org/7134035/
>

The UI should update the from the model. For example, the user clicks
on a tab, which triggers updating the model and repainting the UI.

  -Scott

[Yoyo Zhou, 2011-06-09 19:18:01]

On 2011/06/09 18:30:05, sky wrote:
> On Thu, Jun 9, 2011 at 11:27 AM,  <mailto:jstritar@chromium.org> wrote:
> > On 2011/06/09 18:08:53, Yoyo Zhou wrote:
> >
> >> It gets discarded in ExtensionBrowserEventRouter::TabCreatedAt. I'm not
> >> sure
> >> why; we could certainly use it.
> >
> >> Actually, we seem to have related bugs with tab selection/activeness and
> >
> > window
> >>
> >> focus, where the callback result value from creating a tab or window is
> >
> > derived
> >>
> >> directly from the TabContents or Browser, which may not have actually
> >> become
> >> active or focused yet.
> >
> >> It sounds like you're suggesting that in the result value, we should
> >> override
> >> the TabContents or Browser's active/focused state with what it will
> >> become.
> >
> >> I'm not sure about the implications of telling the callback function that
> >> a
> >
> > tab
> >>
> >> is active when it might not yet be in the UI. +asargent who might know
> >> more
> >> about the timing of the tabs API.
> >
> >
> > I remember looking into this before. I think the tab model updates its
> > active
> > tab based on the GTK / window manager events, which are fired later (after
> > rendering?), but the tab strip model observer fires when the model itself
> > changes (like when you call the TabStripModel methods), so the two are out
> > of
> > sync. Using the value passed into the observers may work better since
> > although
> > it may not have finished rendering, it will be in the tab strip.
> >
> > http://codereview.chromium.org/7134035/
> >
> 
> The UI should update the from the model. For example, the user clicks
> on a tab, which triggers updating the model and repainting the UI.

Ok, but here (if active == true) the TabStripModel hasn't been updated with
SetSelectedIndex before notifying observers that TabInsertedAt with active ==
true. That's why this seemed wrong to me initially.

[sky, 2011-06-09 19:22:39]

On Thu, Jun 9, 2011 at 12:18 PM,  <yoz@chromium.org> wrote:
> On 2011/06/09 18:30:05, sky wrote:
>>
>> On Thu, Jun 9, 2011 at 11:27 AM,  <mailto:jstritar@chromium.org> wrote:
>> > On 2011/06/09 18:08:53, Yoyo Zhou wrote:
>> >
>> >> It gets discarded in ExtensionBrowserEventRouter::TabCreatedAt. I'm not
>> >> sure
>> >> why; we could certainly use it.
>> >
>> >> Actually, we seem to have related bugs with tab selection/activeness
>> >> and
>> >
>> > window
>> >>
>> >> focus, where the callback result value from creating a tab or window is
>> >
>> > derived
>> >>
>> >> directly from the TabContents or Browser, which may not have actually
>> >> become
>> >> active or focused yet.
>> >
>> >> It sounds like you're suggesting that in the result value, we should
>> >> override
>> >> the TabContents or Browser's active/focused state with what it will
>> >> become.
>> >
>> >> I'm not sure about the implications of telling the callback function
>> >> that
>> >> a
>> >
>> > tab
>> >>
>> >> is active when it might not yet be in the UI. +asargent who might know
>> >> more
>> >> about the timing of the tabs API.
>> >
>> >
>> > I remember looking into this before. I think the tab model updates its
>> > active
>> > tab based on the GTK / window manager events, which are fired later
>> > (after
>> > rendering?), but the tab strip model observer fires when the model
>> > itself
>> > changes (like when you call the TabStripModel methods), so the two are
>> > out
>> > of
>> > sync. Using the value passed into the observers may work better since
>> > although
>> > it may not have finished rendering, it will be in the tab strip.
>> >
>> > http://codereview.chromium.org/7134035/
>> >
>
>> The UI should update the from the model. For example, the user clicks
>> on a tab, which triggers updating the model and repainting the UI.
>
> Ok, but here (if active == true) the TabStripModel hasn't been updated with
> SetSelectedIndex before notifying observers that TabInsertedAt with active
> ==
> true. That's why this seemed wrong to me initially.

OTOH your change would modify the selection without notifying.
Observers then see an inconsistent state. That's what makes me nervous
and that's why you should use the boolean passed to TabInsertedAt to
know if it's going to be active.

  -Scott

[Yoyo Zhou, 2011-06-09 22:04:16]

On 2011/06/09 19:22:39, sky wrote:
> OTOH your change would modify the selection without notifying.
> Observers then see an inconsistent state. That's what makes me nervous
> and that's why you should use the boolean passed to TabInsertedAt to
> know if it's going to be active.

Okay, I revised this change to leave TabStripModel alone, and to actually use
the boolean. Another look?

Note that I have now assumed that page actions and browser actions can only be
executed on a selected tab. Let me know if this is incorrect. Also, I named
things "selected" rather than "active" because that's what the tabs API calls
them. =/

[sky, 2011-06-09 22:29:19]

Your method names have to be named like chrome code (eg ActiveTabChanged). I
also think we should strive to be self consistent. For these reasons you should
go with active and convert to selected only when passing values to the
extension.

  -Scott

[Yoyo Zhou, 2011-06-09 23:09:59]

On 2011/06/09 22:29:19, sky wrote:
> Your method names have to be named like chrome code (eg ActiveTabChanged). I
> also think we should strive to be self consistent. For these reasons you
should
> go with active and convert to selected only when passing values to the
> extension.

I changed selected to active (and in other places where it made sense).

As for the function name CreateTabValueWithSelection, I don't like it much
either, but there are other names with similar grammar (e.g.
DispatchEventWithTab). I changed it to CreateTabValueActive, which isn't much
better. Just CreateTabValue might be best, but I think that  could be too
confusing with the existing ones.

[sky, 2011-06-10 14:58:58]

LGTM

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browser_event_router.cc:220: contents,
foreground);
foreground->active

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browser_event_router.cc:390: bool
foreground) {
foreground -> active

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browser_event_router.h:122: bool
foreground);
foreground -> active

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:196: const TabContents*
contents, bool active) {
each param on its own line.

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:37: bool selected);
selected -> active

[Yoyo Zhou, 2011-06-10 18:04:01]

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browser_event_router.h:122: bool
foreground);
On 2011/06/10 14:58:59, sky wrote:
> foreground -> active

Done here and elsewhere.

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:196: const TabContents*
contents, bool active) {
On 2011/06/10 14:58:59, sky wrote:
> each param on its own line.

Done here and elsewhere.

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/7134035/diff/4009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:37: bool selected);
On 2011/06/10 14:58:59, sky wrote:
> selected -> active

Done.

[commit-bot: I haz the power, 2011-06-10 18:58:28]

Change committed as 88703