Remove PrefObserver usages, batch 6.

Remove PrefObserver usages, batch 6.

TBR=ben@chromium.org
BUG=155525
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167973

[Mattias Nissler (ping if slow), 2012-11-15 10:01:31]

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

https://codereview.chromium.org/11362250/diff/1/chrome/browser/ui/prefs/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:598: void
PrefsTabHelper::OnPreferenceChanged(const std::string& pref_name_in) {
should probably split into separate functions with better names.

[Jói, 2012-11-15 13:45:42]

PTAL

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

https://codereview.chromium.org/11362250/diff/1/chrome/browser/ui/prefs/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:598: void
PrefsTabHelper::OnPreferenceChanged(const std::string& pref_name_in) {
On 2012/11/15 10:01:31, Mattias Nissler wrote:
> should probably split into separate functions with better names.

Done.

[Mattias Nissler (ping if slow), 2012-11-15 14:56:22]

https://codereview.chromium.org/11362250/diff/5001/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11362250/diff/5001/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:604: StartsWithASCII(pref_name_in,
"webkit.webprefs.", true)) {
I would even go as far as dropping this check and binding to OnWebPrefChanged()
directly, the test in the old code is just the pref-name-switch pattern
boilerplate I guess. Even if the check is considered useful, how about putting
the DCHECK(StartsWithASCII()) at the registration in line 395?

Maybe we should check with an OWNER :)

[Jói, 2012-11-15 16:27:28]

Excellent point, that will make the production code more efficient.
Will switch to that and upload a new snapshot.  I agree this is enough
change that we should loop in an OWNER for that code before comitting.

Cheers,
Jói


On Thu, Nov 15, 2012 at 2:56 PM,  <mnissler@chromium.org> wrote:
>
>
https://codereview.chromium.org/11362250/diff/5001/chrome/browser/ui/prefs/pr...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
>
>
https://codereview.chromium.org/11362250/diff/5001/chrome/browser/ui/prefs/pr...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:604:
> StartsWithASCII(pref_name_in, "webkit.webprefs.", true)) {
> I would even go as far as dropping this check and binding to
> OnWebPrefChanged() directly, the test in the old code is just the
> pref-name-switch pattern boilerplate I guess. Even if the check is
> considered useful, how about putting the DCHECK(StartsWithASCII()) at
> the registration in line 395?
>
> Maybe we should check with an OWNER :)
>
> https://codereview.chromium.org/11362250/

[Jói, 2012-11-15 16:39:30]

PTAL.

Note that you yourself are an OWNER for
chrome/browser/ui/prefs/prefs_tab_helper.h and .cc :) so I'm not sure we need to
add another.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-15 16:43:47]

LGTM then ;)

[commit-bot: I haz the power, 2012-11-15 16:48:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11362250/11001

[commit-bot: I haz the power, 2012-11-15 16:49:07]

Presubmit check for 11362250-11001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/browser/ui/webui/options
    chrome/browser/ui/cocoa/toolbar

Presubmit checks took 4.2s to calculate.

[Jói, 2012-11-15 16:50:31]

TBR=ben@chromium.org for chrome/browser/ui/OWNERS rubber stamp.  This is for a
Prefs API change, already reviewed by mnissler@.

Cheers,
Jói

[commit-bot: I haz the power, 2012-11-15 16:51:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11362250/11001

[commit-bot: I haz the power, 2012-11-15 16:53:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11362250/3002

[commit-bot: I haz the power, 2012-11-15 16:53:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11362250/3002

[Ben Goodger (Google), 2012-11-15 17:02:55]

lgtm

[commit-bot: I haz the power, 2012-11-15 18:47:33]

Change committed as 167973