[Signin Error Dialog] (3/3) Added the triggering code

[Signin Error Dialog] (3/3) Added the triggering code

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 triggering the dialog.
DO NOT LAND until after the first two CLs land (
https://codereview.chromium.org/2274013002/,
https://codereview.chromium.org/2275883003/).

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

BUG=630523
BUG=615893
Committed: https://crrev.com/0abc347b7f09546244d619b81a5d0f3c55de3fee
Cr-Commit-Position: refs/heads/master@{#421220}

[anthonyvd, 2016-09-19 19:25:38]

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

[anthonyvd, 2016-09-19 19:25:39]

Hi Tommy,

This is part 3 of the work Jane started before her internship finished. I
uploaded the patch as it was in https://codereview.chromium.org/2255413002 in
patchset #1 and the same stuff rebased on current ToT in patchset #2.

Jane's latest patch seemed to address your feedback. Can you please take a look
at this?

Thanks!

[tommycli, 2016-09-20 17:48:05]

lgtm looks good. one nit:

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

https://codereview.chromium.org/2348423002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:83: void
DisplayLoginResult(Browser* browser,
I assume this displays a "success" dialog if there's an empty error_message and
an "error" dialog if it's populated? If so, can we add a bit of explanatory
comments?

[anthonyvd, 2016-09-21 15:07:23]

anthonyvd@chromium.org changed reviewers:
+ rogerta@chromium.org, rsesek@chromium.org, sky@chromium.org

[anthonyvd, 2016-09-21 15:07:24]

Hello everyone, can you please take a

rogerta@chromium.org: c/b/ui/sync/*
rsesek@chromium.org: c/b/ui/cocoa/*
sky@chromium.org: c/b/ui/* and c/b/ui/views*

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

https://codereview.chromium.org/2348423002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.h:83: void
DisplayLoginResult(Browser* browser,
On 2016/09/20 17:48:05, tommycli wrote:
> I assume this displays a "success" dialog if there's an empty error_message
and
> an "error" dialog if it's populated? If so, can we add a bit of explanatory
> comments?

Done.

[anthonyvd, 2016-09-21 15:09:26]

On 2016/09/21 15:07:24, anthonyvd wrote:
> Hello everyone, can you please take a
> 
> mailto:rogerta@chromium.org: c/b/ui/sync/*
> mailto:rsesek@chromium.org: c/b/ui/cocoa/*
> mailto:sky@chromium.org: c/b/ui/* and c/b/ui/views*
> 
>
https://codereview.chromium.org/2348423002/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/signin/login_ui_service.h (right):
> 
>
https://codereview.chromium.org/2348423002/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/login_ui_service.h:83: void
> DisplayLoginResult(Browser* browser,
> On 2016/09/20 17:48:05, tommycli wrote:
> > I assume this displays a "success" dialog if there's an empty error_message
> and
> > an "error" dialog if it's populated? If so, can we add a bit of explanatory
> > comments?
> 
> Done.

Oops, hit send mid-message... Let's try again!

Can you please take a look at this reupload of Jane's CL from here:
https://codereview.chromium.org/2255413002 ? This is the last part of the Signin
Error Modal implementation.

Thanks!

[sky, 2016-09-21 16:14:40]

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/signi...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/signi...
chrome/browser/ui/signin_view_controller_delegate.h:37: static
SigninViewControllerDelegate* CreateSigninErrorDelegate(
Please document lifetime here.

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:157:
views::WebView* SigninViewControllerDelegateViews::CreateSigninErrorWebView(
It's hard to reason about lifetime with raw pointers. Please return a unique_ptr
here and why supplying the WebView to SigninViewControllerDelegateViews.

[anthonyvd, 2016-09-22 16:52:01]

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/signi...
File chrome/browser/ui/signin_view_controller_delegate.h (right):

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/signi...
chrome/browser/ui/signin_view_controller_delegate.h:37: static
SigninViewControllerDelegate* CreateSigninErrorDelegate(
On 2016/09/21 16:14:39, sky wrote:
> Please document lifetime here.

Done.

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:157:
views::WebView* SigninViewControllerDelegateViews::CreateSigninErrorWebView(
On 2016/09/21 16:14:39, sky wrote:
> It's hard to reason about lifetime with raw pointers. Please return a
unique_ptr
> here and why supplying the WebView to SigninViewControllerDelegateViews.

Agreed, done. Could you clarify something about views ownership? I'm under the
impression that a view owns its subviews (and thus deletes them when it's
deleted itself). Is that correct? 

For example, in this case SigninViewControllerDelegate is a WidgetDelegate
subclass that doesn't own the WebView that it returns through GetContentsView. I
based this ownership model on the way WidgetDelegate handles its
default_content_view_ member (it instantiates it with new but doesn't delete
it).

Thanks in advance, I just want to make sure I'm not leaking anything!

[Robert Sesek, 2016-09-22 18:39:50]

cocoa/ LGTM

[sky, 2016-09-22 20:29:35]

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:157:
views::WebView* SigninViewControllerDelegateViews::CreateSigninErrorWebView(
On 2016/09/22 16:52:01, anthonyvd wrote:
> On 2016/09/21 16:14:39, sky wrote:
> > It's hard to reason about lifetime with raw pointers. Please return a
> unique_ptr
> > here and why supplying the WebView to SigninViewControllerDelegateViews.
> 
> Agreed, done. Could you clarify something about views ownership? I'm under the
> impression that a view owns its subviews (and thus deletes them when it's
> deleted itself). Is that correct? 

You are correct for the common case. The exception to that is if someone calls
set_owned_by_client() on a view. When a view is destroyed it iterates over the
children deleting those marked as not owned-by-client and removing those marked
as owned-by-client.

> For example, in this case SigninViewControllerDelegate is a WidgetDelegate
> subclass that doesn't own the WebView that it returns through GetContentsView.
I
> based this ownership model on the way WidgetDelegate handles its
> default_content_view_ member (it instantiates it with new but doesn't delete
> it).

Widget adds the view returned from GetContentsView as a child of the RootView.
So, as long as you haven't called set_owned_by_client() on the WebView it'll be
deleted by virtual of the RootView being deleted.

> 
> Thanks in advance, I just want to make sure I'm not leaking anything!

[anthonyvd, 2016-09-23 16:57:56]

On 2016/09/22 20:29:35, sky wrote:
>
https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
> (right):
> 
>
https://codereview.chromium.org/2348423002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:157:
> views::WebView* SigninViewControllerDelegateViews::CreateSigninErrorWebView(
> On 2016/09/22 16:52:01, anthonyvd wrote:
> > On 2016/09/21 16:14:39, sky wrote:
> > > It's hard to reason about lifetime with raw pointers. Please return a
> > unique_ptr
> > > here and why supplying the WebView to SigninViewControllerDelegateViews.
> > 
> > Agreed, done. Could you clarify something about views ownership? I'm under
the
> > impression that a view owns its subviews (and thus deletes them when it's
> > deleted itself). Is that correct? 
> 
> You are correct for the common case. The exception to that is if someone calls
> set_owned_by_client() on a view. When a view is destroyed it iterates over the
> children deleting those marked as not owned-by-client and removing those
marked
> as owned-by-client.
> 
> > For example, in this case SigninViewControllerDelegate is a WidgetDelegate
> > subclass that doesn't own the WebView that it returns through
GetContentsView.
> I
> > based this ownership model on the way WidgetDelegate handles its
> > default_content_view_ member (it instantiates it with new but doesn't delete
> > it).
> 
> Widget adds the view returned from GetContentsView as a child of the RootView.
> So, as long as you haven't called set_owned_by_client() on the WebView it'll
be
> deleted by virtual of the RootView being deleted.
> 
> > 
> > Thanks in advance, I just want to make sure I'm not leaking anything!

Thanks for the explanation! We're not calling set_owned_by_client() anywhere for
those WebViews so we should be good.

[sky, 2016-09-23 17:59:57]

LGTM

[Roger Tawa OOO till Jul 10th, 2016-09-26 19:00:36]

lgtm

[anthonyvd, 2016-09-26 19:17:09]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-09-26 19:17:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2348423002/#ps60001
(title: "Address feedback.")

[commit-bot: I haz the power, 2016-09-26 19:18:08]

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-26 19:59:48]

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

[commit-bot: I haz the power, 2016-09-26 19:59:49]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[anthonyvd, 2016-09-26 20:07:20]

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

[commit-bot: I haz the power, 2016-09-26 20:08: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-26 22:16:45]

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

[commit-bot: I haz the power, 2016-09-26 22:16:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[anthonyvd, 2016-09-27 14:55:10]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-09-27 14:55:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
tommycli@chromium.org, rogerta@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2348423002/#ps80001
(title: "Fix malformed function declaration on mac.")

[commit-bot: I haz the power, 2016-09-27 14:55:23]

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-27 15:41:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-27 15:43:20]

Description was changed from

==========
[Signin Error Dialog] (3/3) Added the triggering code

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 triggering the dialog.
DO NOT LAND until after the first two CLs land (
https://codereview.chromium.org/2274013002/,
https://codereview.chromium.org/2275883003/).

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

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

to

==========
[Signin Error Dialog] (3/3) Added the triggering code

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 triggering the dialog.
DO NOT LAND until after the first two CLs land (
https://codereview.chromium.org/2274013002/,
https://codereview.chromium.org/2275883003/).

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

BUG=630523
BUG=615893

Committed: https://crrev.com/0abc347b7f09546244d619b81a5d0f3c55de3fee
Cr-Commit-Position: refs/heads/master@{#421220}
==========

[commit-bot: I haz the power, 2016-09-27 15:43:21]

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