Settings Rewrite: Fork SyncSetupHandler for MD-Settings

Settings Rewrite: Fork SyncSetupHandler for MD-Settings

This patch copies the SyncSetupHandler used in Options to the MD-Settings directory as well.

It's basically a straight-copy with the options-specific code removed.

BUG=537405
Committed: https://crrev.com/22664b9a04bc8972fd12370c484a3dad9aecffc5
Cr-Commit-Position: refs/heads/master@{#357165}

[tommycli, 2015-10-27 23:32:38]

Description was changed from

==========
Settings Rewrite: Move SyncSetupHandler back out to webui level.

Previously SyncSetupHandler was only used by (old) Options.
Now it is used by both (old) Options and (new) MD-Settings.
This is why I am moving it out one level to the webui directory.

BUG=537405
==========

to

==========
Settings Rewrite: Move SyncSetupHandler back out to webui level.

Previously SyncSetupHandler was only used by (old) Options.
Now it is used by both (old) Options and (new) MD-Settings.
This is why I am moving it out one level to the webui directory.

This patch also breaks off the Options-specific code and puts it into a
SyncOptionsHandler that delegates to an inner (owned) SyncSetupHandler.

BUG=537405
==========

[tommycli, 2015-10-28 00:01:20]

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

[tommycli, 2015-10-28 00:01:21]

dbeam: PTAL, this moves the SyncSetupHandler up a level, while leaving the
Options-specific stuff behind in an options-specific wrapper.

[Dan Beam, 2015-10-28 17:49:48]

why is this better than just moving the file?

[tommycli, 2015-10-28 18:06:52]

On 2015/10/28 17:49:48, Dan Beam wrote:
> why is this better than just moving the file?

I could just do that, but then webui/sync_setup_handler.h would be a subclass of
options::OptionsPageUIHandler. It would contain options-specific code.

[tommycli, 2015-10-28 18:17:01]

On 2015/10/28 18:06:52, tommycli wrote:
> On 2015/10/28 17:49:48, Dan Beam wrote:
> > why is this better than just moving the file?
> 
> I could just do that, but then webui/sync_setup_handler.h would be a subclass
of
> options::OptionsPageUIHandler. It would contain options-specific code.

Patchset 1 is what a straight-move looks like btw, that's what I originally did.

[Dan Beam, 2015-10-28 20:49:47]

On 2015/10/28 18:06:52, tommycli wrote:
> On 2015/10/28 17:49:48, Dan Beam wrote:
> > why is this better than just moving the file?
> 
> I could just do that, but then webui/sync_setup_handler.h would be a subclass
of
> options::OptionsPageUIHandler. It would contain options-specific code.

so we talked about this IRL a bit:
1) yes, it'd be weird to have an options-specific class in c/b/ui/webui/
2) it'd be mildly weird to do concrete inheritance
3) yes, SettingsPageUIHandler is a little different than OptionsPageUIHandler

if your explorations don't change much, I can review the wrapper.

Maybe we could just put an interface in c/b/ui/webui/ and implement it from both
options/ and settings/?

[tommycli, 2015-10-28 21:18:20]

Patchset #3 (id:40001) has been deleted

[tommycli, 2015-10-28 21:29:06]

Patchset #3 (id:60001) has been deleted

[tommycli, 2015-10-28 22:31:34]

dbeam: Alright here's another revision.

I looked at the callsites for set_web_ui, and in nearly all cases, we could have
just passed it in during construction.

I marked that fn as deprecated. In the future, we can hopefully get rid of all
set_web_ui and make it immutable per Handler.

[Dan Beam, 2015-10-29 01:47:20]

I guess I agree with your split a little more now

ultimately, if we wanted to move toward a world where all WebUIMessageHandlers
took a WebUI* as a ctor arg, might be useful to templatize, but that'd probably
make it harder for all those WebUIMessageHandlers that already take different
types.  this is currently already possible when they're externally constructed.

btw, you'll need estade@ or jhawkins@ for content/browser/webui/OWNERS (I'll
look into being in that OWNERS file as well)

