Add a pref to track progress of OOBE and resume if needed.

Add a pref to track progress of OOBE and resume if needed.

BUG=372368
TEST=browser_test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272056

[pastarmovj, 2014-05-14 13:32:58]

Please review this CL that adds logic to remember the last reached page of the
OOBE flow.

[Mattias Nissler (ping if slow), 2014-05-14 13:39:02]

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:212: bool shoud_not_resume =
false;
This could just be should_not_resume = <condition-of-if-statement-below>

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:218:
GetLocalState()->FindPreference(prefs::kOobeStagePending);
it's a bit confusing that the new code calls this "stage" while everything else
calls it "screen". Maybe rename to prefs::kOobeScreenToRestartAt?

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
Why would you need a default value check here?

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
nit: no curlies required.

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:775: // First remember how
far have we reached so that we can resume if needed.
Most transitions go through SetCurrentScreen instead of AdvanceToScreen, so not
sure this is good enough?

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:910: 
remove extra blank line

[pastarmovj, 2014-05-14 14:23:49]

Addressed the comments. PTAL.

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:212: bool shoud_not_resume =
false;
On 2014/05/14 13:39:03, Mattias Nissler wrote:
> This could just be should_not_resume = <condition-of-if-statement-below>

True. This code was going through a couple of versions and I forgot to tidy it
up in the end.

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
On 2014/05/14 13:39:03, Mattias Nissler wrote:
> Why would you need a default value check here?

If there is no value set we don't need to intervene right.

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:775: // First remember how
far have we reached so that we can resume if needed.
On 2014/05/14 13:39:03, Mattias Nissler wrote:
> Most transitions go through SetCurrentScreen instead of AdvanceToScreen, so
not
> sure this is good enough?

Yep missed that one but now it is added and I verified that the code still
behaves. :)

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:910: 
On 2014/05/14 13:39:03, Mattias Nissler wrote:
> remove extra blank line

Done.

[Mattias Nissler (ping if slow), 2014-05-14 14:54:41]

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
On 2014/05/14 14:23:49, pastarmovj wrote:
> On 2014/05/14 13:39:03, Mattias Nissler wrote:
> > Why would you need a default value check here?
> 
> If there is no value set we don't need to intervene right.

Well, you could just check for empty string I guess? In fact, doesn't the code
handle empty string just fine already? In that case, this entire thing would
work just as well in this form:

if (!should_not_resume)
  first_screen_name_ = GetLocalState()->GetString(prefs::kOobeStagePending);

[pastarmovj, 2014-05-14 15:26:44]

Renamed stage to screen because I forgot in the previous CL.

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
On 2014/05/14 14:54:41, Mattias Nissler wrote:
> On 2014/05/14 14:23:49, pastarmovj wrote:
> > On 2014/05/14 13:39:03, Mattias Nissler wrote:
> > > Why would you need a default value check here?
> > 
> > If there is no value set we don't need to intervene right.
> 
> Well, you could just check for empty string I guess? In fact, doesn't the code
> handle empty string just fine already? In that case, this entire thing would
> work just as well in this form:
> 
> if (!should_not_resume)
>   first_screen_name_ = GetLocalState()->GetString(prefs::kOobeStagePending);

I think doing the check this way is more efficient isn't it and not less clear.

I don't mind changing this to a check for empty string but I would rather leave
this check here at least to keep the test code happy if not for added
belt-and-suspenders effect.

[Mattias Nissler (ping if slow), 2014-05-15 09:30:25]

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:219: if
(!oobe_stage_pref->IsDefaultValue() && !shoud_not_resume) {
On 2014/05/14 15:26:44, pastarmovj wrote:
> On 2014/05/14 14:54:41, Mattias Nissler wrote:
> > On 2014/05/14 14:23:49, pastarmovj wrote:
> > > On 2014/05/14 13:39:03, Mattias Nissler wrote:
> > > > Why would you need a default value check here?
> > > 
> > > If there is no value set we don't need to intervene right.
> > 
> > Well, you could just check for empty string I guess? In fact, doesn't the
code
> > handle empty string just fine already? In that case, this entire thing would
> > work just as well in this form:
> > 
> > if (!should_not_resume)
> >   first_screen_name_ = GetLocalState()->GetString(prefs::kOobeStagePending);
> 
> I think doing the check this way is more efficient isn't it and not less
clear.
> 
> I don't mind changing this to a check for empty string but I would rather
leave
> this check here at least to keep the test code happy if not for added
> belt-and-suspenders effect.

I don't think efficiency matters at all here (it's hard to do something that's
not cheap).

