Initial CL for Data Saver (Flywheel) prompt when cellular network detected

Initial CL for Data Saver (Flywheel) prompt when cellular network detected

BUG=454959
TEST=Attach to cellular network.  A systray notification should appear,
prompting to download Data Saver.  Click to go to CWS Data Saver page.

This is a first draft, so folks can try it out.  All new functionality is behind the "enable-datasaver-prompt" experimental flag.  UMA code and possible UI tweaks still to come.

Committed: https://crrev.com/6ac071f7b837e4c519d480155fe2b62488877aac
Cr-Commit-Position: refs/heads/master@{#324126}

[Greg Levin, 2015-03-23 00:37:16]

glevin@chromium.org changed reviewers:
+ derat@chromium.org, finnur@chromium.org, stevenjb@chromium.org

[Greg Levin, 2015-03-23 00:44:22]

Right now, the feature is behind the "enable-datasaver-prompt" experimental
flag.  I’ve added CTRL+ALT+SHIFT+3 as an accelerator to summon the systray
notification / prompt for testing and demo purposes, since access to a cellular
network isn’t alway easy to come by.

I’m currently using the Data Saver icon for the notification box.  I shrunk the
provided icon to 40x40 and 80x80.  We may want to use a different icon, or one
specifically prepared by design team in the future.

Here’s a reviewer breakdown of directory owners for included files:
derat@
  ash/accelerators/    chrome/browser/chromeos/
  chromeos/            ui/chromeos/resources/
finnur@chromium.org
  chrome/browser/      chrome/common/extensions/
stevenjb@chromium.org
  chromeos/            chrome/browser/chromeos/
  chromeos/network/    ui/chromeos/resources/
whoever
  chrome/app/          chrome/common/

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6890: +      <message
name="IDS_FLAGS_ENABLE_DATASAVER_PROMPT_NAME" desc="Title for the flag to enable
a prompt to install Data Saver when dongle/3G netword is detected.">
"netword"->"network" ; also fix below

[Greg Levin, 2015-03-23 00:45:19]

P.S.- Please have a look! :-)

[Daniel Erat, 2015-03-23 03:49:21]