https://codereview.chromium.org/1421893005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (left):

https://codereview.chromium.org/1421893005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:156:
~TestingSyncSetupHandler() override { set_web_ui(NULL); }
do we know why this was necessary?

https://codereview.chromium.org/1421893005/diff/140001/content/browser/webui/...
File content/browser/webui/web_ui_impl.cc (right):

https://codereview.chromium.org/1421893005/diff/140001/content/browser/webui/...
content/browser/webui/web_ui_impl.cc:225: void
WebUIImpl::AddMessageHandler(WebUIMessageHandler* handler) {
this would be cooler, IMO:

template<class T>
T* WebUIImpl::CreateMessageHandler() {
  T* handler = new T(this);
  handlers_.push_back(handler);
  handler->RegisterMessages();
  return handler;
}

web_ui->CreateMessageHandler<SomeSpecificHandler>();

[tommycli, 2015-10-29 17:34:14]

dbeam: see below, thanks!

https://codereview.chromium.org/1421893005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (left):

https://codereview.chromium.org/1421893005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:156:
~TestingSyncSetupHandler() override { set_web_ui(NULL); }
On 2015/10/29 01:47:20, Dan Beam wrote:
> do we know why this was necessary?

Yeah, this is necessary because of:
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...

It's not optimal, but probably better than adding a separate member variable
(for now).

https://codereview.chromium.org/1421893005/diff/140001/content/browser/webui/...
File content/browser/webui/web_ui_impl.cc (right):

https://codereview.chromium.org/1421893005/diff/140001/content/browser/webui/...
content/browser/webui/web_ui_impl.cc:225: void
WebUIImpl::AddMessageHandler(WebUIMessageHandler* handler) {
On 2015/10/29 01:47:20, Dan Beam wrote:
> this would be cooler, IMO:
> 
> template<class T>
> T* WebUIImpl::CreateMessageHandler() {
>   T* handler = new T(this);
>   handlers_.push_back(handler);
>   handler->RegisterMessages();
>   return handler;
> }
> 
> web_ui->CreateMessageHandler<SomeSpecificHandler>();

Agreed. Unfortunately the vast majority of the 50-odd handlers currently have
only the T() constructor and rely on the T.set_web_ui method to set the web ui.

I didn't want to add the T(WebUI*) constructor on all of them right now.

[tommycli, 2015-10-29 17:49:03]

tommycli@chromium.org changed reviewers:
+ jhawkins@chromium.org

[tommycli, 2015-10-29 17:49:04]

jhawkins: PTAL content/browser/webui, thanks!

[James Hawkins, 2015-10-29 17:53:30]

I'm not totally comfortable with the location of this refactored code.  Yes,
it's currently shared by two subtrees, but that is just because we can't get rid
of the old code base yet.  In the end this code should ultimately live in
settings/.  What is the plan?

[tommycli, 2015-10-29 17:56:00]

On 2015/10/29 17:53:30, James Hawkins wrote:
> I'm not totally comfortable with the location of this refactored code.  Yes,
> it's currently shared by two subtrees, but that is just because we can't get
rid
> of the old code base yet.  In the end this code should ultimately live in
> settings/.  What is the plan?

Yep, the plan is:

1. Divorce the shared portions of the code from the Options-specific wrapper
(this CL).
2. Avoid adding any Settings-specific code, if possible. The Settings code
should just use the base class.
3. If/when we get rid of Options entirely, delete the Options-specific wrapper
and move the base class to the settings/ directory.

[tommycli, 2015-10-29 18:34:30]

On 2015/10/29 17:56:00, tommycli wrote:
> On 2015/10/29 17:53:30, James Hawkins wrote:
> > I'm not totally comfortable with the location of this refactored code.  Yes,
> > it's currently shared by two subtrees, but that is just because we can't get
> rid
> > of the old code base yet.  In the end this code should ultimately live in
> > settings/.  What is the plan?
> 
> Yep, the plan is:
> 
> 1. Divorce the shared portions of the code from the Options-specific wrapper
> (this CL).
> 2. Avoid adding any Settings-specific code, if possible. The Settings code
> should just use the base class.
> 3. If/when we get rid of Options entirely, delete the Options-specific wrapper
> and move the base class to the settings/ directory.

The main alternative to this is to make a copy of SyncSetupHandler into
chrome/browser/ui/webui/settings. Not the worst, just a bunch of copied code.

[James Hawkins, 2015-10-29 18:36:57]

On 2015/10/29 18:34:30, tommycli wrote:
> On 2015/10/29 17:56:00, tommycli wrote:
> > On 2015/10/29 17:53:30, James Hawkins wrote:
> > > I'm not totally comfortable with the location of this refactored code. 
Yes,
> > > it's currently shared by two subtrees, but that is just because we can't
get
> > rid
> > > of the old code base yet.  In the end this code should ultimately live in
> > > settings/.  What is the plan?
> > 
> > Yep, the plan is:
> > 
> > 1. Divorce the shared portions of the code from the Options-specific wrapper
> > (this CL).
> > 2. Avoid adding any Settings-specific code, if possible. The Settings code
> > should just use the base class.
> > 3. If/when we get rid of Options entirely, delete the Options-specific
wrapper
> > and move the base class to the settings/ directory.
> 
> The main alternative to this is to make a copy of SyncSetupHandler into
> chrome/browser/ui/webui/settings. Not the worst, just a bunch of copied code.

What's the benefit of sharing it?  I don't have a problem with copied code as
long as the original copy definitely gets deleted at some point.

[tommycli, 2015-10-29 18:50:16]

On 2015/10/29 18:36:57, James Hawkins wrote:
> On 2015/10/29 18:34:30, tommycli wrote:
> > On 2015/10/29 17:56:00, tommycli wrote:
> > > On 2015/10/29 17:53:30, James Hawkins wrote:
> > > > I'm not totally comfortable with the location of this refactored code. 
> Yes,
> > > > it's currently shared by two subtrees, but that is just because we can't
> get
> > > rid
> > > > of the old code base yet.  In the end this code should ultimately live
in
> > > > settings/.  What is the plan?
> > > 
> > > Yep, the plan is:
> > > 
> > > 1. Divorce the shared portions of the code from the Options-specific
wrapper
> > > (this CL).
> > > 2. Avoid adding any Settings-specific code, if possible. The Settings code
> > > should just use the base class.
> > > 3. If/when we get rid of Options entirely, delete the Options-specific
> wrapper
> > > and move the base class to the settings/ directory.
> > 
> > The main alternative to this is to make a copy of SyncSetupHandler into
> > chrome/browser/ui/webui/settings. Not the worst, just a bunch of copied
code.
> 
> What's the benefit of sharing it?  I don't have a problem with copied code as
> long as the original copy definitely gets deleted at some point.

Main benefit is that there aren't two copies that can get out of sync. This
handler comes with a unit test, so there would be two copies of the unit test
too. Seems like doubling the maintenance burden.

If you two still both favor a copy, I'm good with that also, just let me know.

[Dan Beam, 2015-10-29 18:51:42]

On 2015/10/29 17:53:30, James Hawkins wrote:
> I'm not totally comfortable with the location of this refactored code.  Yes,
> it's currently shared by two subtrees, but that is just because we can't get
rid
> of the old code base yet.  In the end this code should ultimately live in
> settings/.  What is the plan?

currently:
  - options/sync_setup_handler.{h,cc}

proposed (while both options/ and settings/ need to exist):
  - sync_setup_handler.{h,cc} -> used by both
  - options/sync_options_handler.{h,cc} -> options-specific

when options/ is removed:
  - settings/sync_setup_handler.{h,cc}

[James Hawkins, 2015-10-29 18:51:56]

On 2015/10/29 18:50:16, tommycli wrote:
> On 2015/10/29 18:36:57, James Hawkins wrote:
> > On 2015/10/29 18:34:30, tommycli wrote:
> > > On 2015/10/29 17:56:00, tommycli wrote:
> > > > On 2015/10/29 17:53:30, James Hawkins wrote:
> > > > > I'm not totally comfortable with the location of this refactored code.

> > Yes,
> > > > > it's currently shared by two subtrees, but that is just because we
can't
> > get
> > > > rid
> > > > > of the old code base yet.  In the end this code should ultimately live
> in
> > > > > settings/.  What is the plan?
> > > > 
> > > > Yep, the plan is:
> > > > 
> > > > 1. Divorce the shared portions of the code from the Options-specific
> wrapper
> > > > (this CL).
> > > > 2. Avoid adding any Settings-specific code, if possible. The Settings
code
> > > > should just use the base class.
> > > > 3. If/when we get rid of Options entirely, delete the Options-specific
> > wrapper
> > > > and move the base class to the settings/ directory.
> > > 
> > > The main alternative to this is to make a copy of SyncSetupHandler into
> > > chrome/browser/ui/webui/settings. Not the worst, just a bunch of copied
> code.
> > 
> > What's the benefit of sharing it?  I don't have a problem with copied code
as
> > long as the original copy definitely gets deleted at some point.
> 
> Main benefit is that there aren't two copies that can get out of sync. This
> handler comes with a unit test, so there would be two copies of the unit test
> too. Seems like doubling the maintenance burden.
> 
> If you two still both favor a copy, I'm good with that also, just let me know.

Options is in super maintenance mode, but I realize the dependency on Sync here
means there is a chance some change could be required; however, I don't believe
it warrants the cost of the proposed refactoring.  I prefer a copy.

[tommycli, 2015-10-29 18:58:31]

On 2015/10/29 18:51:56, James Hawkins wrote:
> On 2015/10/29 18:50:16, tommycli wrote:
> > On 2015/10/29 18:36:57, James Hawkins wrote:
> > > On 2015/10/29 18:34:30, tommycli wrote:
> > > > On 2015/10/29 17:56:00, tommycli wrote:
> > > > > On 2015/10/29 17:53:30, James Hawkins wrote:
> > > > > > I'm not totally comfortable with the location of this refactored
code.
> 
> > > Yes,
> > > > > > it's currently shared by two subtrees, but that is just because we
> can't
> > > get
> > > > > rid
> > > > > > of the old code base yet.  In the end this code should ultimately
live
> > in
> > > > > > settings/.  What is the plan?
> > > > > 
> > > > > Yep, the plan is:
> > > > > 
> > > > > 1. Divorce the shared portions of the code from the Options-specific
> > wrapper
> > > > > (this CL).
> > > > > 2. Avoid adding any Settings-specific code, if possible. The Settings
> code
> > > > > should just use the base class.
> > > > > 3. If/when we get rid of Options entirely, delete the Options-specific
> > > wrapper
> > > > > and move the base class to the settings/ directory.
> > > > 
> > > > The main alternative to this is to make a copy of SyncSetupHandler into
> > > > chrome/browser/ui/webui/settings. Not the worst, just a bunch of copied
> > code.
> > > 
> > > What's the benefit of sharing it?  I don't have a problem with copied code
> as
> > > long as the original copy definitely gets deleted at some point.
> > 
> > Main benefit is that there aren't two copies that can get out of sync. This
> > handler comes with a unit test, so there would be two copies of the unit
test
> > too. Seems like doubling the maintenance burden.
> > 
> > If you two still both favor a copy, I'm good with that also, just let me
know.
> 
> Options is in super maintenance mode, but I realize the dependency on Sync
here
> means there is a chance some change could be required; however, I don't
believe
> it warrants the cost of the proposed refactoring.  I prefer a copy.

By proposed refactoring - do you mean the delegation business? or the changes to
WebUIMessageHandler?

I think the delegation business is a little annoying, but the changes to
WebUIMessageHandler are a good idea regardless of whether we copy vs. delegate.

[James Hawkins, 2015-10-29 18:59:40]

On 2015/10/29 18:58:31, tommycli wrote:
> On 2015/10/29 18:51:56, James Hawkins wrote:
> > On 2015/10/29 18:50:16, tommycli wrote:
> > > On 2015/10/29 18:36:57, James Hawkins wrote:
> > > > On 2015/10/29 18:34:30, tommycli wrote:
> > > > > On 2015/10/29 17:56:00, tommycli wrote:
> > > > > > On 2015/10/29 17:53:30, James Hawkins wrote:
> > > > > > > I'm not totally comfortable with the location of this refactored
> code.
> > 
> > > > Yes,
> > > > > > > it's currently shared by two subtrees, but that is just because we
> > can't
> > > > get
> > > > > > rid
> > > > > > > of the old code base yet.  In the end this code should ultimately
> live
> > > in
> > > > > > > settings/.  What is the plan?
> > > > > > 
> > > > > > Yep, the plan is:
> > > > > > 
> > > > > > 1. Divorce the shared portions of the code from the Options-specific
> > > wrapper
> > > > > > (this CL).
> > > > > > 2. Avoid adding any Settings-specific code, if possible. The
Settings
> > code
> > > > > > should just use the base class.
> > > > > > 3. If/when we get rid of Options entirely, delete the
Options-specific
> > > > wrapper
> > > > > > and move the base class to the settings/ directory.
> > > > > 
> > > > > The main alternative to this is to make a copy of SyncSetupHandler
into
> > > > > chrome/browser/ui/webui/settings. Not the worst, just a bunch of
copied
> > > code.
> > > > 
> > > > What's the benefit of sharing it?  I don't have a problem with copied
code
> > as
> > > > long as the original copy definitely gets deleted at some point.
> > > 
> > > Main benefit is that there aren't two copies that can get out of sync.
This
> > > handler comes with a unit test, so there would be two copies of the unit
> test
> > > too. Seems like doubling the maintenance burden.
> > > 
> > > If you two still both favor a copy, I'm good with that also, just let me
> know.
> > 
> > Options is in super maintenance mode, but I realize the dependency on Sync
> here
> > means there is a chance some change could be required; however, I don't
> believe
> > it warrants the cost of the proposed refactoring.  I prefer a copy.
> 
> By proposed refactoring - do you mean the delegation business? or the changes
to
> WebUIMessageHandler?
> 
> I think the delegation business is a little annoying, but the changes to
> WebUIMessageHandler are a good idea regardless of whether we copy vs.
delegate.

By proposed refactoring I mean the shared SyncSetupHandler.

[tommycli, 2015-10-29 20:51:15]

dbeam, jhawkins: Latest patchset is the handler-copy version. PTAL and let me
know if you guys like that better.

[James Hawkins, 2015-10-29 22:22:20]

lgtm

[tommycli, 2015-10-30 18:11:35]

jhawkins: thanks!

dbeam: any objections?

[tommycli, 2015-10-30 18:11:49]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 18:14:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421893005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421893005/220001

[commit-bot: I haz the power, 2015-10-30 18:44:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 18:44:46]

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-10-30 18:58:51]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-10-30 18:58:57]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2015-10-30 18:59:47]

Description was changed from

==========
Settings Rewrite: Move SyncSetupHandler back out to webui level.

Previously SyncSetupHandler was only used by (old) Options.
Now it is used by both (old) Options and (new) MD-Settings.
This is why I am moving it out one level to the webui directory.

This patch also breaks off the Options-specific code and puts it into a
SyncOptionsHandler that delegates to an inner (owned) SyncSetupHandler.

BUG=537405
==========

to

==========
Settings Rewrite: Fork SyncSetupHandler for MD-Settings

This patch copies the SyncSetupHandler used in Options to the MD-Settings
directory as well.

It's basically a straight-copy with the options-specific code removed.

BUG=537405
==========

[tommycli, 2015-10-30 18:59:52]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-10-30 19:02:55]

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

[commit-bot: I haz the power, 2015-10-30 19:17:51]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2015-10-30 19:18:38]

Patchset 10 (id:??) landed as
https://crrev.com/22664b9a04bc8972fd12370c484a3dad9aecffc5
Cr-Commit-Position: refs/heads/master@{#357165}

[Dan Beam, 2015-10-30 20:50:31]

lgtm