Adding new general bubble error message consisting of icon, message and caption. Using this message…

Adding new general bubble error message consisting of icon, message and caption. Using this message for the multi signin user error case.

There is still more to do: Unit tests & removal of test code. Beside that feature wise it is now fully functional as requested.

BUG=239201
TEST=visual, multiple scenarios

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

[Mr4D (OOO till 08-26), 2013-05-21 00:56:21]

Adding new error message box and integrate it into the multi login scenario.

There is still more to do: Unit tests & removal of test code. Beside that
feature wise it is now fully functional as requested.

Please have a look!

[msw, 2013-05-21 01:41:27]

https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h#newcode36
ash/popup_message.h:36: PopupMessage(const base::string16& caption,
Could the GlobalErrorBubble be extended for this use case? Or could you expose
and re-use the error message appearance (and/or other components) of its
internal Views implementation? That might be better than re-implementing
something similar but separate.

https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:277: if (GetBubbleFrameView() &&
GetBubbleFrameView()->bubble_border())
You should be able to do this in the PopupMessage::MessageBubble ctor, since
GetBubbleFrameView() is a protected member of its BubbleDelegateView superclass.
Then you don't need to modify the widely used bubble implementation for a less
common setting.

[Mr4D (OOO till 08-26), 2013-05-21 17:26:40]

Addressed. Have another look!

https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h#newcode36
ash/popup_message.h:36: PopupMessage(const base::string16& caption,
I have never seen that thing being used - even more so I didn't even know that
existed.

Looking at the views version of this bubble I can see that there are about 20
lines of "real code" and 160 lines of construction of the elements (which would
possibly be replaced for that). Furthermore it seems not remotely to be what is
being asked for. As such I would say that the idea to use a general class is
good (in fact I asked around before implementing and no one knew about this) but
that thing is too far away from what we want. (It is meant to be a targeted
notification).

So the answer would be no.

https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
File ui/views/bubble/bubble_delegate.cc (right):

https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
ui/views/bubble/bubble_delegate.cc:277: if (GetBubbleFrameView() &&
GetBubbleFrameView()->bubble_border())
Well - it appears that you have never tried this before because changing the
offset post creation has several implications. But I was able to find a (in my
view not so clean) way around it and to remove this function. For the sake of
clarity and simplicity I would strongly recommend to add this function though.

Removed as requested.

[Nikita (slow), 2013-05-21 18:03:52]

couple of drive-by comments (I haven't reviewed whole CL).

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:109: // Offsetting the popup message relativ to the
tray menu.
nit: relative

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:313: // The anchor view for the "all users are
already assigned" message.
nit: I would leave comment more generic as it could be other message as well.

[msw, 2013-05-21 18:12:53]

On 2013/05/21 17:26:40, Mr4D wrote:
> Addressed. Have another look!
> 
> https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h
> File ash/popup_message.h (right):
> 
> https://codereview.chromium.org/15329005/diff/1/ash/popup_message.h#newcode36
> ash/popup_message.h:36: PopupMessage(const base::string16& caption,
> I have never seen that thing being used - even more so I didn't even know that
> existed.
> 
> Looking at the views version of this bubble I can see that there are about 20
> lines of "real code" and 160 lines of construction of the elements (which
would
> possibly be replaced for that). Furthermore it seems not remotely to be what
is
> being asked for. As such I would say that the idea to use a general class is
> good (in fact I asked around before implementing and no one knew about this)
but
> that thing is too far away from what we want. (It is meant to be a targeted
> notification).
> 
> So the answer would be no.

I wasn't sure it'd help, but thank you for evaluating that code against your
needs.
Any opportunities to improve and consolidate code ought to be explored.

> 
>
https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
> File ui/views/bubble/bubble_delegate.cc (right):
> 
>
https://codereview.chromium.org/15329005/diff/1/ui/views/bubble/bubble_delega...
> ui/views/bubble/bubble_delegate.cc:277: if (GetBubbleFrameView() &&
> GetBubbleFrameView()->bubble_border())
> Well - it appears that you have never tried this before because changing the
> offset post creation has several implications. But I was able to find a (in my
> view not so clean) way around it and to remove this function. For the sake of
> clarity and simplicity I would strongly recommend to add this function though.
> 
> Removed as requested.

I hope I'm mistaken sensing hostility here, as I'm only trying to help.
Can you upload your revised code? I'm happy take a look and reconsider.

[Mr4D (OOO till 08-26), 2013-05-21 18:44:28]

That was not meant to be hostile in any way. Really :).

I did upload the patch (had even still the command buffer in my shell and the
output in my buffer - not sure what git was doing there). I uploaded it again so
that you can have a look.

[Mr4D (OOO till 08-26), 2013-05-21 19:23:33]

Addressed & uploaded (I checked :) ). Please have another look!

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:109: // Offsetting the popup message relativ to the
tray menu.
On 2013/05/21 18:03:52, Nikita Kostylev wrote:
> nit: relative

Done.

https://codereview.chromium.org/15329005/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:313: // The anchor view for the "all users are
already assigned" message.
Good point. Done.

[msw, 2013-05-21 19:41:40]

This set_arrow_offset approach seems fine, thanks!

[msw, 2013-05-21 20:31:03]

On 2013/05/21 19:41:40, msw wrote:
> This set_arrow_offset approach seems fine, thanks!

Thanks for resolving my bubble API comment, that bit LGTM.
I'll defer to the other reviewers for a full review.

[James Cook, 2013-05-22 12:29:07]

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc
File ash/popup_message.cc (right):

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:22: const int kMessageAppearanceDelay = 200;  // msec
nit: Consider kMessageAppearanceDelayMs, then you don't need the comment

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:27: // ash/Message/Message_controller.cc
Message/Message -> message/message

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:61: // if not 0.
two space indent

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:68: const base::string16& caption,
indent

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:100: // Add the icon to the first column (if there is one).
optional nit: This class could use a little ASCII-art drawing of which item goes
where.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:203: void PopupMessage::CancelHidingAnimation() {
Do you think this would be more clear with a single Close(bool animate)
function?  Or a CloseImpl(bool animate)?  It feels a little weird to set up a
hide animation in the constructor, then cancel it.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newcode1
ash/popup_message.h:1: // Copyright (c) 2013 The Chromium Authors. All rights
reserved.
nit: new code does not need the "(c)" in the copyright notice.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:13: #include "ui/views/bubble/bubble_delegate.h"
Is this include needed?

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:35: // arrow offset in pixels from the border.
Hooray for detailed function comments!

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:64: 
only one blank line needed

[Mr4D (OOO till 08-26), 2013-05-22 14:57:34]

Done! Please have another look!

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc
File ash/popup_message.cc (right):

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:22: const int kMessageAppearanceDelay = 200;  // msec
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> nit: Consider kMessageAppearanceDelayMs, then you don't need the comment

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:27: // ash/Message/Message_controller.cc
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> Message/Message -> message/message

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:61: // if not 0.
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> two space indent

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:68: const base::string16& caption,
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> indent

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:100: // Add the icon to the first column (if there is one).
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> optional nit: This class could use a little ASCII-art drawing of which item
goes
> where.

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.cc#newc...
ash/popup_message.cc:203: void PopupMessage::CancelHidingAnimation() {
Well... 
a. I have found this somewhere else and took the same bahavior.
b. Since this cannot be called directly but is called from the destructor this
seems fine.
As such the way to do it is: Call close if you want an animation or destroy
immediately (without close) to immediately let it go away. So maybe Close()
should get renamed to AnimatedClose() - but even then it is still not clear that
when the destructor gets called the animation will not be called and or
continues to run. 
Adding a parameter to Close however would not make this any clearer since the
cancelling takes place on destruction (if close wasn't called before). 

Since this was done this way in another place I felt obliged to keep it this
way, but if you feel strongly I could can it.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newcode1
ash/popup_message.h:1: // Copyright (c) 2013 The Chromium Authors. All rights
reserved.
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> nit: new code does not need the "(c)" in the copyright notice.

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:13: #include "ui/views/bubble/bubble_delegate.h"
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> Is this include needed?

Done.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:35: // arrow offset in pixels from the border.
You are welcome! Added also the layout with what the parameters actually mean.

https://codereview.chromium.org/15329005/diff/31001/ash/popup_message.h#newco...
ash/popup_message.h:64: 
On 2013/05/22 12:29:07, James Cook (Chromium) wrote:
> only one blank line needed

Done.

[James Cook, 2013-05-22 15:54:58]

LGTM

https://codereview.chromium.org/15329005/diff/41001/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/41001/ash/popup_message.h#newco...
ash/popup_message.h:36: // Here is the layout (arrow given as TOP_LEFT):
Nice picture!

[Mr4D (OOO till 08-26), 2013-05-22 16:12:01]

Thanks!

https://codereview.chromium.org/15329005/diff/41001/ash/popup_message.h
File ash/popup_message.h (right):

https://codereview.chromium.org/15329005/diff/41001/ash/popup_message.h#newco...
ash/popup_message.h:36: // Here is the layout (arrow given as TOP_LEFT):
You asked for it and it makes a lot of sense. Will add more if I come across
another one.

[commit-bot: I haz the power, 2013-05-22 16:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/15329005/41001

[commit-bot: I haz the power, 2013-05-22 16:37:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-22 18:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/15329005/48002

[commit-bot: I haz the power, 2013-05-22 23:05:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/15329005/48002

[commit-bot: I haz the power, 2013-05-22 23:06:57]

Change committed as 201627