Add an update warning for downloading over mobile data

Add an update warning for downloading over mobile data

https://screenshot.googleplex.com/sHy71kU48TQ

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2795603002
Cr-Commit-Position: refs/heads/master@{#463314}
Committed: https://chromium.googlesource.com/chromium/src/+/23ddc246ca2419019e16b59e1a55d775e3ce3ca1

[weidongg, 2017-04-02 20:40:19]

Description was changed from

==========
Add an update warning for downloading over mobile data

Add an alert in about chrome os page. It shows when user checks for
update and update is available while the user is on mobile data.

Changes to be committed:
modified:   chrome/app/settings_strings.grdp
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.html
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.js
modified:   chrome/browser/resources/settings/settings_resources.grd
modified:  
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

BUG=691108
==========

to

==========
Add an update warning for downloading over mobile data

Add an alert in about chrome os page. It shows when user checks for
update and update is available while the user is on mobile data.

Changes to be committed:
modified:   chrome/app/settings_strings.grdp
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.html
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.js
modified:   chrome/browser/resources/settings/settings_resources.grd
modified:  
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[weidongg, 2017-04-02 20:43:26]

weidongg@chromium.org changed reviewers:
+ afakhry@chromium.org, thestig@chromium.org

[afakhry, 2017-04-03 16:27:26]

afakhry@chromium.org changed reviewers:
+ michaelpg@chromium.org
- thestig@chromium.org

[afakhry, 2017-04-03 16:27:27]

+michaelpg, -thestig

It helps if you show links to screenshots of how this actually would look.

Also, can you please remove the "Changes to be committed:" section from the
commit message as well as the issue description?

https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/about_page/update_warning_dialog.js
(right):

https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:19:
browserProxy_: null,
Why do you need this? I'm not seeing it being used anywhere.

[weidongg, 2017-04-03 16:51:30]

On 2017/04/03 16:27:27, afakhry wrote:
> +michaelpg, -thestig
> 
> It helps if you show links to screenshots of how this actually would look.
> 
> Also, can you please remove the "Changes to be committed:" section from the
> commit message as well as the issue description?
> 
>
https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/about_page/update_warning_dialog.js
> (right):
> 
>
https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/about_page/update_warning_dialog.js:19:
> browserProxy_: null,
> Why do you need this? I'm not seeing it being used anywhere.

Sure, I will remove those and posted the links to screenshots in next patch.
For 'browserProxy: null', I am gonna need it for interacting with handlers in
cpp. But for now, I don't need that. I can remove it in next patch.

[weidongg, 2017-04-03 18:27:29]

The CQ bit was checked by weidongg@chromium.org

[weidongg, 2017-04-03 18:27:31]

The CQ bit was unchecked by weidongg@chromium.org

[weidongg, 2017-04-03 18:36:15]

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

[commit-bot: I haz the power, 2017-04-03 18:37:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-04-03 18:57:10]

Description was changed from

==========
Add an update warning for downloading over mobile data

Add an alert in about chrome os page. It shows when user checks for
update and update is available while the user is on mobile data.

Changes to be committed:
modified:   chrome/app/settings_strings.grdp
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.html
new file:  
chrome/browser/resources/settings/about_page/update_warning_dialog.js
modified:   chrome/browser/resources/settings/settings_resources.grd
modified:  
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add an update warning for downloading over mobile data

https://screenshot.googleplex.com/sHy71kU48TQ

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[weidongg, 2017-04-03 19:22:46]

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

[commit-bot: I haz the power, 2017-04-03 19:24:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:48:55]

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

[commit-bot: I haz the power, 2017-04-03 22:48:56]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-04-04 00:38:40]

On 2017/04/03 16:51:30, weidongg wrote:
> On 2017/04/03 16:27:27, afakhry wrote:
> > +michaelpg, -thestig
> > 
> > It helps if you show links to screenshots of how this actually would look.
> > 
> > Also, can you please remove the "Changes to be committed:" section from the
> > commit message as well as the issue description?
> > 
> >
>
https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
> > File chrome/browser/resources/settings/about_page/update_warning_dialog.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2795603002/diff/1/chrome/browser/resources/se...
> > chrome/browser/resources/settings/about_page/update_warning_dialog.js:19:
> > browserProxy_: null,
> > Why do you need this? I'm not seeing it being used anywhere.
> 
> Sure, I will remove those and posted the links to screenshots in next patch.

