Mac: change tab change notifications to pass full TabChangeType.

Mac: change tab change notifications to pass full TabChangeType.

This makes the Mac code better parallel that on other platforms. It will also be needed to make the "glow" animation for (unselected, title-changing) pinned tabs work. (I have a mostly-working patch which does the glow, but it's horribly hacky and to get it to completely work would either involve much more hackiness or significant refactoring. That's why the glow is not part of this CL.)

Also: prevent changes in background tabs (e.g., loading finished) from killing any current bookmark bar animation. Still to do on this front (part of issue 27693): be smarter about bookmark bar updates in the selected tab.

BUG=28154, 27693
TEST=Load lots of (slow-loading) pages in background tabs while pressing Shift-Cmd-B repeatedly; make sure changes in background (unselected) tabs don't cancel the animation.

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

[pink (ping after 24hrs), 2009-12-01 16:35:39]

i still have no idea what TITLE_NOT_LOADING implies. The header comment doesn't
really help.

http://codereview.chromium.org/449056/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/449056/diff/1/2#newcode1106
chrome/browser/cocoa/browser_window_controller.mm:1106: #if 0
off topic, can we just remove these #if 0's now? They're clearly not ever going
to be used.

http://codereview.chromium.org/449056/diff/1/2#newcode1132
chrome/browser/cocoa/browser_window_controller.mm:1132:
windowShim_->UpdateTitleBar();
shouldn't this be indented?

http://codereview.chromium.org/449056/diff/1/2#newcode1137
chrome/browser/cocoa/browser_window_controller.mm:1137: if (change !=
TabStripModelObserver::TITLE_NOT_LOADING)
why doesn't this get hit when the tab is selected in the method above? Why do we
have to add something here?

[viettrungluu, 2009-12-01 16:54:04]

Thanks.

TITLE_NOT_LOADING means (AFAICT) that only the tab's title changed (presumably
through Javascript), without any other change in content/loading status. (So,
e.g., the displayed title has to be updated but not the throbber.)

http://codereview.chromium.org/449056/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/449056/diff/1/2#newcode1106
chrome/browser/cocoa/browser_window_controller.mm:1106: #if 0
On 2009/12/01 16:35:40, pink wrote:
> off topic, can we just remove these #if 0's now? They're clearly not ever
going
> to be used.

Sure. Done.

http://codereview.chromium.org/449056/diff/1/2#newcode1132
chrome/browser/cocoa/browser_window_controller.mm:1132:
windowShim_->UpdateTitleBar();
On 2009/12/01 16:35:40, pink wrote:
> shouldn't this be indented?

Done.

http://codereview.chromium.org/449056/diff/1/2#newcode1137
chrome/browser/cocoa/browser_window_controller.mm:1137: if (change !=
TabStripModelObserver::TITLE_NOT_LOADING)
On 2009/12/01 16:35:40, pink wrote:
> why doesn't this get hit when the tab is selected in the method above? Why do
we
> have to add something here?

If the change in the tab is between the NTP and something else, we may have to
change the bookmark bar (between the floating bar and nothing).

[pink (ping after 24hrs), 2009-12-01 17:09:18]

LGTM with comments.

http://codereview.chromium.org/449056/diff/11/2001
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/449056/diff/11/2001#oldcode1116
chrome/browser/cocoa/browser_window_controller.mm:1116:
toolbar_->SetProfile(newContents->profile());
this worries me, as if there is something that might care about a new
TabContents having a *different* profile and that we'd somehow have to react to
that. Can you ask ben or brettw about this before removing it entirely?

http://codereview.chromium.org/449056/diff/11/2001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/449056/diff/11/2001#newcode1124
chrome/browser/cocoa/browser_window_controller.mm:1124: // isn't just the title
which changed.
Can you comment here about the NTP -> page transition and why the bar may need
updating?

[viettrungluu, 2009-12-01 17:24:47]

http://codereview.chromium.org/449056/diff/11/2001
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/449056/diff/11/2001#oldcode1116
chrome/browser/cocoa/browser_window_controller.mm:1116:
toolbar_->SetProfile(newContents->profile());
On 2009/12/01 17:09:18, pink wrote:
> this worries me, as if there is something that might care about a new
> TabContents having a *different* profile and that we'd somehow have to react
to
> that. Can you ask ben or brettw about this before removing it entirely?

Sure, I'll ask.

From quickly looking through the code, it looks like the location bar cares
about the profile. But can we really have different profiles in different tabs
(in the same browser window)? Is this a legacy capability?

http://codereview.chromium.org/449056/diff/11/2001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/449056/diff/11/2001#newcode1124
chrome/browser/cocoa/browser_window_controller.mm:1124: // isn't just the title
which changed.
On 2009/12/01 17:09:18, pink wrote:
> Can you comment here about the NTP -> page transition and why the bar may need
> updating?

Done.

[viettrungluu, 2009-12-01 17:36:05]

On 2009/12/01 17:24:47, viettrungluu wrote:
> http://codereview.chromium.org/449056/diff/11/2001#oldcode1116
> chrome/browser/cocoa/browser_window_controller.mm:1116:
> toolbar_->SetProfile(newContents->profile());
> On 2009/12/01 17:09:18, pink wrote:
> > this worries me, as if there is something that might care about a new
> > TabContents having a *different* profile and that we'd somehow have to react
> to
> > that. Can you ask ben or brettw about this before removing it entirely?
> 
> Sure, I'll ask.
> 
> From quickly looking through the code, it looks like the location bar cares
> about the profile. But can we really have different profiles in different tabs
> (in the same browser window)? Is this a legacy capability?

So Brett's quick response (to what's in the Windows code) is "that's kind of
dumb" (or something like that), for the reasons I suggested (namely that
profiles are per-browser).