MD Settings: Google Play Store: Add subpage and polish

MD Settings: Google Play Store: Add subpage and polish

See issue for screenshots and details.

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

Review-Url: https://codereview.chromium.org/2785013003
Cr-Commit-Position: refs/heads/master@{#461813}
Committed: https://chromium.googlesource.com/chromium/src/+/936a64459466aea94d5782255bc0570d9d764dae

[stevenjb, 2017-03-30 01:20:22]

Description was changed from

==========
MD Settings: Google Play Store: Add subpage and polish

See issue for screenshots and details.

BUG=698463
==========

to

==========
MD Settings: Google Play Store: Add subpage and polish

See issue for screenshots and details.

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

[stevenjb, 2017-03-30 01:22:14]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-03-30 04:47:20]

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

[commit-bot: I haz the power, 2017-03-30 04:47:47]

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-03-30 05:04:49]

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

[commit-bot: I haz the power, 2017-03-30 05:04:49]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Dan Beam, 2017-03-30 13:24:22]

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

https://codereview.chromium.org/2785013003/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:218: Install apps and games from Google Play.
<ph name="BEGIN_LINK">&lt;a target="_blank" href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
these <ph> are a little odd to me.  can we just make one for the URL instead? 
that's the only real replacement that's happening here, as far as I can tell

Install apps and games from Google Play. &lt;a target="_blank" href="<ph
name="URL">$1<ex>https://google.com/</ex></ph>"&gt;Learn more&lt;/a&gt;

https://codereview.chromium.org/2785013003/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:236: <ph name="BEGIN_LINK">&lt;a
target="_blank" href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
same note regarding <ph>, I don't think they're that useful with HTML in them

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.html
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:52:
associated-control="[[$$('#adroid-apps')]]"
this is misspelled as "adroid" and would break search

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.js
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:49:
getSubtext_() {
ES6

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:52:
this.i18nAdvanced('androidAppsSubtext', {});
i'm confused.  i18nAdvanced() takes tags or attribute callbacks.  i'm fairly
sure if you're not passing those, it's the same as calling just i18n().

in looking at the underlying code, I think <a href="https://..."
target="_blank"> is allowed via i18n()

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:26:
on-tap="onRemoveTap_">
most of our code uses the naming convention: onIdNameTap_, as in:
onAndroidAppsTap_, but I don't really know why this ID even exists.  is it used?

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:38:
<div class="body" inner-h-t-m-l="[[getDialogBody_()]]"></div>
can you just change to using i18n('...') directly instead of getDialogBody_()?

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:51:
allowRemove_() {
also using ES6

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:52:
return this.prefs.arc.enabled.enforcement !=
i don't think this will work with closure.

you might to do

this.get('prefs.arc.enabled.enforcement')

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:64: 
extra \n

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:71:
return this.i18nAdvanced('androidAppsDisableDialogMessage', {});
is this equivalent to i18n()?

[stevenjb, 2017-03-30 23:05:37]

PTAL

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

https://codereview.chromium.org/2785013003/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:218: Install apps and games from Google Play.
<ph name="BEGIN_LINK">&lt;a target="_blank" href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> these <ph> are a little odd to me.  can we just make one for the URL instead? 
> that's the only real replacement that's happening here, as far as I can tell
> 
> Install apps and games from Google Play. &lt;a target="_blank" href="<ph
> name="URL">$1<ex>https://google.com/</ex></ph>"&gt;Learn more&lt;/a&gt;

I agree this looks better. FWIW, this is the pattern we use everywhere else
except for one example in Downloads added by you :). I'm fine with changing the
pattern, but there are dozen other examples in this file that we should consider
cleaning up also.

https://codereview.chromium.org/2785013003/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:236: <ph name="BEGIN_LINK">&lt;a
target="_blank" href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> same note regarding <ph>, I don't think they're that useful with HTML in them

Done.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.html
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:52:
associated-control="[[$$('#adroid-apps')]]"
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> this is misspelled as "adroid" and would break search

Thanks, fixed.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.js
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:49:
getSubtext_() {
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> ES6

Done.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:52:
this.i18nAdvanced('androidAppsSubtext', {});
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> i'm confused.  i18nAdvanced() takes tags or attribute callbacks.  i'm fairly
> sure if you're not passing those, it's the same as calling just i18n().
> 
> in looking at the underlying code, I think <a href="https://..."
> target="_blank"> is allowed via i18n()

You are correct. I somehow didn't think that would work. Done.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:26:
on-tap="onRemoveTap_">
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> most of our code uses the naming convention: onIdNameTap_, as in:
> onAndroidAppsTap_, but I don't really know why this ID even exists.  is it
used?

Renamed. The id is used in the (now updated) tests.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:38:
<div class="body" inner-h-t-m-l="[[getDialogBody_()]]"></div>
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> can you just change to using i18n('...') directly instead of getDialogBody_()?

Done. (I think it's a little more likely now that someone might think the
behavior is unused, but I guess devs working on this code should know better).

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js
(right):

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:51:
allowRemove_() {
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> also using ES6

Done.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:52:
return this.prefs.arc.enabled.enforcement !=
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> i don't think this will work with closure.
> 
> you might to do
> 
> this.get('prefs.arc.enabled.enforcement')

It does.

Since prefs.arc.enabled will always exist, I don't think we need to use get
here.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:64: 
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> extra \n

Done.

https://codereview.chromium.org/2785013003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:71:
return this.i18nAdvanced('androidAppsDisableDialogMessage', {});
On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> is this equivalent to i18n()?

Done.

[Dan Beam, 2017-03-30 23:35:19]

lgtm

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

https://codereview.chromium.org/2785013003/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:218: Install apps and games from Google Play.
<ph name="BEGIN_LINK">&lt;a target="_blank" href="$1"&gt;</ph>Learn more<ph
name="END_LINK">&lt;/a&gt;<ex>&lt;/a&gt;</ex></ph>
On 2017/03/30 23:05:36, stevenjb wrote:
> On 2017/03/30 13:24:22, Dan Beam (slow) wrote:
> > these <ph> are a little odd to me.  can we just make one for the URL
instead? 
> > that's the only real replacement that's happening here, as far as I can tell
> > 
> > Install apps and games from Google Play. &lt;a target="_blank" href="<ph
> > name="URL">$1<ex>https://google.com/</ex></ph>"&gt;Learn more&lt;/a&gt;
> 
> I agree this looks better. FWIW, this is the pattern we use everywhere else
> except for one example in Downloads added by you :). I'm fine with changing
the
> pattern, but there are dozen other examples in this file that we should
consider
> cleaning up also.

people will keep copy pasting the ones that don't make much sense until they're
cleaned up, so sgtm

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/android_apps_page_test.js (right):

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/android_apps_page_test.js:92: var div =
androidAppsPage.$$('#android-apps');
can you rename this to a more descriptive name?  |div| is not very helpful.  or
just inline?

MockInteractions.tap(androidAppsPage.$$('#android-apps'));

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/android_apps_page_test.js:134: var div =
androidAppsPage.$$('#android-apps');
same nit re: div

[stevenjb, 2017-03-31 16:33:24]

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/android_apps_page_test.js (right):

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/android_apps_page_test.js:92: var div =
androidAppsPage.$$('#android-apps');
On 2017/03/30 23:35:19, Dan Beam (slow) wrote:
> can you rename this to a more descriptive name?  |div| is not very helpful. 
or
> just inline?
> 
> MockInteractions.tap(androidAppsPage.$$('#android-apps'));

Done.

https://codereview.chromium.org/2785013003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/android_apps_page_test.js:134: var div =
androidAppsPage.$$('#android-apps');
On 2017/03/30 23:35:19, Dan Beam (slow) wrote:
> same nit re: div

Done.

[stevenjb, 2017-03-31 16:33:27]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-31 16:33:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2785013003/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-03-31 16:34:06]

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-03-31 16:55:13]

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

