views: Add an Options enum to MessageBoxView control.

views: Add an Options enum to MessageBoxView control.

R=sky@chromium.org
TBR=jam@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113331

[tfarina, 2011-11-17 20:23:11]

Scott, could you review this to me?

Thanks!

[sky, 2011-11-18 17:17:44]

Where are you hoping to go with these changes?

[tfarina, 2011-11-18 17:24:23]

On 2011/11/18 17:17:44, sky wrote:
> Where are you hoping to go with these changes?

I'm trying to have a better code without these ugly MessageBoxFlags api.

What is wrong with this change now?

[sky, 2011-11-21 15:33:45]

On 2011/11/18 17:24:23, tfarina wrote:
> On 2011/11/18 17:17:44, sky wrote:
> > Where are you hoping to go with these changes?
> 
> I'm trying to have a better code without these ugly MessageBoxFlags api.
> 
> What is wrong with this change now?

Does that mean you hope to remove MessageBoxFlags entirely in the long run?

[tfarina, 2011-11-23 14:43:13]

On 2011/11/21 15:33:45, sky wrote:
> On 2011/11/18 17:24:23, tfarina wrote:
> > On 2011/11/18 17:17:44, sky wrote:
> > > Where are you hoping to go with these changes?
> > 
> > I'm trying to have a better code without these ugly MessageBoxFlags api.
> > 
> > What is wrong with this change now?
> 
> Does that mean you hope to remove MessageBoxFlags entirely in the long run?

Yes, that is the big picture I want to archive.

[sky, 2011-11-29 01:22:23]

Aren't MessageBoxFlags used in cross platform code in a couple of places?

[tfarina, 2011-11-29 11:52:01]

On Mon, Nov 28, 2011 at 11:22 PM,  <sky@chromium.org> wrote:
> Aren't MessageBoxFlags used in cross platform code in a couple of places?
>

After this patch, MessageBoxFlags is going to be used only by
JSModalDialog API, so we can do better.

-- 
Thiago

[sky, 2011-11-29 16:46:30]

Maybe you should do those changes first so I can see where you're headed.

  -Scott

On Tue, Nov 29, 2011 at 3:51 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Mon, Nov 28, 2011 at 11:22 PM,  <sky@chromium.org> wrote:
>> Aren't MessageBoxFlags used in cross platform code in a couple of places?
>>
>
> After this patch, MessageBoxFlags is going to be used only by
> JSModalDialog API, so we can do better.
>
> --
> Thiago

[tfarina, 2011-11-29 20:19:04]

On 2011/11/29 16:46:30, sky wrote:
> Maybe you should do those changes first so I can see where you're headed.
> 
Thanks for pushing me to the limit!

Please, could you review patch set 10?