(steven is probably a better reviewer for most of this than i am, but here are
some comments)

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:893: case
DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
nit: keep this list alphabetized

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:1036: case
DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
same here

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.cc:227: { true, ui::VKEY_3, ui::EF_SHIFT_DOWN
| ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
alphabetization

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.h (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.h:65: DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT,
fix alphabetization

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/debug_comm...
File ash/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/debug_comm...
ash/accelerators/debug_commands.cc:174: case DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
alphabetization

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/data_promo_notification.cc:291: if (hotkey_debug)
style guide requires curly brackets for multiline "then" portions of if
statements

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/data_promo_notification.h (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/data_promo_notification.h:38: void
ShowOptionalMobileDataPromoNotification(bool hotkey_debug);
please update the comments to document what the "hotkey_debug" argument is for
(or rename the argument to be more descriptive)

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.h:230: // Used to simulate change in
network properties
nit: add trailing period

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.h:231: void
DebugNotifyNetworkPropertiesUpdated();
adding a debug method here doesn't seem right.

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler_observer.h:19: const char
kHotkeyDebugDataSaver[] = "hotkey-debug-datasaver";
please don't put constants in the toplevel chromeos namespace unless they need
to be there. this would be better as a public static member of whichever class
it's most closely related to.

https://codereview.chromium.org/1022333002/diff/1/ui/chromeos/resources/ui_ch...
File ui/chromeos/resources/ui_chromeos_resources.grd (right):

https://codereview.chromium.org/1022333002/diff/1/ui/chromeos/resources/ui_ch...
ui/chromeos/resources/ui_chromeos_resources.grd:49: <structure
type="chrome_scaled_image" name="IDR_AURA_UBER_TRAY_NOTIFICATION_DS"
file="network/notification_ds.png" />
"DS" isn't meaningful to most people. please rename the resource and file names
to include "data_saver"

[Finnur, 2015-03-23 11:34:33]

LGTM for my parts.

Note, I'm not owner in chrome/browser (for these files).

Also, I'd encourage you to never leave files for review to "anyone". Most people
skim the list for their name and stop reading after that so an "anyone who feels
like reviewing"-list is bound to get ignored. Instead, all files should be
assigned to 1+ reviewer -- that way there's always someone who feels
accountable.

https://codereview.chromium.org/1022333002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:5546: Save data and browser faster using Google
Data Saver. Click to download.
Does it start downloading right away or should the text read "Click for
details"?

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6894: +        Enables a prompt to take user
to Data Saver page on Chrome Web Store, which appears when a 3G network
connection is detected.
Add 'the' in front of 'user' and 'Data'
Also, the latter part of the sentence makes one wonder if the Chrome Web Store
appears when 3G network is detected...

[Greg Levin, 2015-03-24 20:35:33]

Fixed, except for comment on DebugNotifyNetworkPropertiesUpdated(), which I need
some more guidance on.  Please have another look!

finnur:
> Note, I'm not owner in chrome/browser (for these files).

Yeah, but I didn't see a good owner for about_flags.cc (the only file at that
level), and your name was at least in the same OWNERS file, so I gave it to you
:-/

> Also, I'd encourage you to never leave files for review to "anyone".[...]

Got it, thank!  (I'm obviously still learning the finer points of reviewer
selection etiquette)

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:893: case
DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
On 2015/03/23 03:49:20, Daniel Erat wrote:
> nit: keep this list alphabetized

Done, and thanks (it was alphabetized until I renamed it from "FLYWHEEL" to
"DATASAVER" :-P)

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:1036: case
DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
On 2015/03/23 03:49:20, Daniel Erat wrote:
> same here

Done.

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.cc:227: { true, ui::VKEY_3, ui::EF_SHIFT_DOWN
| ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
On 2015/03/23 03:49:20, Daniel Erat wrote:
> alphabetization

Done.

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_table.h (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_table.h:65: DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT,
On 2015/03/23 03:49:20, Daniel Erat wrote:
> fix alphabetization

Done.

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/debug_comm...
File ash/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/1022333002/diff/1/ash/accelerators/debug_comm...
ash/accelerators/debug_commands.cc:174: case DEBUG_TOGGLE_SHOW_DATASAVER_PROMPT:
On 2015/03/23 03:49:20, Daniel Erat wrote:
> alphabetization

Done.

https://codereview.chromium.org/1022333002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:5546: Save data and browser faster using Google
Data Saver. Click to download.
On 2015/03/23 11:34:33, Finnur wrote:
> Does it start downloading right away or should the text read "Click for
> details"?

No, it takes you to CWS.  Thanks for catching this!

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6890: +      <message
name="IDS_FLAGS_ENABLE_DATASAVER_PROMPT_NAME" desc="Title for the flag to enable
a prompt to install Data Saver when dongle/3G netword is detected.">
On 2015/03/23 00:44:22, Greg Levin wrote:
> "netword"->"network" ; also fix below

Done.

https://codereview.chromium.org/1022333002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6894: +        Enables a prompt to take user
to Data Saver page on Chrome Web Store, which appears when a 3G network
connection is detected.
On 2015/03/23 11:34:33, Finnur wrote:
> Add 'the' in front of 'user' and 'Data'
> Also, the latter part of the sentence makes one wonder if the Chrome Web Store
> appears when 3G network is detected...

Done.

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/data_promo_notification.cc:291: if (hotkey_debug)
On 2015/03/23 03:49:21, Daniel Erat wrote:
> style guide requires curly brackets for multiline "then" portions of if
> statements

Done.

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/data_promo_notification.h (right):

https://codereview.chromium.org/1022333002/diff/1/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/data_promo_notification.h:38: void
ShowOptionalMobileDataPromoNotification(bool hotkey_debug);
On 2015/03/23 03:49:21, Daniel Erat wrote:
> please update the comments to document what the "hotkey_debug" argument is for
> (or rename the argument to be more descriptive)

Done.

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.h:230: // Used to simulate change in
network properties
On 2015/03/23 03:49:21, Daniel Erat wrote:
> nit: add trailing period

Done.

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.h:231: void
DebugNotifyNetworkPropertiesUpdated();
On 2015/03/23 03:49:21, Daniel Erat wrote:
> adding a debug method here doesn't seem right.

Could you elaborate on what seems wrong about this?  Do you have any suggestions
for a better place to put it, or a different approach to triggering the prompt
from an accelerator?  Here's my thinking for putting this here:

* CTRL+ALT+SHIFT+3 is meant to simulate a switch to cellular data (since this
isn't always easy to do in practice), which will trigger the Data Saver prompt
for debugging and demo purposes.  NetworkStateHandler is where
NetworkPropertiesUpdated notifications are dispatched, so this seems about like
what my accelerator is trying to fake.

* There didn't seem to be a good way to put the accelerator handler closer to
the source in DataPromoNotification.  This class is only instantiated as a
member of ChromeBrowserMainPartsChromeos, and is never accessed directly. 
DataPromoNotification registers itself on construction as a NetworkStateHandler
observer, and the NetworkPropertiesUpdated and DefaultNetworkChanged
notifications are currently the only entry points into DataPromoNotification
code.  So the accelerator handler has to go through NetworkStateHandler (unless
I built some new framework), and that class can be accessed directly from
debug_commands.cc.

* Not that this is an excuse for bad design, but this code is temporary.  The
debug accelerator and this function will be removed once the Data Saver prompt
is taken out from behind its experimental flag.

Of course, I don't pretend to understand all the surrounding code, so there
could easily be a better solution that I'm not seeing.

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler_observer.h:19: const char
kHotkeyDebugDataSaver[] = "hotkey-debug-datasaver";
On 2015/03/23 03:49:21, Daniel Erat wrote:
> please don't put constants in the toplevel chromeos namespace unless they need
> to be there. this would be better as a public static member of whichever class
> it's most closely related to.

Done.

https://codereview.chromium.org/1022333002/diff/1/ui/chromeos/resources/ui_ch...
File ui/chromeos/resources/ui_chromeos_resources.grd (right):

https://codereview.chromium.org/1022333002/diff/1/ui/chromeos/resources/ui_ch...
ui/chromeos/resources/ui_chromeos_resources.grd:49: <structure
type="chrome_scaled_image" name="IDR_AURA_UBER_TRAY_NOTIFICATION_DS"
file="network/notification_ds.png" />
On 2015/03/23 03:49:21, Daniel Erat wrote:
> "DS" isn't meaningful to most people. please rename the resource and file
names
> to include "data_saver"

Done.

[Daniel Erat, 2015-03-24 20:42:25]

lgtm, but stevenjb@ should still review

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler_observer.h:58: // Fake network path, used
to indicate that NotifyNetworkPropertiesUpdated was
nit: please move this to the top of the 'public' section to match the order in
the .cc file

[Daniel Erat, 2015-03-24 20:45:46]

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/1022333002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.h:231: void
DebugNotifyNetworkPropertiesUpdated();
On 2015/03/24 20:35:32, Greg Levin wrote:
> On 2015/03/23 03:49:21, Daniel Erat wrote:
> > adding a debug method here doesn't seem right.
> 
> Could you elaborate on what seems wrong about this?  Do you have any
suggestions
> for a better place to put it, or a different approach to triggering the prompt
> from an accelerator?  Here's my thinking for putting this here:
> 
> * CTRL+ALT+SHIFT+3 is meant to simulate a switch to cellular data (since this
> isn't always easy to do in practice), which will trigger the Data Saver prompt
> for debugging and demo purposes.  NetworkStateHandler is where
> NetworkPropertiesUpdated notifications are dispatched, so this seems about
like
> what my accelerator is trying to fake.
> 
> * There didn't seem to be a good way to put the accelerator handler closer to
> the source in DataPromoNotification.  This class is only instantiated as a
> member of ChromeBrowserMainPartsChromeos, and is never accessed directly. 
> DataPromoNotification registers itself on construction as a
NetworkStateHandler
> observer, and the NetworkPropertiesUpdated and DefaultNetworkChanged
> notifications are currently the only entry points into DataPromoNotification
> code.  So the accelerator handler has to go through NetworkStateHandler
(unless
> I built some new framework), and that class can be accessed directly from
> debug_commands.cc.
> 
> * Not that this is an excuse for bad design, but this code is temporary.  The
> debug accelerator and this function will be removed once the Data Saver prompt
> is taken out from behind its experimental flag.
> 
> Of course, I don't pretend to understand all the surrounding code, so there
> could easily be a better solution that I'm not seeing.

thanks for the explanation. there are stub implementations of many of the
chromeos/ classes that are used only when running a chrome os build on the
desktop, although i'm not sure whether that applies to this code as well.
stevenjb@ is the best person to comment on this. if it needs to go here, please
add a comment stating that it's temporary and linking to the bug that's tracking
the feature (to make it less likely that we'll forget to remove it later).

[stevenjb, 2015-03-25 17:57:08]

We already have a pretty robust fake shill implementation for testing the
network behavior. We really shouldn't be adding debugging code like this to NSH
or the data promo code. It makes the code more error prone and harder to follow.

Looking at the logic it seems like switching to Cellular (e.g. by disabling
wifi) should always trigger the notification? If I am missing something, we
could use the --shill-stub command line flag to set up the Cellular network such
that it will naturally trigger the notification.

I am more than happy to make suggestions as to how to set up testing this
without the hotkey.

https://codereview.chromium.org/1022333002/diff/20001/ash/accelerators/debug_...
File ash/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/ash/accelerators/debug_...
ash/accelerators/debug_commands.cc:173: ->DebugNotifyNetworkPropertiesUpdated();
The correlation between "show dataserver prompt" and
"DebugNotifyNetworkPropertiesUpdated" is not clear.

https://codereview.chromium.org/1022333002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:99: PrefService* prefs
= profile->GetPrefs();
nit: I think these would actually be more clear without the extra locals, i.e.
ProfileManager::GetActiveUserProfile()->GetPrefs()->GetInteger

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:100:
NotifyNetworkPropertiesUpdated(&network);
I don't like putting debug code like this in NetworkStateHandler. It seems like
this could be handled entirely in the promo notification code. See also my high
level comment.

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler_observer.h:60: static const char
kDebugDataSaverNetworkPath[];
This definitely doesn't belong in this file. If there is a strong reason why we
have to do this through NSH, we should create a more generic debug handler and
pass the path and Type to it.

[Greg Levin, 2015-04-03 18:28:37]

After some good off-line feedback from Steven, I removed all hotkey code. 
Instead, the "enable-datasaver-prompt" switch is now multivalued, with
"disabled", "enabled", and "demo mode" values.  In demo mode, any change in
network properties will trigger the Data Saver prompt, and there's no limit to
the number of times it can be shown.  This would be annoying under normal
circumstances, but will be useful for debugging and demonstration purposes.

Please have another look!

https://codereview.chromium.org/1022333002/diff/20001/ash/accelerators/debug_...
File ash/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/ash/accelerators/debug_...
ash/accelerators/debug_commands.cc:173: ->DebugNotifyNetworkPropertiesUpdated();
On 2015/03/25 17:57:08, stevenjb wrote:
> The correlation between "show dataserver prompt" and
> "DebugNotifyNetworkPropertiesUpdated" is not clear.

All hotkey code deleted.

https://codereview.chromium.org/1022333002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:99: PrefService* prefs
= profile->GetPrefs();
On 2015/03/25 17:57:08, stevenjb wrote:
> nit: I think these would actually be more clear without the extra locals, i.e.
> ProfileManager::GetActiveUserProfile()->GetPrefs()->GetInteger

Done.

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:100:
NotifyNetworkPropertiesUpdated(&network);
On 2015/03/25 17:57:08, stevenjb wrote:
> I don't like putting debug code like this in NetworkStateHandler. It seems
like
> this could be handled entirely in the promo notification code. See also my
high
> level comment.
> 

Done (code removed).

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler_observer.h:58: // Fake network path, used
to indicate that NotifyNetworkPropertiesUpdated was
On 2015/03/24 20:42:25, Daniel Erat wrote:
> nit: please move this to the top of the 'public' section to match the order in
> the .cc file

Removed.

https://codereview.chromium.org/1022333002/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler_observer.h:60: static const char
kDebugDataSaverNetworkPath[];
On 2015/03/25 17:57:08, stevenjb wrote:
> This definitely doesn't belong in this file. If there is a strong reason why
we
> have to do this through NSH, we should create a more generic debug handler and
> pass the path and Type to it.

Removed.

[stevenjb, 2015-04-03 20:07:26]

Much cleaner, thanks! Just a few comments / questions.

https://codereview.chromium.org/1022333002/diff/40001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1022333002/diff/40001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5546: Save data and browser faster using Google
Data Saver. Click to learn more.
s/browser/browse/

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:224:
->HasPendingConnectRequest())
nit: I think that keeping these separate is a little more clear. At least maybe
separate out the default_network check after the others.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:230: 
nit: It looks like we can set |notifications_shown_| = true here, since both
return paths below set it to true? (This was true before with |check_for_promo_|
also, I only mention it because it confused me while reviewing this change).

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:311:
base::Bind(&NotificationClicked, path, kDataSaverExtensionUrl)));
Is the intended behavior to show the settings for the network if the
notification is clicked? That's probably fine, but not entirely intuitive, I
would confirm with UX that it is the desired behavior.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:314:
SetDataSaverPromptsShown(0);  // demo mode resets times shown counts.
If we are resetting the times shown here, do we also need the check on line 288?
Less checks would be better.

[Greg Levin, 2015-04-03 22:17:33]

https://codereview.chromium.org/1022333002/diff/40001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1022333002/diff/40001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5546: Save data and browser faster using Google
Data Saver. Click to learn more.
On 2015/04/03 20:07:26, stevenjb wrote:
> s/browser/browse/
> 

Done.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:224:
->HasPendingConnectRequest())
On 2015/04/03 20:07:26, stevenjb wrote:
> nit: I think that keeping these separate is a little more clear. At least
maybe
> separate out the default_network check after the others.

Done.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:230: 
On 2015/04/03 20:07:26, stevenjb wrote:
> nit: It looks like we can set |notifications_shown_| = true here, since both
> return paths below set it to true? (This was true before with
|check_for_promo_|
> also, I only mention it because it confused me while reviewing this change).
> 

Done.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:311:
base::Bind(&NotificationClicked, path, kDataSaverExtensionUrl)));
On 2015/04/03 20:07:26, stevenjb wrote:
> Is the intended behavior to show the settings for the network if the
> notification is clicked? That's probably fine, but not entirely intuitive, I
> would confirm with UX that it is the desired behavior.

You mean from |path|?  The network settings were never being shown, but it
didn't need to be there either, just leftovers from a copy-paste that I didn't
dig deep enough into.

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:314:
SetDataSaverPromptsShown(0);  // demo mode resets times shown counts.
On 2015/04/03 20:07:26, stevenjb wrote:
> If we are resetting the times shown here, do we also need the check on line
288?
> Less checks would be better.

This is done to accommodate the case where you're switching back and forth
between "enabled" and "demo mode".  The intent is to have demo mode reset the
count, so that if you then switch back to enabled, you can test the system with
a fresh count = 0, check that the notification only appears twice, and then do
this as many times as you want.  I'll document this in the issue description
once this lands.

Does this answer your question, or was I missing your point?

[stevenjb, 2015-04-03 22:27:53]

lgtm

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:314:
SetDataSaverPromptsShown(0);  // demo mode resets times shown counts.
On 2015/04/03 22:17:33, Greg Levin wrote:
> On 2015/04/03 20:07:26, stevenjb wrote:
> > If we are resetting the times shown here, do we also need the check on line
> 288?
> > Less checks would be better.
> 
> This is done to accommodate the case where you're switching back and forth
> between "enabled" and "demo mode".  The intent is to have demo mode reset the
> count, so that if you then switch back to enabled, you can test the system
with
> a fresh count = 0, check that the notification only appears twice, and then do
> this as many times as you want.  I'll document this in the issue description
> once this lands.
> 
> Does this answer your question, or was I missing your point?

I see. If we have shown the notification and then switch to demo mode we
wouldn't ever see it without the extra test on line 288. That's fine then.

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:310:
base::Bind(&NotificationClicked, "", kDataSaverExtensionUrl)));
Oh, I see, NotificationClicked looks at kDataSaverExtensionUrl first. If that is
empty however I would suggest that we change NotificationClicked to do nothing
if |path| is empty (instead of just showing the network settings). Not a big
deal either way really.

