The new toast experiments.

The new toast experiments.

I've done some generalizing of the code and added the bits to show the new
UI designs. Also fixed some bugs. Gone is the idea of 'compact' toast and now
we just specify the flags we want.

BUG=129499
TEST=see bug
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149257

[cpu_(ooo_6.6-7.5), 2012-07-25 19:56:31]

Mark, look at kExperiments[] in google_chrome_distribution.cc is a reasonable
way to specify an experiment. I changed the format a lot.

Finnur, the rest, also start at google_chrome_distribution.cc grokking the
experiment format change will make it easier to parse the change.

[Finnur, 2012-07-26 12:27:06]

I would love to see some screenshots of the bubble. Maybe update the bug?

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:115: // Optional second row:
[pad][pad][radio 1].
Can you add a line break above this and on lines 121, 127, 135, 141, 145 and
152. Thank you for your cooperation.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:202: // The compact bubble
has, as its second radio button, "Don't bug me".
This comment refers to the compact bubble. Do we want that? Hmm, there might be
other references too...

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:222:
make_default_->SetChecked(true);
nit: For consistency with line 190 maybe move this up one line (to be above
AddView).

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:333: if
(make_default_->checked())
nit: collapse 332 and 333 into one line and loose the braces.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
File chrome/browser/first_run/try_chrome_dialog_view.h (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.h:38: //  
+-----------------------------------------------+
Is this diagram still up to date? If not, can you update it?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/brows...
File chrome/installer/util/browser_distribution.h (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/brows...
chrome/installer/util/browser_distribution.h:53: bool make_default;    //
Whether to show a checkbox to make chrome default.
If you have the flags, why do you need the bool?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:67: const wchar_t
kToastExpTriesOkDefaultGroup[] = L"28";
This would change the result code that gets sent back to the server. Are you
sure it is OK to change the error code from 28 to 48 and reuse 28 for another
purpose?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:630: };
This struct should probably move to line 617, as the comment there no longer
describes the struct it was intended to describe...

Also, document it?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:642: { L"*", L"CHMA",  //
all locales, CHMA brand.
nit: Capitalize first letter of comments in this line and below.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:646: { {
IDS_TRY_TOAST_HEADING3, kDontBugMeAsButton | kUninstall | kWhyLink },
I thought we'd decided not to have a Why link, since it doesn't actually work
(Chrome startup is halted at the toast dialog and pressing the link will launch
a new process that will also halt until you dismiss the dialog). No?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:647: {
IDS_TRY_TOAST_HEADING3, 0 },
I'm confused about what these enums mean. What does 0 mean for example?  Is it
TryMe and Don'tBugMe?

Also, the Why link isn't being requested in the bug?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:654: L"EA",          //
experiment is EAxx, EBxx, etc.
Just curious... There's no experiment, so why have an experiment code?

[Mark Larson, 2012-07-26 16:32:13]

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:67: const wchar_t
kToastExpTriesOkDefaultGroup[] = L"28";
I'm also concerned about how this will show up on the backend. I think it's
probably OK, but it might make more sense to swap 28 and 48 for backward
compatibility. Is there a reason to change the meaning of 28?

BTW, you don't seem to use TriesOKDefaultGroup anywhere else.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:838: // The chrome process
has exited, figure out what happened.
Don't we need something here that sets outcome to kToastExpTriesOkDefaultGroup?

[cpu_(ooo_6.6-7.5), 2012-07-27 03:06:23]

please take a look again.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:115: // Optional second row:
[pad][pad][radio 1].
On 2012/07/26 12:27:07, Finnur wrote:
> Can you add a line break above this and on lines 121, 127, 135, 141, 145 and
> 152. Thank you for your cooperation.

Done.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:202: // The compact bubble
has, as its second radio button, "Don't bug me".
On 2012/07/26 12:27:07, Finnur wrote:
> This comment refers to the compact bubble. Do we want that? Hmm, there might
be
> other references too...

Reworded the comments mentioning compact and regular bubble.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:222:
make_default_->SetChecked(true);
On 2012/07/26 12:27:07, Finnur wrote:
> nit: For consistency with line 190 maybe move this up one line (to be above
> AddView).

Done.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.cc:333: if
(make_default_->checked())
On 2012/07/26 12:27:07, Finnur wrote:
> nit: collapse 332 and 333 into one line and loose the braces.

Done.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
File chrome/browser/first_run/try_chrome_dialog_view.h (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.h:38: //  
+-----------------------------------------------+
On 2012/07/26 12:27:07, Finnur wrote:
> Is this diagram still up to date? If not, can you update it?

The wording has changed but otherwise it is flavor = 0. I updated the strings
and did a second flavor. Seems impractical to do all possible dialogs though.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/brows...
File chrome/installer/util/browser_distribution.h (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/brows...
chrome/installer/util/browser_distribution.h:53: bool make_default;    //
Whether to show a checkbox to make chrome default.
On 2012/07/26 12:27:07, Finnur wrote:
> If you have the flags, why do you need the bool?

Good catch. done.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:67: const wchar_t
kToastExpTriesOkDefaultGroup[] = L"28";
On 2012/07/26 12:27:07, Finnur wrote:
> This would change the result code that gets sent back to the server. Are you
> sure it is OK to change the error code from 28 to 48 and reuse 28 for another
> purpose?

Done.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:642: { L"*", L"CHMA",  //
all locales, CHMA brand.
On 2012/07/26 12:27:07, Finnur wrote:
> nit: Capitalize first letter of comments in this line and below.

Done.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:646: { {
IDS_TRY_TOAST_HEADING3, kDontBugMeAsButton | kUninstall | kWhyLink },
No idea. Can you please raise the issue in the bug?

On 2012/07/26 12:27:07, Finnur wrote:
> I thought we'd decided not to have a Why link, since it doesn't actually work
> (Chrome startup is halted at the toast dialog and pressing the link will
launch
> a new process that will also halt until you dismiss the dialog). No?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:647: {
IDS_TRY_TOAST_HEADING3, 0 },
On 2012/07/26 12:27:07, Finnur wrote:
> I'm confused about what these enums mean. What does 0 mean for example?  Is it
> TryMe and Don'tBugMe?

it means don't bug me is a radio button, because the flag just controls how is
shown, and no uninstall or why link. Zero gives you the second ascii art example
in the header :)


> 
> Also, the Why link isn't being requested in the bug?

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:654: L"EA",          //
experiment is EAxx, EBxx, etc.
On 2012/07/26 12:27:07, Finnur wrote:
> Just curious... There's no experiment, so why have an experiment code?

No very good reason: In case we f*ck up we can see in the omaha dashboard. We
would see EAxx, EBxx.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:838: // The chrome process
has exited, figure out what happened.
On 2012/07/26 16:32:13, Mark Larson wrote:
> Don't we need something here that sets outcome to
kToastExpTriesOkDefaultGroup?

Yes, I am trying to figure how to do this. Stay tuned for a followup change.

[Finnur, 2012-07-29 19:09:44]

Looks like you wanted to do more on this list, so I'll hold off on giving a
green light.

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
File chrome/browser/first_run/try_chrome_dialog_view.h (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/browser/first_run/tr...
chrome/browser/first_run/try_chrome_dialog_view.h:38: //  
+-----------------------------------------------+
That's fine. One would have been enough.

On 2012/07/27 03:06:23, cpu wrote:
> On 2012/07/26 12:27:07, Finnur wrote:
> > Is this diagram still up to date? If not, can you update it?
> 
> The wording has changed but otherwise it is flavor = 0. I updated the strings
> and did a second flavor. Seems impractical to do all possible dialogs though.

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:646: { {
IDS_TRY_TOAST_HEADING3, kDontBugMeAsButton | kUninstall | kWhyLink },
OK, I refreshed my memory by looking up the bug...
http://code.google.com/p/chromium/issues/detail?id=110672
The consensus was that we should not show the link if Chrome is the default
browser...

On 2012/07/27 03:06:23, cpu wrote:
> No idea. Can you please raise the issue in the bug?
> 
> On 2012/07/26 12:27:07, Finnur wrote:
> > I thought we'd decided not to have a Why link, since it doesn't actually
work
> > (Chrome startup is halted at the toast dialog and pressing the link will
> launch
> > a new process that will also halt until you dismiss the dialog). No?
>

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:654: L"EA",          //
experiment is EAxx, EBxx, etc.
Fair enough. Consider documenting on line 624?

On 2012/07/27 03:06:23, cpu wrote:
> On 2012/07/26 12:27:07, Finnur wrote:
> > Just curious... There's no experiment, so why have an experiment code?
> 
> No very good reason: In case we f*ck up we can see in the omaha dashboard. We
> would see EAxx, EBxx.

http://codereview.chromium.org/10821007/diff/12001/chrome/installer/util/goog...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/12001/chrome/installer/util/goog...
chrome/installer/util/google_chrome_distribution.cc:68: const wchar_t
kToastExpTriesOkDefaultGroup[] = L"48";
nit: This was ordered by error code but is no longer. Is that intentional? Up to
you.

[Finnur, 2012-07-29 19:10:08]

s/more on this list/more on this changelist/ 
:)

[cpu_(ooo_6.6-7.5), 2012-07-31 04:04:09]

Couple of changes since last seen:

1- Added italics to the checkbox
2- Added the missing kToastExpTriesOkDefaultGroup that Larson noted.

The thing that mal@ and I ended up with is not setting the
kToastExpTriesOkDefaultGroup if the user set chrome as default but set it if
that was the experiment he/she was drafted into (regardless if he/she uncheck
the box), it is simpler and more in line with the rest of the other measures.

That is it. No more changes :)

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:654: L"EA",          //
experiment is EAxx, EBxx, etc.
Others missing. I'll change here after I talk to larson,or he will document.

On 2012/07/29 19:09:44, Finnur wrote:
> Fair enough. Consider documenting on line 624?
> 
> On 2012/07/27 03:06:23, cpu wrote:
> > On 2012/07/26 12:27:07, Finnur wrote:
> > > Just curious... There's no experiment, so why have an experiment code?
> > 
> > No very good reason: In case we f*ck up we can see in the omaha dashboard.
We
> > would see EAxx, EBxx.
>

http://codereview.chromium.org/10821007/diff/1008/chrome/installer/util/googl...
chrome/installer/util/google_chrome_distribution.cc:838: // The chrome process
has exited, figure out what happened.
On 2012/07/27 03:06:23, cpu wrote:
> On 2012/07/26 16:32:13, Mark Larson wrote:
> > Don't we need something here that sets outcome to
> kToastExpTriesOkDefaultGroup?
> 
> Yes, I am trying to figure how to do this. Stay tuned for a followup change.

Done.

http://codereview.chromium.org/10821007/diff/12001/chrome/installer/util/goog...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/12001/chrome/installer/util/goog...
chrome/installer/util/google_chrome_distribution.cc:68: const wchar_t
kToastExpTriesOkDefaultGroup[] = L"48";
Intentional, I want to order by bit pattern instead.

On 2012/07/29 19:09:44, Finnur wrote:
> nit: This was ordered by error code but is no longer. Is that intentional? Up
to
> you.

[Finnur, 2012-07-31 07:26:27]

LGTM, one nit.

http://codereview.chromium.org/10821007/diff/14002/chrome/installer/util/goog...
File chrome/installer/util/google_chrome_distribution.cc (right):

http://codereview.chromium.org/10821007/diff/14002/chrome/installer/util/goog...
chrome/installer/util/google_chrome_distribution.cc:861: kToastExpTriesOkGroup;
nit: You need braces here (two line if clause).