views: Add a new ctor to MessageBoxView that takes only a InitParams.

views: Add a new ctor to MessageBoxView that takes only a InitParams.

This simplifies many consumers of this API.

R=sky@chromium.org

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

[sky, 2012-05-10 15:38:52]

http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
File ui/views/controls/message_box_view.h (right):

http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
ui/views/controls/message_box_view.h:40: MessageBoxView(const string16&
message);
Is there really a compelling reason to do this? I would prefer a single
constructor.

[tfarina, 2012-05-10 15:42:36]

http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
File ui/views/controls/message_box_view.h (right):

http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
ui/views/controls/message_box_view.h:40: MessageBoxView(const string16&
message);
On 2012/05/10 15:38:53, sky wrote:
> Is there really a compelling reason to do this? I would prefer a single
> constructor.
No. Besides the cleanness in the caller side. But if you don't want this, OK!

[tfarina, 2012-05-10 15:49:42]

On Thu, May 10, 2012 at 12:42 PM,  <tfarina@chromium.org> wrote:
>
>
http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
> File ui/views/controls/message_box_view.h (right):
>
>
http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
> ui/views/controls/message_box_view.h:40: MessageBoxView(const string16&
> message);
> On 2012/05/10 15:38:53, sky wrote:
>>
>> Is there really a compelling reason to do this? I would prefer a
>
> single
>>
>> constructor.
>
A single constructor?

You prefer this verbose: new
views::MessageBoxView(views::MessageBoxView::NO_OPTIONS, message,
string16(), kWidth); instead of views::MessageBoxView(message)?

> No. Besides the cleanness in the caller side. But if you don't want
> this, OK!
>
> http://codereview.chromium.org/10378086/



-- 
Thiago

[sky, 2012-05-10 16:04:11]

Here's what the style guide says:

"Use overloaded functions (including constructors) only if a reader
looking at a call site can get a good idea of what is happening
without having to first figure out exactly which overload is being
called."

I definitely prefer MessageBoxView("foo"), but it isn't clear from
that what the options are going to be.

  -Scott

On Thu, May 10, 2012 at 8:49 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 10, 2012 at 12:42 PM,  <tfarina@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
>> File ui/views/controls/message_box_view.h (right):
>>
>>
http://codereview.chromium.org/10378086/diff/2002/ui/views/controls/message_b...
>> ui/views/controls/message_box_view.h:40: MessageBoxView(const string16&
>> message);
>> On 2012/05/10 15:38:53, sky wrote:
>>>
>>> Is there really a compelling reason to do this? I would prefer a
>>
>> single
>>>
>>> constructor.
>>
> A single constructor?
>
> You prefer this verbose: new
> views::MessageBoxView(views::MessageBoxView::NO_OPTIONS, message,
> string16(), kWidth); instead of views::MessageBoxView(message)?
>
>> No. Besides the cleanness in the caller side. But if you don't want
>> this, OK!
>>
>> http://codereview.chromium.org/10378086/
>
>
>
> --
> Thiago

[tfarina, 2012-05-10 16:22:25]

On Thu, May 10, 2012 at 1:04 PM, Scott Violet <sky@chromium.org> wrote:
> Here's what the style guide says:
>
> "Use overloaded functions (including constructors) only if a reader
> looking at a call site can get a good idea of what is happening
> without having to first figure out exactly which overload is being
> called."
>
> I definitely prefer MessageBoxView("foo"), but it isn't clear from
> that what the options are going to be.
>
I'm thinking here...

Would you be fine with this approach:

- Only one constructor that takes the message.
- Setter for the other parameters (SetOptions, SetDefaultPrompt,
SetMessageWidth).

?

Would that make things worse?

-- 
Thiago

[sky, 2012-05-10 22:07:14]

On Thu, May 10, 2012 at 9:22 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 10, 2012 at 1:04 PM, Scott Violet <sky@chromium.org> wrote:
>> Here's what the style guide says:
>>
>> "Use overloaded functions (including constructors) only if a reader
>> looking at a call site can get a good idea of what is happening
>> without having to first figure out exactly which overload is being
>> called."
>>
>> I definitely prefer MessageBoxView("foo"), but it isn't clear from
>> that what the options are going to be.
>>
> I'm thinking here...
>
> Would you be fine with this approach:
>
> - Only one constructor that takes the message.
> - Setter for the other parameters (SetOptions, SetDefaultPrompt,
> SetMessageWidth).
>
> ?
>
> Would that make things worse?