[Greg Levin, 2015-04-04 19:27:26]

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:310:
base::Bind(&NotificationClicked, "", kDataSaverExtensionUrl)));
On 2015/04/03 22:27:53, stevenjb wrote:
> Oh, I see, NotificationClicked looks at kDataSaverExtensionUrl first. If that
is
> empty however I would suggest that we change NotificationClicked to do nothing
> if |path| is empty (instead of just showing the network settings). Not a big
> deal either way really.
> 

I fixed it so that it won't open a blank web page if |info_url| is empty. 
However, ShowNetworkSettings() still shows the settings page, even if
|service_path| is empty.  Since the url won't be empty when called from
ShowDataSaverNotification(), conditioning on |service_path| being nonempty could
only change the behavior when called from
ShowOptionalMobileDataPromoNotification(), which I was planning to leave alone
as much as possible.  Does this version look okay?

[stevenjb, 2015-04-06 15:34:39]

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/1022333002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/status/data_promo_notification.cc:310:
base::Bind(&NotificationClicked, "", kDataSaverExtensionUrl)));
On 2015/04/04 19:27:25, Greg Levin wrote:
> On 2015/04/03 22:27:53, stevenjb wrote:
> > Oh, I see, NotificationClicked looks at kDataSaverExtensionUrl first. If
that
> is
> > empty however I would suggest that we change NotificationClicked to do
nothing
> > if |path| is empty (instead of just showing the network settings). Not a big
> > deal either way really.
> > 
> 
> I fixed it so that it won't open a blank web page if |info_url| is empty. 
> However, ShowNetworkSettings() still shows the settings page, even if
> |service_path| is empty.  Since the url won't be empty when called from
> ShowDataSaverNotification(), conditioning on |service_path| being nonempty
could
> only change the behavior when called from
> ShowOptionalMobileDataPromoNotification(), which I was planning to leave alone
> as much as possible.  Does this version look okay?