[commit-bot: I haz the power, 2017-03-31 16:55:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[stevenjb, 2017-04-03 21:57:12]

PTAL - Tom requested that we revert some string changes.

[stevenjb, 2017-04-03 21:57:16]

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

[commit-bot: I haz the power, 2017-04-03 21:58: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 23:06:54]

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

[commit-bot: I haz the power, 2017-04-03 23:06:55]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-04-04 18:39:14]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-04-04 18:39:14]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2785013003/#ps60001
(title: "Restore some strings")

[commit-bot: I haz the power, 2017-04-04 18:40:46]

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-04 20:20:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491331154558360,
"parent_rev": "ce7aab5943f71944de9de888afcb711988f16bc4", "commit_rev":
"936a64459466aea94d5782255bc0570d9d764dae"}

[commit-bot: I haz the power, 2017-04-04 20:21:09]

Description was changed from

==========
MD Settings: Google Play Store: Add subpage and polish

See issue for screenshots and details.

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

to

==========
MD Settings: Google Play Store: Add subpage and polish

See issue for screenshots and details.

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

Review-Url: https://codereview.chromium.org/2785013003
Cr-Commit-Position: refs/heads/master@{#461813}
Committed:
https://chromium.googlesource.com/chromium/src/+/936a64459466aea94d5782255bc0...
==========

[commit-bot: I haz the power, 2017-04-04 20:21:10]

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

[Dan Beam, 2017-04-04 21:57:15]

https://codereview.chromium.org/2785013003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.js
(right):

https://codereview.chromium.org/2785013003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:52:
return this.androidAppsInfo_.appReady ? this.i18n('androidAppsSubtext') :
should this be 'toggleOn'?

https://codereview.chromium.org/2785013003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:53:
this.i18n('androidAppsSubtext');
whoops

[kelvinp, 2017-04-04 22:11:55]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2798753002/ by kelvinp@chromium.org.

The reason for reverting is: This change causes 88 browser_tests to fail on
Builder Linux ChromiumOS Tests (dbg)(1) Build 

https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....