Network connectivity errors handling (incl. CP) for enrollment flow

Network connectivity errors handling (incl. CP) for enrollment flow

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

[Nikita (slow), 2014-06-18 12:05:48]

I'll let Yuri review this CL.

[ygorshenin1, 2014-06-18 13:11:35]

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/error_screen_actor.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/error_screen_actor.h:56: // Shows the
screen and call closure on hide.
1. Replace "closure" by "|closure|".
2. Pass closure by const reference instead of by value.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/webui_login_view.h:47: typedef
content::WebContentsObserver FrameObserver;
nit: add an empty line after line #47.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:409: }
nit: curly braces are redundant here.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:429: //
TODO(rsorokin): To many consecutive reloads.
nit: s/To/Too/

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:433: if
(!is_online || is_frame_error) {
nit: curly braces are redundant here.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:441:
std::string GetNetworkName(const std::string& service_path) {
Move this function to the anonymous namespace.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:483:
LOG(ERROR) << "fixcaptiveportal";
Remove debug code.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:514: 
I suggest you to inverse condition:
if (IsEnrollmentScreenHiddenByError())
  error_screen_actor_->Hide();

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:527:
base::UTF8ToUTF16("*oauth-enroll-signin-frame*")))
As condition requires 2 lines, curly braces are needed. As below, I thought it
would be better to inverse condition.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:115:
OobeUI::Screen GetCurrentScreen() const;
nit: add comment to each of these three methods.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:146:
net::Error frame_error_;
nit: add comment to |frame_error_|.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:150:
ErrorScreenActor* error_screen_actor_;
nit: insert an empty line after #149.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:90:
DCHECK(closure_.get());
No need in DCHECK() as you're check closure_.get() in the following line.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.h:48: virtual void
Show(OobeDisplay::Screen parent_screen,
Add OVERRIDE.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.h:58: void
CheckAndShowScreen();
Add comment to this method.

[Roman Sorokin (ftl), 2014-06-19 11:57:38]

PTAL

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/error_screen_actor.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/error_screen_actor.h:56: // Shows the
screen and call closure on hide.
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> 1. Replace "closure" by "|closure|".
> 2. Pass closure by const reference instead of by value.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/webui_login_view.h:47: typedef
content::WebContentsObserver FrameObserver;
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: add an empty line after line #47.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:409: }
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: curly braces are redundant here.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:429: //
TODO(rsorokin): To many consecutive reloads.
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: s/To/Too/

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:433: if
(!is_online || is_frame_error) {
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: curly braces are redundant here.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:441:
std::string GetNetworkName(const std::string& service_path) {
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> Move this function to the anonymous namespace.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:483:
LOG(ERROR) << "fixcaptiveportal";
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:514: 
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> I suggest you to inverse condition:
> if (IsEnrollmentScreenHiddenByError())
>   error_screen_actor_->Hide();

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:527:
base::UTF8ToUTF16("*oauth-enroll-signin-frame*")))
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> As condition requires 2 lines, curly braces are needed. As below, I thought it
> would be better to inverse condition.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:115:
OobeUI::Screen GetCurrentScreen() const;
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: add comment to each of these three methods.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:146:
net::Error frame_error_;
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: add comment to |frame_error_|.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:150:
ErrorScreenActor* error_screen_actor_;
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> nit: insert an empty line after #149.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:90:
DCHECK(closure_.get());
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> No need in DCHECK() as you're check closure_.get() in the following line.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.h:48: virtual void
Show(OobeDisplay::Screen parent_screen,
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> Add OVERRIDE.

Done.

https://codereview.chromium.org/332243002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.h:58: void
CheckAndShowScreen();
On 2014/06/18 13:11:35, ygorshenin1 wrote:
> Add comment to this method.

Done.

[ygorshenin1, 2014-06-19 18:28:53]

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/error_screen_actor.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/error_screen_actor.h:56: // Shows the
screen and call |hide_closure| on hide.
nit: I suggest to rename "hide_closure" to "on_hide".

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/webui_login_view.h:47: typedef
content::WebContentsObserver FrameObserver;
Are you planning to implement your own Observer class instead of
WebContentsObserver?

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:118: //
Returns true if current visible screen is the Enrollment sign-in page.
nit: s/Enrollment/enrollment/

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:123: bool
IsEnrollmentScreenHiddenByError() const;
nit: s/Enrollment/enrollment/

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:152: //
Latest Enrollment frame error.
nit: s/Enrollment/enrollment/

[Roman Sorokin (ftl), 2014-06-23 12:58:30]

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/screens/error_screen_actor.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/screens/error_screen_actor.h:56: // Shows the
screen and call |hide_closure| on hide.
On 2014/06/19 18:28:53, ygorshenin1 wrote:
> nit: I suggest to rename "hide_closure" to "on_hide".

Done.

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/webui_login_view.h:47: typedef
content::WebContentsObserver FrameObserver;
On 2014/06/19 18:28:53, ygorshenin1 wrote:
> Are you planning to implement your own Observer class instead of
> WebContentsObserver?

Done.

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:118: //
Returns true if current visible screen is the Enrollment sign-in page.
On 2014/06/19 18:28:53, ygorshenin1 wrote:
> nit: s/Enrollment/enrollment/

Done.

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:123: bool
IsEnrollmentScreenHiddenByError() const;
On 2014/06/19 18:28:53, ygorshenin1 wrote:
> nit: s/Enrollment/enrollment/

Done.

https://codereview.chromium.org/332243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:152: //
Latest Enrollment frame error.
On 2014/06/19 18:28:53, ygorshenin1 wrote:
> nit: s/Enrollment/enrollment/

Done.

[Nikita (slow), 2014-06-23 13:42:20]

Denis, please help with this review.

[Denis Kuznetsov (DE-MUC), 2014-06-23 17:42:02]

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ch...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ch...
chrome/browser/chromeos/login/ui/webui_login_view.cc:315: if (frame_observer &&
!frame_observer_list_.HasObserver(frame_observer))
Does it make sense to have NULL frame_observer?
What should happen on RemoveObserver if it was NULL initially?
What if we double-add it and then double-remove it?

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:137: if
(chromeos::LoginDisplayHostImpl::default_host()) {
What if it is NULL here?

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:428: void
EnrollmentScreenHandler::UpdateState(
Make a note that this is a copy-pasted code from SigninScreenHandler, so that we
can easily find it later.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:502:
base::Bind(&EnrollmentScreenHandler::DoShow, base::Unretained(this)));
Please use weak refs instead of unretained.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:513: void
EnrollmentScreenHandler::onFrameError(
Why this method name starts with lower-case? (see above)

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:73: virtual
void onFrameError(const base::string16& frame_unique_name) OVERRIDE;
s/on/On/

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:84:
base::Unretained(this)));
Use weak-refs istead of Unretained.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:91:
on_hide_->Run();
What blocks you from using usual ShowScreen path, why do we need to make this
workaround for calling DoShow directly?

[Roman Sorokin (ftl), 2014-06-25 16:30:13]

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ch...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ch...
chrome/browser/chromeos/login/ui/webui_login_view.cc:315: if (frame_observer &&
!frame_observer_list_.HasObserver(frame_observer))
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> Does it make sense to have NULL frame_observer?
I copied this code from another function AddObserver which is virtual. This
function is not virtual and being used in one place so I guess I could remove
test on NULL.
> What should happen on RemoveObserver if it was NULL initially?
Nothing
> What if we double-add it and then double-remove it?
ObserverList does not allow double-add observers. It will log error and won't
add observer second time.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:137: if
(chromeos::LoginDisplayHostImpl::default_host()) {
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> What if it is NULL here?

It should be created at this point. If not we wouldn't receive messages about
enrollment frame loading fails. Or should I use DCHECK?

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:428: void
EnrollmentScreenHandler::UpdateState(
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> Make a note that this is a copy-pasted code from SigninScreenHandler, so that
we
> can easily find it later.

Done.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:502:
base::Bind(&EnrollmentScreenHandler::DoShow, base::Unretained(this)));
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> Please use weak refs instead of unretained.

Done.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:513: void
EnrollmentScreenHandler::onFrameError(
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> Why this method name starts with lower-case? (see above)

Done.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:73: virtual
void onFrameError(const base::string16& frame_unique_name) OVERRIDE;
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> s/on/On/

Done.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:84:
base::Unretained(this)));
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> Use weak-refs istead of Unretained.

Done.

https://chromiumcodereview.appspot.com/332243002/diff/60001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:91:
on_hide_->Run();
On 2014/06/23 17:42:01, Denis Kuznetsov wrote:
> What blocks you from using usual ShowScreen path, why do we need to make this
> workaround for calling DoShow directly?

Cause we need to pass screen_data (which is DoShow sets) to ShowScreen to show
enrollment screen.

[Roman Sorokin (ftl), 2014-07-01 09:13:33]

PTAL

[Denis Kuznetsov (DE-MUC), 2014-07-01 16:04:58]

lgtm

[ygorshenin1, 2014-07-07 10:30:14]

lgtm

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:414: if
(login_view_)
nit: it would be better to check that login_window_ is not NULL too.

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:88:
net::Error FrameError() const { return frame_error_; }
nit: you can replace FrameError() by frame_error().

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:91: if
(on_hide_.get())
nit: if (on_hide_) ...

[Roman Sorokin (ftl), 2014-07-08 17:19:54]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-07-08 17:20:03]

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:414: if
(login_view_)
On 2014/07/07 10:30:14, ygorshenin1 wrote:
> nit: it would be better to check that login_window_ is not NULL too.

Done.

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h:88:
net::Error FrameError() const { return frame_error_; }
On 2014/07/07 10:30:14, ygorshenin1 wrote:
> nit: you can replace FrameError() by frame_error().

Done.

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc (right):

https://codereview.chromium.org/332243002/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc:91: if
(on_hide_.get())
On 2014/07/07 10:30:14, ygorshenin1 wrote:
> nit: if (on_hide_) ...

Done.

[commit-bot: I haz the power, 2014-07-08 17:21:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsorokin@chromium.org/332243002/180001

[commit-bot: I haz the power, 2014-07-08 22:53:22]

Change committed as 281860