Watch the preferences for changes per profile instead of per tab.

Watch the preferences for changes per profile instead of per tab.

PrefsTabHelper was watching > 1K prefs so that it could update the renderer side data structures when they change. When session restore was used and it had 50 tabs, this would take almost a second on a Z620. Switch this to watching these prefs once per profile instead.

BUG=452693
Committed: https://crrev.com/d9bb3b780aad1d1672b6574069ae8fab35319bc7
Cr-Commit-Position: refs/heads/master@{#313573}

[jam, 2015-01-28 05:03:46]

jam@chromium.org changed reviewers:
+ bauerb@chromium.org

[jam, 2015-01-28 17:32:20]

jam@chromium.org changed reviewers:
+ brettw@chromium.org
- bauerb@chromium.org

[brettw, 2015-01-28 19:13:29]

lgtm

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:403: for (auto it =
helpers_.begin(); it != helpers_.end(); ++it)
for (const auto& helper : helpers_)
  helper->UpdateRendererPreferences();

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:408: for (auto it =
helpers_.begin(); it != helpers_.end(); ++it)
Ditto

[jam, 2015-01-28 19:29:11]

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:403: for (auto it =
helpers_.begin(); it != helpers_.end(); ++it)
On 2015/01/28 19:13:29, brettw wrote:
> for (const auto& helper : helpers_)
>   helper->UpdateRendererPreferences();

Done.

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:408: for (auto it =
helpers_.begin(); it != helpers_.end(); ++it)
On 2015/01/28 19:13:29, brettw wrote:
> Ditto

Done.

[jam, 2015-01-28 19:29:24]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-01-28 19:30:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878363002/20001

[commit-bot: I haz the power, 2015-01-28 20:32:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-28 20:34:27]

Patchset 2 (id:??) landed as
https://crrev.com/d9bb3b780aad1d1672b6574069ae8fab35319bc7
Cr-Commit-Position: refs/heads/master@{#313573}

[Bernhard Bauer, 2015-01-28 23:35:52]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-01-28 23:35:53]

FTR, I was OOO today, so I couldn't do the whole review, but in general I'm
available over chat if I'm not responding to reviews.

https://codereview.chromium.org/878363002/diff/1/chrome/browser/profiles/chro...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/878363002/diff/1/chrome/browser/profiles/chro...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:237:
PrefsTabHelper::GetServiceInstance();
This isn't quite idiomatic; usually we expose the factory and call GetInstance()
directly. I'm personally not opposed to not exposing the factory, but seeing
this as it is right now still makes me do a double-take. Could we name the
method something like EnsureKeyedServiceInstanceCreated()? That way we
explicitly mention a KeyedService (as opposed to a generic "service", which
could be a lot of things), and if this method doesn't actually return anything,
naming it Get... is a bit weird.

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:340: // http://crbug.com/452693
Nit: I don't think we even need to mention the bug here. The comment already
does a good job of explaining the rationale behind this class, and in the long
term, historic context is not going to be very relevant.

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:431: PrefWatcherFactory() :
BrowserContextKeyedServiceFactory(
This looks somewhat weirdly formatted. Does breaking before the colon work?

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.h (right):

https://codereview.chromium.org/878363002/diff/1/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.h:45: friend class PrefWatcher;
Can we move PrefWatcher into this class? Then we wouldn't need to friend it, and
we would avoid potential naming collisions due to PrefWatcher being in the
global namespace.