MD Settings: avoid double pref observation on Startup Pages

MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517
Committed: https://crrev.com/4d8ec38c22b0a35ec194cf72d2b567e99397c59e
Cr-Commit-Position: refs/heads/master@{#380561}

[Dan Beam, 2016-03-10 00:51:10]

Description was changed from

==========
MD Settings: avoid double pref registration on Startup Pages

R=dschuyler@chromium.org
BUG=591517
==========

to

==========
MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517
==========

[Dan Beam, 2016-03-10 00:53:28]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-03-10 00:57:42]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2016-03-10 21:49:49]

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.h
(right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.h:31: //
SettingsPageUIHandler
missing period.
Maybe add period on line 34 as well.

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:129:
handlers_[i]->RenderViewReused();
How about this instead?
for (auto handler : handlers_)
  handler->RenderViewReused();

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:133: DCHECK(handler);
Would it be bad to have the same handler added twice?
Is that worth DCHECK'ing?

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:57: // Overridden from
content::WebUIController:
s/:/./

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.h (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.h:31: //
SettingsPageUIHandler:
s/:/./
Maybe on line 35 as well.

[Dan Beam, 2016-03-11 00:07:12]

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.h
(right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/easy_unlock_settings_handler.h:31: //
SettingsPageUIHandler
On 2016/03/10 21:49:49, dschuyler wrote:
> missing period.
> Maybe add period on line 34 as well.

the style guide encourages (but doesn't require) full sentences.  periods are
used to end full sentences.

this is neither a full sentence, nor requires a period, BUT the most common
thing in chrome here is to do:
1) // namespace::Interface:
2) // namespace::Interface implementation.

this code did neither, and my preference is for 1), so I'll make this do 1) for
you (which means i'm actually adding a colon).

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:129:
handlers_[i]->RenderViewReused();
On 2016/03/10 21:49:49, dschuyler wrote:
> How about this instead?
> for (auto handler : handlers_)
>   handler->RenderViewReused();

Done.

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:133: DCHECK(handler);
On 2016/03/10 21:49:49, dschuyler wrote:
> Would it be bad to have the same handler added twice?
> Is that worth DCHECK'ing?

Done.

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:57: // Overridden from
content::WebUIController:
On 2016/03/10 21:49:49, dschuyler wrote:
> s/:/./

following the code below, as per the style guide:

"""
Parting Words

Use common sense and BE CONSISTENT.

If you are editing code, take a few minutes to look at the code around you and
determine its style. If they use spaces around their if clauses, you should,
too. If their comments have little boxes of stars around them, make your
comments have little boxes of stars around them too.
"""

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.h (right):

https://codereview.chromium.org/1776393003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.h:31: //
SettingsPageUIHandler:
On 2016/03/10 21:49:49, dschuyler wrote:
> s/:/./
> Maybe on line 35 as well.

Acknowledged.

[michaelpg, 2016-03-11 01:16:55]

not a review

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:31: virtual void
RenderViewReused() {}
plz comment what this is

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:73: // Weak references; all
|handlers_| are owned by |web_ui()|.
asking out of ignorance: web_ui() exists here from ctor to dtor?

(also added question to
https://docs.google.com/document/d/1ubLoYnTSyjCMFCpy1tPy6g1RAm5FajmbuQygjqN3a...)

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.h (right):

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.h:43: class PeopleHandler :
public SettingsPageUIHandler,
why?

[dschuyler, 2016-03-11 02:33:20]

lgtm

[Dan Beam, 2016-03-11 05:39:06]

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:31: virtual void
RenderViewReused() {}
On 2016/03/11 01:16:54, michaelpg wrote:
> plz comment what this is

Done.

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.h:73: // Weak references; all
|handlers_| are owned by |web_ui()|.
On 2016/03/11 01:16:54, michaelpg wrote:
> asking out of ignorance: web_ui() exists here from ctor to dtor?
> 
> (also added question to
>
https://docs.google.com/document/d/1ubLoYnTSyjCMFCpy1tPy6g1RAm5FajmbuQygjqN3a...)

yes

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/people_handler.h (right):

https://codereview.chromium.org/1776393003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/people_handler.h:43: class PeopleHandler :
public SettingsPageUIHandler,
On 2016/03/11 01:16:54, michaelpg wrote:
> why?

I restricted AddSettingsPageUIHandler() to take a SettingsPageUIHandler* instead
of a content::WebUIMessageHandler* so that every handler has a
RenderViewReused() method

[Dan Beam, 2016-03-11 06:37:40]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-03-11 06:37:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1776393003/#ps100001
(title: "doc comment")

[commit-bot: I haz the power, 2016-03-11 06:38:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1776393003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1776393003/100001

[commit-bot: I haz the power, 2016-03-11 06:53:07]

Description was changed from

==========
MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517
==========

to

==========
MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517
==========

[commit-bot: I haz the power, 2016-03-11 06:53:08]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-03-11 06:54:57]

Description was changed from

==========
MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517
==========

to

==========
MD Settings: avoid double pref observation on Startup Pages

R=dschuyler@chromium.org
BUG=591517

Committed: https://crrev.com/4d8ec38c22b0a35ec194cf72d2b567e99397c59e
Cr-Commit-Position: refs/heads/master@{#380561}
==========

[commit-bot: I haz the power, 2016-03-11 06:54:58]

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