Create a field test for sync sign in promo strings.

Create a field test for sync sign in promo strings.

BUG=105542
TEST=Start a new user so you see the sync promo page. Sign into Sync. Check about:histograms and ensure that SyncPromo.MessageDisplayed and SyncPromo.MessageOnSignIn both have the same enum value recorded.

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

[SteveT, 2011-11-28 16:17:31]

Hey Sail,

Here's my patch so far for the SyncPromo experiment. I have a few more details
to clarify (hence one remaining TODO in my code), but the code is ready for
review. PTAL.

Thanks!


Steve

[sail, 2011-11-28 16:34:25]

Hey Steve, I think this CL looks good so far.

Could you add jhawkins to the review? He knows more about the details of the
sync setup UI.

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_handler.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_handler.cc:183: // Call the original
implementation.
comment should say why you're doing something not what you're doing

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_ui.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.cc:35: const char
kSyncPromoQueryKeyLaunchPage[] = "launch_page";
same, is launch page?

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_ui.h (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.h:48: // Gets the launch page value from
the query portion of the sync promo URL.
"the launch page value" suggests that the return value is a URL or something.
Maybe "Gets the 'is launch page' value"?

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.h:49: static bool
GetLaunchPageForSyncPromoURL(const GURL& url);
Maybe GetIsLaunchPageForSyncPromoURL() instead?

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:387: { "promoMessageBodyA",
IDS_SYNC_PROMO_MESSAGE_BODY_A},
instead of having multiple strings here and then doing a javascript call to
change the string it would be simpler to do something like this:
       SyncPromoUIHTMLSource::SyncPromoUIHTMLSource(WebUI* web_ui) {
         SyncSetupHandler::GetStaticLocalizedValues(&localized_strings)
         SyncPromoHandler::GetStaticLocalizedValues(&localized_strings, web_ui)
       }

       SyncSetupHandler::GetStaticLocalizedValues(DictionaryValue*
localized_strings, WebUI* web_ui) {
                if (IsInExperiment())
                                ...
        }
       }

If you could figure out a way to get rid of GetStaticLocalizedValues() and just
use GetLocalizedValues() then that would be even better!

[SteveT, 2011-11-28 20:07:32]

R+jhawkins - James, could you take a look at the SyncSetupUI and related
changes? I'm also not entirely sure how to get the NTPResourceCache to play
nicely with the changes I made to SyncSetupHandler::GetStaticLocalizedValues
(which now requires a WebUI object to tell us if we have a particular query
param or not).

Sail - All your comments are addressed. PTAL and let me know if there's anything
else we can clean up.

Thanks!


Steve

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_handler.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_handler.cc:183: // Call the original
implementation.
Oops sorry - this was from the remnants of some earlier implementation which is
not needed and removed now.

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_ui.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.cc:35: const char
kSyncPromoQueryKeyLaunchPage[] = "launch_page";
On 2011/11/28 16:34:25, sail wrote:
> same, is launch page?

