[Signin Error Dialog] (2/3) Added handlers and UI constructors

[Signin Error Dialog] (2/3) Added handlers and UI constructors

Signin Error Dialog:

Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or
signin error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.

DO NOT LAND until after the first CL lands
(https://codereview.chromium.org/2274013002/).

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.9vm5owqqt3w1

BUG=630523
BUG=615893
Committed: https://crrev.com/32af54611ca4a83a599347cf52042c51838c6579
Cr-Commit-Position: refs/heads/master@{#418673}

[anthonyvd, 2016-09-07 16:05:27]

anthonyvd@chromium.org changed reviewers:
+ achuith@chromium.org, sky@chromium.org, tommycli@chromium.org

[anthonyvd, 2016-09-07 16:05:28]

Hello everyone,

This CL is a reupload of https://codereview.chromium.org/2275883003/ (which was
all lgtm'd but Jane's internship is over). 

It's essentially the same, except it fixes GN builds and some linter errors. Can
you please take a quick look?

Thanks!

[anthonyvd, 2016-09-07 16:06:54]

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

[commit-bot: I haz the power, 2016-09-07 16:07:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 17:12:24]

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

[commit-bot: I haz the power, 2016-09-07 17:12:25]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-07 17:22:10]

Any chance you could upload Jane's patch (
https://codereview.chromium.org/2275883003/ ) merged to tip of tree as
the first patch, and then the fixes as the second patch so we can
easily diff it?

  -Scott

