For the Metro mode infobar, move various variables from the caller side to the

For the Metro mode infobar, move various variables from the caller side to the
infobar side.  This will make a subsequent infobar creation change patch I'm
working on much easier.  Besides, it's not really important for the caller to
know these sorts of low-level details.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174640

[Bernhard Bauer, 2012-12-22 01:55:53]

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_infobar_delegates.cc (left):

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_infobar_delegates.cc:489:
DCHECK(show_dont_ask_again_button_);
Is there a reason you removed this DCHECK? I kinda like documenting assumptions
this way.

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_infobar_delegates.cc:462: return (mode_ ==
MISSING_PLUGIN) ? BUTTON_OK : (BUTTON_OK | BUTTON_CANCEL);
I think making these into switch statements would look clearer.

[Peter Kasting, 2012-12-26 18:40:36]

Is this LGTM, or should I make further changes?

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_infobar_delegates.cc (left):

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_infobar_delegates.cc:489:
DCHECK(show_dont_ask_again_button_);
On 2012/12/22 01:55:54, Bernhard Bauer wrote:
> Is there a reason you removed this DCHECK? I kinda like documenting
assumptions
> this way.

I removed this because I killed the member variable.

I could put it back as a check of the mode variable.  The only potential issue
then is that it makes the code somewhat more brittle, in that if we start
wanting to show the cancel button in both modes for some reason the check will
just get in the way.

I put the check back on the assumption you'd prefer that way.

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_infobar_delegates.cc:462: return (mode_ ==
MISSING_PLUGIN) ? BUTTON_OK : (BUTTON_OK | BUTTON_CANCEL);
On 2012/12/22 01:55:54, Bernhard Bauer wrote:
> I think making these into switch statements would look clearer.

I'm not a huge fan of switches for a two-element enum (which is already just an
enum instead of a simple bool so that the states can be named more clearly).  It
expands one line of code here into seven:

  switch (mode_) {
    case MISSING_PLUGIN:
      return BUTTON_OK;

    case DESKTOP_MODE_REQUIRED:
      return BUTTON_OK | BUTTON_CANCEL;
  }

It just feels so verbose :(

[Bernhard Bauer, 2012-12-26 22:38:58]

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/11667015/diff/1/chrome/browser/plugins/plugin...
chrome/browser/plugins/plugin_infobar_delegates.cc:462: return (mode_ ==
MISSING_PLUGIN) ? BUTTON_OK : (BUTTON_OK | BUTTON_CANCEL);
On 2012/12/26 18:40:36, Peter Kasting wrote:
> On 2012/12/22 01:55:54, Bernhard Bauer wrote:
> > I think making these into switch statements would look clearer.
> 
> I'm not a huge fan of switches for a two-element enum (which is already just
an
> enum instead of a simple bool so that the states can be named more clearly). 
It
> expands one line of code here into seven:
> 
>   switch (mode_) {
>     case MISSING_PLUGIN:
>       return BUTTON_OK;
> 
>     case DESKTOP_MODE_REQUIRED:
>       return BUTTON_OK | BUTTON_CANCEL;
>   }
> 
> It just feels so verbose :(

I know what you mean, but the same argument could probably be made for having an
enum instead of a boolean flag. I guess that mismatch between having an enum
that could potentially have an arbitrary number of states, and handling one
single condition, is what feels weird to me. But if you feel strongly about it,
the current state LGTM.

[Peter Kasting, 2012-12-26 22:44:41]

On 2012/12/26 22:38:58, Bernhard Bauer wrote:
> I guess that mismatch between having an enum
> that could potentially have an arbitrary number of states, and handling one
> single condition, is what feels weird to me. But if you feel strongly about
it,
> the current state LGTM.

I don't feel super-strongly.  There is precedent in the rest of the infobar
system -- we're very consistent about using ?: for things like GetButtonLabel(),
and assuming the button enum only has two buttons in it.  Doing something
similar here is more consistent with the rest of the file and other infobar
code.

I guess in the interests of just making a decision I'll go ahead and check in,
but if you want to refactor the code and change it in the future I'll try to
remember not to whine about it :).

[Bernhard Bauer, 2012-12-26 22:46:25]

On 2012/12/26 22:44:41, Peter Kasting wrote:
> On 2012/12/26 22:38:58, Bernhard Bauer wrote:
> > I guess that mismatch between having an enum
> > that could potentially have an arbitrary number of states, and handling one
> > single condition, is what feels weird to me. But if you feel strongly about
> it,
> > the current state LGTM.
> 
> I don't feel super-strongly.  There is precedent in the rest of the infobar
> system -- we're very consistent about using ?: for things like
GetButtonLabel(),
> and assuming the button enum only has two buttons in it.  Doing something
> similar here is more consistent with the rest of the file and other infobar
> code.
> 
> I guess in the interests of just making a decision I'll go ahead and check in,
> but if you want to refactor the code and change it in the future I'll try to
> remember not to whine about it :).

Deal :)