Done.

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_ui.h (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.h:48: // Gets the launch page value from
the query portion of the sync promo URL.
Yeah that's better. Done.

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_ui.h:49: static bool
GetLaunchPageForSyncPromoURL(const GURL& url);
On 2011/11/28 16:34:25, sail wrote:
> Maybe GetIsLaunchPageForSyncPromoURL() instead?

Done.

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8689006/diff/11003/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:387: { "promoMessageBodyA",
IDS_SYNC_PROMO_MESSAGE_BODY_A},
So I addressed this roughly in the way you suggested. It's not the tidiest
solution since we had to introduce a dependency on a WebUI object, and
NTPResourceCache is not capable of providing it (see my new TODO). Maybe
jhawkins has some suggestions on how to fix this.. plus the use of
GetStaticLocalizedValues.

[sail, 2011-11-28 20:14:02]

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:380:
SyncSetupHandler::GetStaticLocalizedValues(&localized_strings, NULL);
Hm.. this may not be needed anymore. I'll take a look.

[SteveT, 2011-11-28 20:18:54]

My guess is that none of the strings from the Promo bits surface in the NTP
itself, so we might not need this.

The NTP certainly doesn't need my experimental strings, so passing NULL here
doesn't affect my experiment.

[sail, 2011-11-28 20:33:09]

LGTM!

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:380:
SyncSetupHandler::GetStaticLocalizedValues(&localized_strings, NULL);
On 2011/11/28 20:14:02, sail wrote:
> Hm.. this may not be needed anymore. I'll take a look.

Just verified. None of the strings in SyncSetupHandler are used in NTP anymore.
Please remove this entire line.

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_trial.cc:22: const char*
kSyncPromoMsgTrialName = "SyncPromoMsg";
const char kSyncPromoMsgTrialName[] = ...?

[SteveT, 2011-11-28 20:40:59]

R+jeffreyc: Jeff, could you take a look at generated_resources.grd and make sure
the new strings all look good to you?

Sail - Cool. Thanks for going through the trouble of checking those strings!

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:380:
SyncSetupHandler::GetStaticLocalizedValues(&localized_strings, NULL);
On 2011/11/28 20:14:02, sail wrote:
> Hm.. this may not be needed anymore. I'll take a look.

Done.

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_trial.cc (right):

http://codereview.chromium.org/8689006/diff/27002/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_trial.cc:22: const char*
kSyncPromoMsgTrialName = "SyncPromoMsg";
On 2011/11/28 20:33:09, sail wrote:
> const char kSyncPromoMsgTrialName[] = ...?

Done.

[sail, 2011-11-28 20:44:04]

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:311: //
NTPResourceCache::CreateNewTabHTML, which is incapable of passing the
I think you can remove this comment now.

[James Hawkins, 2011-11-28 21:42:09]

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_trial.h (right):

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_trial.h:14: class SyncPromoTrial {
Please get rid of the class and move these methods/defines to a namespace.

[SteveT, 2011-11-28 22:11:15]

Cool, removed the use of a class+statics as you suggested. PTAL.

Jeff - I've also updated String D as per Travis' suggestion in the bug thread.

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_promo_trial.h (right):

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_promo_trial.h:14: class SyncPromoTrial {
On 2011/11/28 21:42:09, James Hawkins wrote:
> Please get rid of the class and move these methods/defines to a namespace.

Done.

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8689006/diff/28016/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:311: //
NTPResourceCache::CreateNewTabHTML, which is incapable of passing the
On 2011/11/28 20:44:04, sail wrote:
> I think you can remove this comment now.

Done.

[James Hawkins, 2011-11-28 22:12:52]

lgtm

[SteveT, 2011-11-28 22:15:08]

Thanks, James!

Jeff - so this is ready to go, pending your approval of the strings, or a
decision to change the experiment content to something other than this
description string (as per Clem's suggestion in our email thread).

[jeffreyc, 2011-11-28 22:58:03]

Let's change "to the web" to "to the cloud" as per the other thread.

Otherwise, LGTM

On Mon, Nov 28, 2011 at 2:15 PM, <stevet@chromium.org> wrote:

> Thanks, James!
>
> Jeff - so this is ready to go, pending your approval of the strings, or a
> decision to change the experiment content to something other than this
> description string (as per Clem's suggestion in our email thread).
>
>
http://codereview.chromium.**org/8689006/<http://codereview.chromium.org/8689...
>

[SteveT, 2011-11-29 15:11:00]

R+clemw,tmccoy,nepper

Jeff's requested change is in.

[SteveT, 2011-11-29 17:34:12]

Clem's change is in. Just waiting on Travis and Patrick for approvals now.

[SteveT, 2011-11-30 17:33:03]

Travis and Patrick - can I ping you guys on this?

[Patrick Nepper, 2011-11-30 21:38:30]

On 2011/11/30 17:33:03, SteveT wrote:
> Travis and Patrick - can I ping you guys on this?

LGTM from a privacy perspective. I don't see issues with this experiment.

[commit-bot: I haz the power, 2011-12-01 01:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/8689006/32001

[commit-bot: I haz the power, 2011-12-01 03:17:42]

Change committed as 112388