[MD settings] pulling in chrome.send for using current pages for on startup pages]

[MD settings] pulling in chrome.send for using current pages for on startup pages.

This will be used in a near-future CL to set the On Startup URL list settings.

BUG=425627
Committed: https://crrev.com/87b67c7e0ad9e29f623d7411eae91f5950e63d73
Cr-Commit-Position: refs/heads/master@{#347026}

[dschuyler, 2015-09-01 23:36:51]

dschuyler@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-09-02 07:31:23]

this generally looks good to me, but where is the code that uses this handler?

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/startup_pages_handler.cc (right):

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.cc:15:
startup_custom_pages_table_model_.reset(
make this just a member rather than a scoped_ptr<>

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.cc:26: if
(Profile::FromWebUI(web_ui())->IsOffTheRecord())
should this be a DCHECK() instead?

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/startup_pages_handler.h (right):

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:1: // Copyright (c)
2012-2015 The Chromium Authors. All rights reserved.
only 2015, no (c)

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:20: explicit
StartupPagesHandler(content::WebUI* webui);
where does content::WebUI come from?

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:26: //
ui::TableModelObserver implementation.
just

  // ui::TableModelObserver:

the " implementation." is just fat and redundant to me

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:34: void
SetStartupPagesToCurrentPages(const base::ListValue* args);
forward declare base::ListValue

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:38:
DISALLOW_COPY_AND_ASSIGN(StartupPagesHandler);
#include "base/macros.h"

[dschuyler, 2015-09-02 19:38:21]

The code is used in another pending change I have, but I'm trying to keep the CL
small-ish.  To give a little more context to connect it into chrome, I've pulled
in the md_settings_ui.cc file from the future change I'm planning.  The real
user of this code will be JavaScript calling chrome.send.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/startup_pages_handler.cc (right):

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.cc:15:
startup_custom_pages_table_model_.reset(
On 2015/09/02 07:31:23, Dan Beam wrote:
> make this just a member rather than a scoped_ptr<>

Done.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.cc:26: if
(Profile::FromWebUI(web_ui())->IsOffTheRecord())
On 2015/09/02 07:31:23, Dan Beam wrote:
> should this be a DCHECK() instead?

Done.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/startup_pages_handler.h (right):

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:1: // Copyright (c)
2012-2015 The Chromium Authors. All rights reserved.
On 2015/09/02 07:31:23, Dan Beam wrote:
> only 2015, no (c)

Done.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:20: explicit
StartupPagesHandler(content::WebUI* webui);
On 2015/09/02 07:31:23, Dan Beam wrote:
> where does content::WebUI come from?

I'm not sure if you're asking for the file it's declared in, defined in, whether
there should be a forward declaration or where the caller passes it into the
ctor.  It's declared in content/public/browser/web_ui.h.
Also, I've added a forward declaration.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:26: //
ui::TableModelObserver implementation.
On 2015/09/02 07:31:23, Dan Beam wrote:
> just
> 
>   // ui::TableModelObserver:
> 
> the " implementation." is just fat and redundant to me

Done.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:34: void
SetStartupPagesToCurrentPages(const base::ListValue* args);
On 2015/09/02 07:31:23, Dan Beam wrote:
> forward declare base::ListValue

Done.

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.h:38:
DISALLOW_COPY_AND_ASSIGN(StartupPagesHandler);
On 2015/09/02 07:31:23, Dan Beam wrote:
> #include "base/macros.h"

Done.

[Dan Beam, 2015-09-02 19:43:52]

lgtm

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc (right):

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:33:
base::DictionaryValue* entry = new base::DictionaryValue();
nit: use scoped_ptr<> to make ownership more explicit and to avoid memory leaks
(if there's a continue or break in the middle of the loop, for instance)

scoped_ptr<base::DictionaryValue> entry(new base::DictionaryValue);

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.cc:39:
startup_pages.Append(entry);
Append(entry.release())

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

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.h:24: // Chrome
browser settings page UI handler.
^ fix

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.h:31: //
OptionsPageUIHandler.
// OptionsPageUIHandler:

https://codereview.chromium.org/1323443006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_startup_pages_handler.h:34: //
ui::TableModelObserver.
// ui::TableModelObserver:

[dschuyler, 2015-09-02 20:39:54]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-09-02 20:39:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1323443006/#ps40001
(title: "nit changes")

[commit-bot: I haz the power, 2015-09-02 20:40:20]

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

[commit-bot: I haz the power, 2015-09-02 21:18:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-02 21:19:17]

Patchset 3 (id:??) landed as
https://crrev.com/87b67c7e0ad9e29f623d7411eae91f5950e63d73
Cr-Commit-Position: refs/heads/master@{#347026}

[Dan Beam, 2015-12-02 02:07:20]

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/startup_pages_handler.cc (right):

https://codereview.chromium.org/1323443006/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/startup_pages_handler.cc:26: if
(Profile::FromWebUI(web_ui())->IsOffTheRecord())
On 2015/09/02 07:31:23, Dan Beam wrote:
> should this be a DCHECK() instead?

no it should not!