This looks correct. I think that I missed that kDataSaverExtensionUrl should
always be valid for this notification, so it should never show Settings, in
which case this is fine. (The new logic is definitely easier to follow).

[Greg Levin, 2015-04-06 15:43:37]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2015-04-06 15:43:37]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
derat@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1022333002/#ps80001
(title: "Clean up NotificationClicked()")

[commit-bot: I haz the power, 2015-04-06 15:43:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022333002/80001

[commit-bot: I haz the power, 2015-04-06 15:48:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-06 15:48:54]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Greg Levin, 2015-04-07 15:14:43]

Patchset #6 (id:100001) has been deleted

[Greg Levin, 2015-04-07 15:17:49]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2015-04-07 15:17:50]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
stevenjb@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1022333002/#ps120001
(title: "Merge")

[Greg Levin, 2015-04-07 15:17:56]

The CQ bit was unchecked by glevin@chromium.org

[Greg Levin, 2015-04-07 15:18:34]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-07 15:19:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022333002/120001

[commit-bot: I haz the power, 2015-04-07 16:34:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-07 16:34:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Greg Levin, 2015-04-07 17:10:50]

glevin@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Greg Levin, 2015-04-07 17:10:51]

Hi Alexei.  Could I get a quick review of the command line flags I added to
histograms.xml?  Everything else is fine (LGTM'd by others), but the
AboutFlagsHistogramTest.CheckHistograms test was failing without these additions
(it passes now).  Thanks!

Failed test log:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

(P.S.- I didn't know this file existed until the test failed, so don't assume
that I have any idea what I'm doing here :-)

[Alexei Svitkine (slow), 2015-04-07 18:42:23]

lgtm

[Greg Levin, 2015-04-07 20:28:33]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2015-04-07 20:28:34]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
stevenjb@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1022333002/#ps140001
(title: "Add command line flags to histograms.xml")

[commit-bot: I haz the power, 2015-04-07 20:28:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022333002/140001

[commit-bot: I haz the power, 2015-04-07 21:48:23]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-04-07 21:51:08]

Patchset 7 (id:??) landed as
https://crrev.com/6ac071f7b837e4c519d480155fe2b62488877aac
Cr-Commit-Position: refs/heads/master@{#324126}