That still requires you to know the defaults right? Which is the
motivation for the style guide saying don't do this. That said, if
nearly all callers only care about the message and want the default
than I'm ok with a new constructor.

  -Scott

[tfarina, 2012-05-10 22:25:15]

On Thu, May 10, 2012 at 7:07 PM, Scott Violet <sky@chromium.org> wrote:
> That still requires you to know the defaults right?
Right.

> Which is the motivation for the style guide saying don't do this. That said,
if
> nearly all callers only care about the message and want the default
> than I'm ok with a new constructor.
>

The only caller that cares about the options parameter is
chrome/browser/ui/views/js_modal_dialog_views.cc. All the other
callers call MessageBoxView with NO_OPTIONS.

In two places, update_recommended_message_box.cc and
user_data_dir_dialog.cc, they call the second constructor to pass
their custom dialog width.

And one more thing is that js_modal_dialog_views.cc is the only
consumer that passes a default_prompt string to MessageBoxView.

I'm not sure how to improve this situation yet. As you raised the
concern about overloading the constructor again, I'm not sure if it's
still worth.

-- 
Thiago

[sky, 2012-05-11 15:33:45]

On Thu, May 10, 2012 at 3:25 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 10, 2012 at 7:07 PM, Scott Violet <sky@chromium.org> wrote:
>> That still requires you to know the defaults right?
> Right.
>
>> Which is the motivation for the style guide saying don't do this. That said,
if
>> nearly all callers only care about the message and want the default
>> than I'm ok with a new constructor.
>>
>
> The only caller that cares about the options parameter is
> chrome/browser/ui/views/js_modal_dialog_views.cc. All the other
> callers call MessageBoxView with NO_OPTIONS.
>
> In two places, update_recommended_message_box.cc and
> user_data_dir_dialog.cc, they call the second constructor to pass
> their custom dialog width.
>
> And one more thing is that js_modal_dialog_views.cc is the only
> consumer that passes a default_prompt string to MessageBoxView.
>
> I'm not sure how to improve this situation yet. As you raised the
> concern about overloading the constructor again, I'm not sure if it's
> still worth.

All of the fields are used from the constructor, so it's a bit hard to
provide setters. I'm OK with creating some struct like:

struct InitParams {
  int options;
  ...
};

and a single constructor that takes the string. The common case becomes:

MessageBoxView(MessageBoxView::InitParams(message));

uncommon:
MessageBoxView::InitParams params(message);
params.foo=bar;
MessageBoxView(params);

  -Scott

[tfarina, 2012-05-11 16:53:34]

InitParams

[tfarina, 2012-05-11 16:59:17]

On 2012/05/11 15:33:45, sky wrote:
> All of the fields are used from the constructor, so it's a bit hard to
> provide setters. I'm OK with creating some struct like:
> 
> struct InitParams {
>   int options;
>   ...
> };
> 
> and a single constructor that takes the string. The common case becomes:
> 
> MessageBoxView(MessageBoxView::InitParams(message));
> 
> uncommon:
> MessageBoxView::InitParams params(message);
> params.foo=bar;
> MessageBoxView(params);
> 
I made these changes. Please, take another look.

[tfarina, 2012-05-11 17:04:47]

InitParams

[sky, 2012-05-11 17:31:10]

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
File ui/views/controls/message_box_view.cc (right):

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.cc:110: Init(params.options, params.message,
params.default_prompt);
Pass InitParams to Init.

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
File ui/views/controls/message_box_view.h (right):

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.h:41: InitParams(const string16& message);
Explicit and add a destructor.

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.h:49: MessageBoxView(const InitParams&
params);
explicit

[tfarina, 2012-05-11 17:40:44]

fix review

[tfarina, 2012-05-11 17:41:41]

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
File ui/views/controls/message_box_view.cc (right):

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.cc:110: Init(params.options, params.message,
params.default_prompt);
On 2012/05/11 17:31:10, sky wrote:
> Pass InitParams to Init.

Done.

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
File ui/views/controls/message_box_view.h (right):

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.h:41: InitParams(const string16& message);
On 2012/05/11 17:31:10, sky wrote:
> Explicit and add a destructor.

Done.

https://chromiumcodereview.appspot.com/10378086/diff/11/ui/views/controls/mes...
ui/views/controls/message_box_view.h:49: MessageBoxView(const InitParams&
params);
On 2012/05/11 17:31:10, sky wrote:
> explicit

Done.

[sky, 2012-05-11 19:00:35]

LGTM