More.

Profiles: Three small changes

- Change how BOOKMARK_BAR_VISIBILITY_PREF_CHANGED is handled because it broke interaction w/ incognito windows.
- Make SIDEBAR_CHANGED listen only to a certain SidebarManager instance.
- Mark COOKIE_CHANGED as ok to listen to AllSources().

BUG=87457, 92506
TEST=none

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

[Elliot Glaysher, 2011-08-10 20:40:03]

pkasting: review
mirandac: FYI when you come back; we can't just follow a single profile because
we then won't receive notifications for the Ingocnito/vice versa.

[Peter Kasting, 2011-08-10 21:03:21]

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:279:
NotificationService::AllBrowserContextsAndSources());
Nit: Might want to comment somewhere that we need this because we want a normal
profile and its incognito partner to stay in sync regarding this pref.

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:58:
NotificationService::AllSources());
Don't you want to change this (and the observer code) similarly to browser.cc?

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:60:
Source<SidebarManager>(SidebarManager::GetInstance()));
Shouldn't the sidebar state be synced between normal and incognito profiles?  Or
am I mistaken about what the sidebar is...

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/gtk/global_me...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/gtk/global_me...
chrome/browser/ui/gtk/global_menu_bar.cc:219:
NotificationService::AllSources());
Nit: AllBrowserContextsAndSources()?

Might also want the comment I suggested in browser.cc.

[Elliot Glaysher, 2011-08-10 21:19:22]

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:58:
NotificationService::AllSources());
On 2011/08/10 21:03:21, Peter Kasting wrote:
> Don't you want to change this (and the observer code) similarly to browser.cc?

I'm doing this in a per-notification way instead of per-file. (See our tracking
spreadsheet
https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ar4V_eAZl8_SdDB2THV...
)

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:60:
Source<SidebarManager>(SidebarManager::GetInstance()));
On 2011/08/10 21:03:21, Peter Kasting wrote:
> Shouldn't the sidebar state be synced between normal and incognito profiles? 
Or
> am I mistaken about what the sidebar is...

The SidebarManager is currently a Singleton. It maps a TabContents to a Sidebar
object. This is just a case where it's easy to remove the AllSources() call and
listen directly on the single object that we're interested in.

(I believe this is an extensions feature. This is an extension API, not the side
tab stuff.)

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/gtk/global_me...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/7610003/diff/1/chrome/browser/ui/gtk/global_me...
chrome/browser/ui/gtk/global_menu_bar.cc:219:
NotificationService::AllSources());
On 2011/08/10 21:03:21, Peter Kasting wrote:
> Nit: AllBrowserContextsAndSources()?
> 
> Might also want the comment I suggested in browser.cc.

Done.

[Peter Kasting, 2011-08-10 21:30:14]

LGTM

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:277: // Need to stay in sync between our incognito
and our normal profile.
Nit: "We listen to all notification sources because the bookmark bar state needs
to stay in sync between the incognito and normal profiles."?

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:55: // This pref applies to all
windows, so all must watch for it.
Nit: Maybe use same comment as browser.cc

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/gtk/global...
File chrome/browser/ui/gtk/global_menu_bar.cc (right):

http://codereview.chromium.org/7610003/diff/3003/chrome/browser/ui/gtk/global...
chrome/browser/ui/gtk/global_menu_bar.cc:217: // need to listen to all profiles
because of normal profile/incognito issues.
Nit: "issues" is vague, maybe use same comment as browser.cc

[Miranda Callahan, 2011-08-11 06:57:59]

On 2011/08/10 20:40:03, Elliot Glaysher wrote:
> pkasting: review
> mirandac: FYI when you come back; we can't just follow a single profile
because
> we then won't receive notifications for the Ingocnito/vice versa.

Makes sense; I imagine we will see this pattern in many other places. LGTM.