On Wed, Sep 7, 2016 at 9:05 AM,  <anthonyvd@chromium.org> wrote:
> Reviewers: achuithb, sky, tommycli (OOO)
> CL: https://codereview.chromium.org/2315393002/
>
> Message:
> Hello everyone,
>
> This CL is a reupload of https://codereview.chromium.org/2275883003/ (which
> was
> all lgtm'd but Jane's internship is over).
>
> It's essentially the same, except it fixes GN builds and some linter errors.
> Can
> you please take a quick look?
>
> Thanks!
>
> Description:
> [Signin Error Dialog] (2/3) Added handlers and UI constructors
>
> Signin Error Dialog:
>
> Part of the desktop user menu revamp project is to migrate signin error
> surfacing from user menu's tutorial card to a tab modal dialog, and thus
> completing the flow of signin modal -> sync confirmation modal dialog or
> signin error modal dialog.
>
> Particularly, this CL adds the code for appropriately constructing and
> handling the dialog.
>
> DO NOT LAND until after the first CL lands
> (https://codereview.chromium.org/2274013002/).
>
> Design doc:
>
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
>
> BUG=630523
> BUG=615893
>
> Affected files (+380, -132 lines):
> M chrome/app/generated_resources.grd
> M chrome/browser/chromeos/login/saml/saml_browsertest.cc
> M chrome/browser/chromeos/login/test/oobe_base_test.cc
> M chrome/browser/profiles/profile_metrics.h
> M chrome/browser/ui/BUILD.gn
> M chrome/browser/ui/signin_view_controller_delegate.cc
> M chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
> D chrome/browser/ui/webui/signin/get_auth_frame.h
> D chrome/browser/ui/webui/signin/get_auth_frame.cc
> M chrome/browser/ui/webui/signin/inline_login_handler.cc
> M chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
> M chrome/browser/ui/webui/signin/login_ui_service.h
> M chrome/browser/ui/webui/signin/login_ui_service.cc
> M chrome/browser/ui/webui/signin/login_ui_test_utils.cc
> A chrome/browser/ui/webui/signin/signin_error_handler.h
> A chrome/browser/ui/webui/signin/signin_error_handler.cc
> A chrome/browser/ui/webui/signin/signin_error_ui.h
> A chrome/browser/ui/webui/signin/signin_error_ui.cc
> A + chrome/browser/ui/webui/signin/signin_utils.h
> A + chrome/browser/ui/webui/signin/signin_utils.cc
> M chrome/browser/ui/webui/signin/sync_confirmation_handler.h
> M chrome/browser/ui/webui/signin/sync_confirmation_handler.cc
> M chrome/chrome_browser_ui.gypi
> M chrome/common/url_constants.h
> M chrome/common/url_constants.cc
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[anthonyvd, 2016-09-07 17:33:44]

Patchset #1 (id:1) has been deleted

[anthonyvd, 2016-09-07 17:48:20]

Patchset #1 (id:20001) has been deleted

[anthonyvd, 2016-09-07 17:52:12]

Patchset #1 (id:40001) has been deleted

[anthonyvd, 2016-09-07 20:41:08]

On 2016/09/07 at 17:22:10, sky wrote:
> Any chance you could upload Jane's patch (
> https://codereview.chromium.org/2275883003/ ) merged to tip of tree as
> the first patch, and then the fixes as the second patch so we can
> easily diff it?
> 
>   -Scott
> 
> On Wed, Sep 7, 2016 at 9:05 AM,  <anthonyvd@chromium.org> wrote:
> > Reviewers: achuithb, sky, tommycli (OOO)
> > CL: https://codereview.chromium.org/2315393002/
> >
> > Message:
> > Hello everyone,
> >
> > This CL is a reupload of https://codereview.chromium.org/2275883003/ (which
> > was
> > all lgtm'd but Jane's internship is over).
> >
> > It's essentially the same, except it fixes GN builds and some linter errors.
> > Can
> > you please take a quick look?
> >
> > Thanks!
> >
> > Description:
> > [Signin Error Dialog] (2/3) Added handlers and UI constructors
> >
> > Signin Error Dialog:
> >
> > Part of the desktop user menu revamp project is to migrate signin error
> > surfacing from user menu's tutorial card to a tab modal dialog, and thus
> > completing the flow of signin modal -> sync confirmation modal dialog or
> > signin error modal dialog.
> >
> > Particularly, this CL adds the code for appropriately constructing and
> > handling the dialog.
> >
> > DO NOT LAND until after the first CL lands
> > (https://codereview.chromium.org/2274013002/).
> >
> > Design doc:
> >
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...
> >
> > BUG=630523
> > BUG=615893
> >
> > Affected files (+380, -132 lines):
> > M chrome/app/generated_resources.grd
> > M chrome/browser/chromeos/login/saml/saml_browsertest.cc
> > M chrome/browser/chromeos/login/test/oobe_base_test.cc
> > M chrome/browser/profiles/profile_metrics.h
> > M chrome/browser/ui/BUILD.gn
> > M chrome/browser/ui/signin_view_controller_delegate.cc
> > M chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
> > D chrome/browser/ui/webui/signin/get_auth_frame.h
> > D chrome/browser/ui/webui/signin/get_auth_frame.cc
> > M chrome/browser/ui/webui/signin/inline_login_handler.cc
> > M chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
> > M chrome/browser/ui/webui/signin/login_ui_service.h
> > M chrome/browser/ui/webui/signin/login_ui_service.cc
> > M chrome/browser/ui/webui/signin/login_ui_test_utils.cc
> > A chrome/browser/ui/webui/signin/signin_error_handler.h
> > A chrome/browser/ui/webui/signin/signin_error_handler.cc
> > A chrome/browser/ui/webui/signin/signin_error_ui.h
> > A chrome/browser/ui/webui/signin/signin_error_ui.cc
> > A + chrome/browser/ui/webui/signin/signin_utils.h
> > A + chrome/browser/ui/webui/signin/signin_utils.cc
> > M chrome/browser/ui/webui/signin/sync_confirmation_handler.h
> > M chrome/browser/ui/webui/signin/sync_confirmation_handler.cc
> > M chrome/chrome_browser_ui.gypi
> > M chrome/common/url_constants.h
> > M chrome/common/url_constants.cc
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 
> 

Done, sorry about that.

The first patch is Jane's original CL (except for the #include
"chrome/grit/browser_resources.h" line, which was #include
"chrome/grit/generated_resources.h" in her CL, which presubmit rejected). The
second patch contains my changes, which are only the BUILD.gn changes.

Thanks!

[sky, 2016-09-07 21:42:25]

Thanks, now I can actually see what changed. LGTM

[anthonyvd, 2016-09-12 14:07:00]

Super friendly ping: since this was lgtm'd in
https://codereview.chromium.org/2275883003/ and sky@ reviewed the BUILD changes,
I'll land this CL this afternoon. 

achuith@, tommycli@: let me know if you have any more feedback!