Thanks for the screenshot, but generally, we shouldn't link to Google-internal
URLs
in Chromium work. We usually attach screenshots to the bug when possible or use
a hashed go/ link.

> For 'browserProxy: null', I am gonna need it for interacting with handlers in
> cpp. But for now, I don't need that. I can remove it in next patch.

[michaelpg, 2017-04-04 00:44:52]

Just nits.

https://codereview.chromium.org/2795603002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:112: <message
name="IDS_SETTINGS_ABOUT_PAGE_UPDATE_WARNING_MESSAGE" desc="Warning about update
over mobile data.">
nit: "..., specifying the size in megabytes."

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/update_warning_dialog.html
(right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:2: <link
rel="import" href="chrome://resources/html/polymer.html">
sort chrome:// imports alphabetically

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:14: <div
id="update-warning-message"></div>
2-space indent

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:21:
$i18n{aboutUpdateWarningContinue}
2-space indent (looks like 3)

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/update_warning_dialog.js
(right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
2017 :-)

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:15:
properties: {
omit empty properties object

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:30:
//TODO(weidongg): implement the real behaviors here.
nit here and below: "// TODO(..." (space after //)

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:41: *
@param {string} updateSizeMb size of the update in megabytes.
nit: capitalize Size

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:46:
this.i18n("aboutUpdateWarningMessage", updateSizeMb);
4-space indent (looks like 5)

[weidongg, 2017-04-04 04:25:12]

I uploaded a bad patch set and I couldn't delete it, so I uploaded another patch
set.

https://codereview.chromium.org/2795603002/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:112: <message
name="IDS_SETTINGS_ABOUT_PAGE_UPDATE_WARNING_MESSAGE" desc="Warning about update
over mobile data.">
On 2017/04/04 00:44:52, michaelpg wrote:
> nit: "..., specifying the size in megabytes."

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/update_warning_dialog.html
(right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:2: <link
rel="import" href="chrome://resources/html/polymer.html">
On 2017/04/04 00:44:52, michaelpg wrote:
> sort chrome:// imports alphabetically

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:14: <div
id="update-warning-message"></div>
On 2017/04/04 00:44:52, michaelpg wrote:
> 2-space indent

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.html:21:
$i18n{aboutUpdateWarningContinue}
On 2017/04/04 00:44:52, michaelpg wrote:
> 2-space indent (looks like 3)

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/update_warning_dialog.js
(right):

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/04/04 00:44:52, michaelpg wrote:
> 2017 :-)

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:15:
properties: {
On 2017/04/04 00:44:52, michaelpg wrote:
> omit empty properties object

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:30:
//TODO(weidongg): implement the real behaviors here.
On 2017/04/04 00:44:52, michaelpg wrote:
> nit here and below: "// TODO(..." (space after //)

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:41: *
@param {string} updateSizeMb size of the update in megabytes.
On 2017/04/04 00:44:52, michaelpg wrote:
> nit: capitalize Size

Done.

https://codereview.chromium.org/2795603002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:46:
this.i18n("aboutUpdateWarningMessage", updateSizeMb);
On 2017/04/04 00:44:52, michaelpg wrote:
> 4-space indent (looks like 5)

Done.

[michaelpg, 2017-04-04 20:32:14]

lgtm

[weidongg, 2017-04-07 23:17:38]

weidongg@chromium.org changed reviewers:
+ grt@chromium.org

[weidongg, 2017-04-07 23:17:39]

grt@chromium.org: Please review changes in 

chrome/app/settings_strings.grdp

[weidongg, 2017-04-07 23:18:22]

weidongg@chromium.org changed reviewers:
+ msw@chromium.org

[weidongg, 2017-04-07 23:18:23]

msw@chromium.org: Please review changes in 

chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

[msw, 2017-04-08 00:23:14]

I'm not sure if you actually need my review, since
chrome/browser/ui/webui/settings/OWNERS points to:
chrome/browser/resources/settings/OWNERS, which includes michaelpg@, and you
already have his approval, but md_settings_localized_strings_provider.cc lgtm
anyway!

[weidongg, 2017-04-08 00:28:28]

On 2017/04/08 00:23:14, msw wrote:
> I'm not sure if you actually need my review, since
> chrome/browser/ui/webui/settings/OWNERS points to:
> chrome/browser/resources/settings/OWNERS, which includes michaelpg@, and you
> already have his approval, but md_settings_localized_strings_provider.cc lgtm
> anyway!

Thanks, I didn't know that.

[weidongg, 2017-04-10 16:43:58]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-04-10 16:44:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-04-10 16:45:37]

weidongg@chromium.org changed reviewers:
- grt@chromium.org

[weidongg, 2017-04-10 16:47:38]

weidongg@chromium.org changed reviewers:
+ thestig@chromium.org

[weidongg, 2017-04-10 16:47:39]

thestig@chromium.org: Please review changes in 

chrome/app/settings_strings.grdp

[commit-bot: I haz the power, 2017-04-10 17:48:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491842638394610,
"parent_rev": "2562e6f3cf64e1eb1ae95098fa46e626a4494cb5", "commit_rev":
"23ddc246ca2419019e16b59e1a55d775e3ce3ca1"}

[commit-bot: I haz the power, 2017-04-10 17:49:32]

Description was changed from

==========
Add an update warning for downloading over mobile data

https://screenshot.googleplex.com/sHy71kU48TQ

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add an update warning for downloading over mobile data

https://screenshot.googleplex.com/sHy71kU48TQ

BUG=691108
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2795603002
Cr-Commit-Position: refs/heads/master@{#463314}
Committed:
https://chromium.googlesource.com/chromium/src/+/23ddc246ca2419019e16b59e1a55...
==========

[commit-bot: I haz the power, 2017-04-10 17:49:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/23ddc246ca2419019e16b59e1a55...

[Lei Zhang, 2017-04-10 17:58:24]

lgtm, even though CQ went ahead without me.

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:279:
#endif
BTW, this #ifdef section is getting long, and a // defined(OS_CHROMEOS) would be
helpful.

[weidongg, 2017-04-10 18:17:01]

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:279:
#endif
On 2017/04/10 17:58:24, Lei Zhang wrote:
> BTW, this #ifdef section is getting long, and a // defined(OS_CHROMEOS) would
be
> helpful.

Sorry, I don't understand. What do you mean by adding a // defined(OS_CHROMEOS)?

[Lei Zhang, 2017-04-10 18:28:33]

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:279:
#endif
On 2017/04/10 18:17:01, weidongg wrote:
> On 2017/04/10 17:58:24, Lei Zhang wrote:
> > BTW, this #ifdef section is getting long, and a // defined(OS_CHROMEOS)
would
> be
> > helpful.
> 
> Sorry, I don't understand. What do you mean by adding a //
defined(OS_CHROMEOS)?
> 

If one looks at
https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
with the default 10 lines of context, one has no idea what #if the #endif
corresponds to. Thus change it to:

#endif  // defined(OS_CHROMEOS)

to improve readability. Normally when an #ifdef section is really short, there
is no need. e.g.

#ifdef FOO
bar
#endif

but if bar becomes 50 lines long, then it would be better if the #endif has a
comment.

[weidongg, 2017-04-10 19:09:58]

On 2017/04/10 18:28:33, Lei Zhang wrote:
>
https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
> File
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
> (right):
> 
>
https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
>
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:279:
> #endif
> On 2017/04/10 18:17:01, weidongg wrote:
> > On 2017/04/10 17:58:24, Lei Zhang wrote:
> > > BTW, this #ifdef section is getting long, and a // defined(OS_CHROMEOS)
> would
> > be
> > > helpful.
> > 
> > Sorry, I don't understand. What do you mean by adding a //
> defined(OS_CHROMEOS)?
> > 
> 
> If one looks at
>
https://codereview.chromium.org/2795603002/diff/60001/chrome/browser/ui/webui...
> with the default 10 lines of context, one has no idea what #if the #endif
> corresponds to. Thus change it to:
> 
> #endif  // defined(OS_CHROMEOS)
> 
> to improve readability. Normally when an #ifdef section is really short, there
> is no need. e.g.
> 
> #ifdef FOO
> bar
> #endif
> 
> but if bar becomes 50 lines long, then it would be better if the #endif has a
> comment.

Ok, I see. Thanks for clarifying. I will add this and create a new CL.

[Lei Zhang, 2017-04-10 19:10:49]

On 2017/04/10 19:09:58, weidongg wrote:
> Ok, I see. Thanks for clarifying. I will add this and create a new CL.

Don't worry too much about it.