I don't see why the test code would stop working if we just read the pref here,
and I don't see how the code you currently have (4 lines) is clearer than what I
proposed (2 lines). Unless I'm missing something and you have to actually test
for the empty string here, but I think just assigning and relying on the pref to
be registered with an empty string as the default value would do the trick?

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/startup_utils.h (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/startup_utils.h:30: // Stores the next pending
OOBE stage in case it will need to be resumed.
s/stage/screen/

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:723: // First remember how
far have we reached so that we can resume if needed.
So there are some interstitial screens that we may not want to return to after
restarting OOBE. The error screen is probably in that category. Not sure whether
that's a problem in practice and if so how to filter them. You probably want to
ask Nikita or Yuri for that.

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:872: 
remove extra blank line.

https://codereview.chromium.org/275913004/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:2087: const char kOobeScreenPending[] =
"OobeStagePending";
two more instances of "stage" to fix.

[Nikita (slow), 2014-05-15 13:05:19]

Rebalancing, I'll let Yuri review this code.

[ygorshenin1, 2014-05-16 13:03:28]

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:212: bool shoud_not_resume =
is_out_of_box_ && !first_screen_name.empty() &&
nit: could you please replace
"should_not_resume = A && !B && !C" by "should_resume = !A || B || C" ?

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:723: // First remember how
far have we reached so that we can resume if needed.
Yes, we definitely shouldn't return to the error screen. Instead, it's better to
resume it's parent screen (take a look at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...).
Also, I thought there're no sense to resume EULA screen or update screen - we
can just start OOBE from scratch.

On 2014/05/15 09:30:25, Mattias Nissler wrote:
> So there are some interstitial screens that we may not want to return to after
> restarting OOBE. The error screen is probably in that category. Not sure
whether
> that's a problem in practice and if so how to filter them. You probably want
to
> ask Nikita or Yuri for that.

[pastarmovj, 2014-05-19 14:12:26]

Thanks for the reviews guys. Please take a look at the updated version.

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/startup_utils.h (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/startup_utils.h:30: // Stores the next pending
OOBE stage in case it will need to be resumed.
On 2014/05/15 09:30:25, Mattias Nissler wrote:
> s/stage/screen/

Done.

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:212: bool shoud_not_resume =
is_out_of_box_ && !first_screen_name.empty() &&
On 2014/05/16 13:03:29, ygorshenin1 wrote:
> nit: could you please replace
> "should_not_resume = A && !B && !C" by "should_resume = !A || B || C" ?

Actually compacted the logic further and i even think it was not exactly right
before anyway.

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:723: // First remember how
far have we reached so that we can resume if needed.
On 2014/05/16 13:03:29, ygorshenin1 wrote:
> Yes, we definitely shouldn't return to the error screen. Instead, it's better
to
> resume it's parent screen (take a look at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...).
> Also, I thought there're no sense to resume EULA screen or update screen - we
> can just start OOBE from scratch.
> 
> On 2014/05/15 09:30:25, Mattias Nissler wrote:
> > So there are some interstitial screens that we may not want to return to
after
> > restarting OOBE. The error screen is probably in that category. Not sure
> whether
> > that's a problem in practice and if so how to filter them. You probably want
> to
> > ask Nikita or Yuri for that.
> 

Ok new proposal implemented below is only store the last screen of the main OOBE
sequence - Network -> EULA -> AU -> (the new screen to come) -> Register.

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:872: 
On 2014/05/15 09:30:25, Mattias Nissler wrote:
> remove extra blank line.

Done.

https://codereview.chromium.org/275913004/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/275913004/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:2087: const char kOobeScreenPending[] =
"OobeStagePending";
On 2014/05/15 09:30:25, Mattias Nissler wrote:
> two more instances of "stage" to fix.

Done.

[Mattias Nissler (ping if slow), 2014-05-20 09:33:12]

This LGTM sufficiently from my side, I just have a couple more suggestions for
cleanup. I'll leave it to Yuri to do the final review + approval.

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:233: first_screen_name_ =
GetLocalState()->GetString(prefs::kOobeScreenPending);
nit: no need to ask local_state again, just assign |screen_pref|

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:742:
StartupUtils::SaveOobePendingScreen(new_current->GetName());
Shouldn't this code be in SetCurrentScreenSmooth (i.e. at the point where we
actually set |current_screen_|, so we catch any screen transitions?

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:794:
StartupUtils::SaveOobePendingScreen(screen_name);
Isn't the code below going through SetCurrentScreen anyways?

[pastarmovj, 2014-05-20 13:57:01]

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:233: first_screen_name_ =
GetLocalState()->GetString(prefs::kOobeScreenPending);
On 2014/05/20 09:33:12, Mattias Nissler wrote:
> nit: no need to ask local_state again, just assign |screen_pref|

Of course :) this was a branch messing artifact :)

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:742:
StartupUtils::SaveOobePendingScreen(new_current->GetName());
On 2014/05/20 09:33:12, Mattias Nissler wrote:
> Shouldn't this code be in SetCurrentScreenSmooth (i.e. at the point where we
> actually set |current_screen_|, so we catch any screen transitions?

Done.

https://codereview.chromium.org/275913004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:794:
StartupUtils::SaveOobePendingScreen(screen_name);
On 2014/05/20 09:33:12, Mattias Nissler wrote:
> Isn't the code below going through SetCurrentScreen anyways?

Done.

[ygorshenin1, 2014-05-20 16:23:15]

lgtm

[pastarmovj, 2014-05-21 07:41:57]

The CQ bit was checked by pastarmovj@chromium.org

[commit-bot: I haz the power, 2014-05-21 07:43:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/275913004/120001

[commit-bot: I haz the power, 2014-05-22 03:01:42]

Change committed as 272056