[sky, 2011-11-30 22:43:11]

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_flags.h
File ui/base/message_box_flags.h (left):

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_flags.h...
ui/base/message_box_flags.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
How come this file isn't being shown as deleted?

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h
File ui/base/message_box_type.h (right):

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h#...
ui/base/message_box_type.h:11: enum MessageBoxType {
This name sounds overly generic for what this enum is now. Maybe
JavaScriptMessageType?

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h#...
ui/base/message_box_type.h:12: MESSAGE_BOX_TYPE_JAVASCRIPT_ALERT   = 0,
Why the = here? Doesn't seem like you need explicit values.

http://codereview.chromium.org/8553001/diff/35026/views/controls/message_box_...
File views/controls/message_box_view.h (right):

http://codereview.chromium.org/8553001/diff/35026/views/controls/message_box_...
views/controls/message_box_view.h:27: NONE = 0,
How about NO_OPTIONS or something to make it more readable in other places.

[tfarina, 2011-12-01 12:43:26]

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_flags.h
File ui/base/message_box_flags.h (left):

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_flags.h...
ui/base/message_box_flags.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2011/11/30 22:43:12, sky wrote:
> How come this file isn't being shown as deleted?

Seems to be a bug with rietveld. maruel should know better.

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h
File ui/base/message_box_type.h (right):

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h#...
ui/base/message_box_type.h:11: enum MessageBoxType {
On 2011/11/30 22:43:12, sky wrote:
> This name sounds overly generic for what this enum is now. Maybe
> JavaScriptMessageType?

Done.

http://codereview.chromium.org/8553001/diff/35026/ui/base/message_box_type.h#...
ui/base/message_box_type.h:12: MESSAGE_BOX_TYPE_JAVASCRIPT_ALERT   = 0,
On 2011/11/30 22:43:12, sky wrote:
> Why the = here? Doesn't seem like you need explicit values.

Done.

http://codereview.chromium.org/8553001/diff/35026/views/controls/message_box_...
File views/controls/message_box_view.h (right):

http://codereview.chromium.org/8553001/diff/35026/views/controls/message_box_...
views/controls/message_box_view.h:27: NONE = 0,
On 2011/11/30 22:43:12, sky wrote:
> How about NO_OPTIONS or something to make it more readable in other places.

Done.

[sky, 2011-12-01 16:42:02]

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:88:
ui::JavascriptMessageType javascript_message_type_;
const

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
File chrome/browser/ui/app_modal_dialogs/native_app_modal_dialog.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
chrome/browser/ui/app_modal_dialogs/native_app_modal_dialog.h:17: virtual int
GetAppModalDialogButtons() const = 0;
This comment is wrong. Isn't this returning DialogButtons? I also think this
method is wrong. Implementations are returning DialogButtons, which doesn't work
as a bitmask. This needs to be straightened out.

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/views/sim...
File chrome/browser/ui/views/simple_message_box_views.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/views/sim...
chrome/browser/ui/views/simple_message_box_views.h:88: DialogType type_;
const

http://codereview.chromium.org/8553001/diff/42001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8553001/diff/42001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:1065:
static_cast<ui::JavascriptMessageType>(flags),
Use a param trait so that you don't have to cast.

[tfarina, 2011-12-01 17:52:58]

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
File chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
chrome/browser/ui/app_modal_dialogs/js_modal_dialog.h:88:
ui::JavascriptMessageType javascript_message_type_;
On 2011/12/01 16:42:02, sky wrote:
> const

Done.

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
File chrome/browser/ui/app_modal_dialogs/native_app_modal_dialog.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/app_modal...
chrome/browser/ui/app_modal_dialogs/native_app_modal_dialog.h:17: virtual int
GetAppModalDialogButtons() const = 0;
On 2011/12/01 16:42:02, sky wrote:
> This comment is wrong. Isn't this returning DialogButtons? I also think this
> method is wrong. Implementations are returning DialogButtons, which doesn't
work
> as a bitmask.

Right. Comment reworded.

Although it is working because:

- The values of the enum constants are in the range 0-2, which means in binary
(0, 1, 10), which is the same as 0, 1 << 0, 1 << 1. The thing would become wrong
if a fourth constant would be added to the enum with the value 3 which in binary
is 11, instead of 1 << 2 (4 in decimal and 100 in binary).

I can change the constants in DialogButton to 1 << 0, and 1 << 1, if you think
this is the best solution.
 
> This needs to be straightened out.

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/views/sim...
File chrome/browser/ui/views/simple_message_box_views.h (right):

http://codereview.chromium.org/8553001/diff/42001/chrome/browser/ui/views/sim...
chrome/browser/ui/views/simple_message_box_views.h:88: DialogType type_;
On 2011/12/01 16:42:02, sky wrote:
> const

Done.

http://codereview.chromium.org/8553001/diff/42001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8553001/diff/42001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:1065:
static_cast<ui::JavascriptMessageType>(flags),
On 2011/12/01 16:42:02, sky wrote:
> Use a param trait so that you don't have to cast.

Done.

[sky, 2011-12-02 16:48:14]

You sure you don't need to modify common_param_traits?

[tfarina, 2011-12-02 17:01:16]

On 2011/12/02 16:48:14, sky wrote:
> You sure you don't need to modify common_param_traits?

Doesn't look like.

I asked John, and he said to use IPC_ENUM_TRAITS, which I'm using in
content/common/view_messages.h line 71.

[sky, 2011-12-02 17:53:53]

LGTM

[tfarina, 2011-12-06 18:36:35]

+Thomas, for the ipc fix!

[Tom Sepez, 2011-12-06 19:49:41]

LGTM for ipc change.