New remoting setup flow.

New remoting setup flow.

BUG=67218
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70238

[Sergey Ulanov, 2010-12-21 02:21:16]

This is base code for remoting setup flow. The new remoting::SetupFlow will
replace RemotingSetupFlow. The steps for host registration and service startup
are not implemented yet, I'm working on it.

[Alpha Left Google, 2010-12-21 21:52:03]

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:90: BrowserThread::PostTask(
This should only be done once.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:127:
handlers->push_back(const_cast<SetupFlow*>(this));
Give the list of SetupFlowStep as handlers. This way this class will be agnostic
about what message to handle.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:176:
dom_ui_->RegisterMessageCallback("SubmitAuth",
Should let SetupFlowStep to register their own messages.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.h (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:31: virtual void Start(SetupFlow* flow,
DoneCallback* done_callback) = 0;
When is done_callback called?

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:65: void Done(SetupFlowStep* next_step);
Need comments for this method. Is this transiting to the next step? If so please
rename the method.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:78: };
disallow_copy_and_assign

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:93: };
disallow_copy_and_assign

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:126: virtual void RegisterMessages();
Is it better to have SetupFlowStep implements DOMMessageHandler. In that case
you don't need receive the message here and delicate to the corresponding
SetupFlowStep.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:146: }
namespace

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow_login_step.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow_login_step.cc:26: if (message ==
"SubmitAuth") {
Make this class DOMMessageHandler and turn this into a DCHECK.

[Sergey Ulanov, 2010-12-21 22:26:49]

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:90: BrowserThread::PostTask(
On 2010/12/21 21:52:04, Alpha wrote:
> This should only be done once.
All other code I could find that calls AddDataSource() may do it more than once,
and so this shouldn't cause any problems. Data sources are refcounted, so this
doesn't leak memory.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:127:
handlers->push_back(const_cast<SetupFlow*>(this));
On 2010/12/21 21:52:04, Alpha wrote:
> Give the list of SetupFlowStep as handlers. This way this class will be
agnostic
> about what message to handle.
SetupFlow doesn't know about all steps of the flow in advance. Result of
SetupFlowStep::GetNextStep() depends on result of current step. Also, current
step is destroyed once we get to the next one. There is no way to tell DOMUI
that one of the message handlers is gone.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:176:
dom_ui_->RegisterMessageCallback("SubmitAuth",
On 2010/12/21 21:52:04, Alpha wrote:
> Should let SetupFlowStep to register their own messages.
That would be possible if DOMUI allowed unregistering messages, but it doesn't,
so once a step is assigned to handle a message, we'd have to keep it alive
forever until the dialog is closed.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.h (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:31: virtual void Start(SetupFlow* flow,
DoneCallback* done_callback) = 0;
On 2010/12/21 21:52:04, Alpha wrote:
> When is done_callback called?

Done.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:65: void Done(SetupFlowStep* next_step);
On 2010/12/21 21:52:04, Alpha wrote:
> Need comments for this method. Is this transiting to the next step? If so
please
> rename the method.

Done.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:78: };
On 2010/12/21 21:52:04, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:93: };
On 2010/12/21 21:52:04, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:126: virtual void RegisterMessages();
On 2010/12/21 21:52:04, Alpha wrote:
> Is it better to have SetupFlowStep implements DOMMessageHandler. In that case
> you don't need receive the message here and delicate to the corresponding
> SetupFlowStep.
This is not possible because we don't have list of all steps in advance.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.h:146: }
On 2010/12/21 21:52:04, Alpha wrote:
> namespace

Done.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow_login_step.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow_login_step.cc:26: if (message ==
"SubmitAuth") {
On 2010/12/21 21:52:04, Alpha wrote:
> Make this class DOMMessageHandler and turn this into a DCHECK.
This is not possible right now. We would have to change DOMUI to fix this.

[Alpha Left Google, 2010-12-24 06:33:05]

LGTM if TODO is added.

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:90: BrowserThread::PostTask(
On 2010/12/21 22:26:49, sergeyu wrote:
> On 2010/12/21 21:52:04, Alpha wrote:
> > This should only be done once.
> All other code I could find that calls AddDataSource() may do it more than
once,
> and so this shouldn't cause any problems. Data sources are refcounted, so this
> doesn't leak memory.

This is in fact a bug and they shouldn't be adding data source more than once.
Add a TODO here please.

[Sergey Ulanov, 2010-12-28 19:07:52]

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
File chrome/browser/remoting/setup_flow.cc (right):

http://codereview.chromium.org/5985003/diff/4001/chrome/browser/remoting/setu...
chrome/browser/remoting/setup_flow.cc:90: BrowserThread::PostTask(
On 2010/12/24 06:33:07, Alpha wrote:
> On 2010/12/21 22:26:49, sergeyu wrote:
> > On 2010/12/21 21:52:04, Alpha wrote:
> > > This should only be done once.
> > All other code I could find that calls AddDataSource() may do it more than
> once,
> > and so this shouldn't cause any problems. Data sources are refcounted, so
this
> > doesn't leak memory.
> 
> This is in fact a bug and they shouldn't be adding data source more than once.
> Add a TODO here please.

Done.