[achuithb, 2016-09-12 19:19:26]

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:86: const base::string16&
GetLastLoginResult();
This one too?

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:90: const base::string16&
GetLastLoginErrorEmail();
Shouldn't this method be const?

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_handler.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:22: SigninErrorHandler() :
duplicate_profile_entry_(nullptr) {}
I think we prefer in-class initialization now

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:30: void
set_duplicate_profile_entry(
Why not inline this?

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:57: const
ProfileAttributesEntry* duplicate_profile_entry_;
= nullptr?

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_ui.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:42: base::string16
last_login_result(login_ui_service->GetLastLoginResult());
const

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:44: if (email.empty()) {
const

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:78: // Elide the existing name
for the switch uer button label.
uer -> user

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_ui.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.h:24:
DISALLOW_COPY_AND_ASSIGN(SigninErrorUI);
This should be private

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_utils.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_utils.cc:68: bool success =
args->GetDouble(0, &height);
const

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:68: DCHECK(browser);
Doesn't this crash on the next line if browser is null?

Here and everywhere below

[anthonyvd, 2016-09-13 14:25:58]

Thanks for the feedback! The latest patchset addresses all comments.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:86: const base::string16&
GetLastLoginResult();
On 2016/09/12 at 19:19:25, achuithb wrote:
> This one too?

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:90: const base::string16&
GetLastLoginErrorEmail();
On 2016/09/12 at 19:19:25, achuithb wrote:
> Shouldn't this method be const?

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_handler.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:22: SigninErrorHandler() :
duplicate_profile_entry_(nullptr) {}
On 2016/09/12 at 19:19:25, achuithb wrote:
> I think we prefer in-class initialization now

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:30: void
set_duplicate_profile_entry(
On 2016/09/12 at 19:19:25, achuithb wrote:
> Why not inline this?

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_handler.h:57: const
ProfileAttributesEntry* duplicate_profile_entry_;
On 2016/09/12 at 19:19:25, achuithb wrote:
> = nullptr?

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_ui.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:42: base::string16
last_login_result(login_ui_service->GetLastLoginResult());
On 2016/09/12 at 19:19:26, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:44: if (email.empty()) {
On 2016/09/12 at 19:19:26, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.cc:78: // Elide the existing name
for the switch uer button label.
On 2016/09/12 at 19:19:26, achuithb wrote:
> uer -> user

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_error_ui.h (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_error_ui.h:24:
DISALLOW_COPY_AND_ASSIGN(SigninErrorUI);
On 2016/09/12 at 19:19:26, achuithb wrote:
> This should be private

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_utils.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_utils.cc:68: bool success =
args->GetDouble(0, &height);
On 2016/09/12 at 19:19:26, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/2315393002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:68: DCHECK(browser);
On 2016/09/12 at 19:19:26, achuithb wrote:
> Doesn't this crash on the next line if browser is null?
> 
> Here and everywhere below

Good point. Since there's really nothing to do in the (exceptional condition)
where this code is reached and there's no browser, I've replaced the DCHECKs by
a wrapping if().

[achuithb, 2016-09-13 17:40:53]

lgtm. 

I'm sorry I didn't get a chance to go through the details of this code in great
depth, and am assuming one of your other reviewers did.

https://codereview.chromium.org/2315393002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/2315393002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:110: if (browser) {
I think early return is preferable here at least (and possibly in some of the
other cases too?)

[anthonyvd, 2016-09-13 18:55:59]

https://codereview.chromium.org/2315393002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/2315393002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:110: if (browser) {
On 2016/09/13 at 17:40:53, achuithb wrote:
> I think early return is preferable here at least (and possibly in some of the
other cases too?)

You're right about this one, so done. I left the other ones as-is since the
small blocks made the early return ever-so-slightly less readable.

[tommycli, 2016-09-14 18:22:00]

lgtm same as original CL

[anthonyvd, 2016-09-14 18:23:16]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-09-14 18:23:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2315393002/#ps120001
(title: "Early return.")

[commit-bot: I haz the power, 2016-09-14 18:23:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 18:27:21]

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

[commit-bot: I haz the power, 2016-09-14 18:27:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2016-09-14 19:09:58]

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

[commit-bot: I haz the power, 2016-09-14 19:11:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 20:57:40]

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

[commit-bot: I haz the power, 2016-09-14 20:57:41]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2016-09-14 21:00:23]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-09-14 21:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, achuith@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2315393002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-14 21:00:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 21:10:21]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-09-14 21:12:40]

Description was changed from

==========
[Signin Error Dialog] (2/3) Added handlers and UI constructors

Signin Error Dialog:

Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or
signin error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.

DO NOT LAND until after the first CL lands
(https://codereview.chromium.org/2274013002/).

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

BUG=630523
BUG=615893
==========

to

==========
[Signin Error Dialog] (2/3) Added handlers and UI constructors

Signin Error Dialog:

Part of the desktop user menu revamp project is to migrate signin error
surfacing from user menu's tutorial card to a tab modal dialog, and thus
completing the flow of signin modal -> sync confirmation modal dialog or
signin error modal dialog.

Particularly, this CL adds the code for appropriately constructing and
handling the dialog.

DO NOT LAND until after the first CL lands
(https://codereview.chromium.org/2274013002/).

Design doc:
https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq...

BUG=630523
BUG=615893

Committed: https://crrev.com/32af54611ca4a83a599347cf52042c51838c6579
Cr-Commit-Position: refs/heads/master@{#418673}
==========

[commit-bot: I haz the power, 2016-09-14 21:12:41]

Patchset 5 (id:??) landed as
https://crrev.com/32af54611ca4a83a599347cf52042c51838c6579
Cr-Commit-Position: refs/heads/master@{#418673}