MD Settings: Add Google Play Store (Arc++) section

MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true

Committed: https://crrev.com/ee01341a740fc754b49a7d3d37fa991d090db384
Cr-Commit-Position: refs/heads/master@{#436406}

[stevenjb, 2016-11-30 18:30:22]

Description was changed from

==========
MD Settings: Add Google Play Store (Arc++) section

BUG=619705
==========

to

==========
MD Settings: Add Google Play Store (Arc++) section

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

[stevenjb, 2016-11-30 20:33:55]

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

[stevenjb, 2016-12-01 20:04:22]

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

[stevenjb, 2016-12-01 20:04:22]

+fukino@

[michaelpg, 2016-12-01 20:15:34]

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:242: // Android
Apps
consistency nit: end w/ period

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.html
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.html:1:
<script src="/android_apps_page/android_apps_browser_proxy.js"></script>
import cr.html

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:18:
function AndroidAppsBrowserProxy() {
opt nit: } on same line

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:32:
function AndroidAppsBrowserProxyImpl() {
same nit, also add a blank line between this and the comments below?

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.html
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:43:
<!-- Confirm disable android apps dialog -->
nit: remove redundant comment

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:59: 
nit: no blank line (its fine keeping <dialog> separated from the prior tag, but
it's not necessary here, same as the <template> line above)

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

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:7: *
'androuid-apps-page' is the settings page for enabling android apps.
android

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:53:
getDialogBody_: function() {
/** @return {string} */

and @private

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:97:
doNothing_: function(event) {
unused?

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:97:
this.pageVisibility.androidApps = false;
does this work? i'm worried about a couple things:

* it's not using this.set, so settings-ui and settings-menu are not notified of
the change (e.g., it won't hide it from the menu)
* it's happening dynamically, so i'm not convinced the page doesn't get stamped
first before hiding
* should we hide this in guest mode via settings_ui.js

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:125: <div
data-path="/androidApps" on-tap="openPage_">
see other examples for hiding based on pageVisibility

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:10: #include
"chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
duplicate include

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:84:
ResolveJavascriptCallback(*callback_id, *info);
this function seems a bit redundant. since JS already has a listener for the
same info, this could just call SendAndroidAppsInfo and avoid all the Promise
boilerplate. so i'm proposing JS that looks like:

    cr.addWebUIListener('android-apps-info-update',
        this.update_.bind(this));
    this.browserProxy_.getAndroidAppsInfo();

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.h (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.h:31: //
SettingsPageUIHandler
nit: " overrides." like below

[stevenjb, 2016-12-02 00:39:19]

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:242: // Android
Apps
On 2016/12/01 20:15:34, michaelpg wrote:
> consistency nit: end w/ period

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.html
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.html:1:
<script src="/android_apps_page/android_apps_browser_proxy.js"></script>
On 2016/12/01 20:15:34, michaelpg wrote:
> import cr.html

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:18:
function AndroidAppsBrowserProxy() {
On 2016/12/01 20:15:34, michaelpg wrote:
> opt nit: } on same line
I got pushback from other reviewers for doing that in the past so I updated my
.clang-format file, and I don't argue with clang.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:32:
function AndroidAppsBrowserProxyImpl() {
On 2016/12/01 20:15:34, michaelpg wrote:
> same nit, also add a blank line between this and the comments below?

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/android_apps_page/android_apps_page.html
(right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:43:
<!-- Confirm disable android apps dialog -->
On 2016/12/01 20:15:34, michaelpg wrote:
> nit: remove redundant comment

I like my highly visible comments separating out the dialog UI.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:59: 
On 2016/12/01 20:15:34, michaelpg wrote:
> nit: no blank line (its fine keeping <dialog> separated from the prior tag,
but
> it's not necessary here, same as the <template> line above)

Done.

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

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:7: *
'androuid-apps-page' is the settings page for enabling android apps.
On 2016/12/01 20:15:34, michaelpg wrote:
> android

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:53:
getDialogBody_: function() {
On 2016/12/01 20:15:34, michaelpg wrote:
> /** @return {string} */
> 
> and @private

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/android_apps_page/android_apps_page.js:97:
doNothing_: function(event) {
On 2016/12/01 20:15:34, michaelpg wrote:
> unused?

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:97:
this.pageVisibility.androidApps = false;
On 2016/12/01 20:15:34, michaelpg wrote:
> does this work? i'm worried about a couple things:
> 
> * it's not using this.set, so settings-ui and settings-menu are not notified
of
> the change (e.g., it won't hide it from the menu)
> * it's happening dynamically, so i'm not convinced the page doesn't get
stamped
> first before hiding
> * should we hide this in guest mode via settings_ui.js

Ugh. The way we handle pageVisibility is confusing. We use it one way for tests,
a different way for gues tmode.

Rather than make a major change to that code, I will just add a separate
property in settings_ui instead. (This was the wrong place for this regardless,
I didn't realize settings_ui was at the top of the tree).

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:125: <div
data-path="/androidApps" on-tap="openPage_">
On 2016/12/01 20:15:34, michaelpg wrote:
> see other examples for hiding based on pageVisibility 

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:10: #include
"chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
On 2016/12/01 20:15:34, michaelpg wrote:
> duplicate include

Done.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:84:
ResolveJavascriptCallback(*callback_id, *info);
On 2016/12/01 20:15:34, michaelpg wrote:
> this function seems a bit redundant. since JS already has a listener for the
> same info, this could just call SendAndroidAppsInfo and avoid all the Promise
> boilerplate. so i'm proposing JS that looks like:
> 
>     cr.addWebUIListener('android-apps-info-update',
>         this.update_.bind(this));
>     this.browserProxy_.getAndroidAppsInfo();

Sure, I can make this a 'request' instead of a 'get'.

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.h (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/android_apps_handler.h:31: //
SettingsPageUIHandler
On 2016/12/01 20:15:34, michaelpg wrote:
> nit: " overrides." like below

I changed the copied code below. Preferences vary, but I prefer short.
'overrides' is redundant.

[michaelpg, 2016-12-02 03:47:44]

lgtm

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2541923002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:97:
this.pageVisibility.androidApps = false;
On 2016/12/02 00:39:19, stevenjb wrote:
> On 2016/12/01 20:15:34, michaelpg wrote:
> > does this work? i'm worried about a couple things:
> > 
> > * it's not using this.set, so settings-ui and settings-menu are not notified
> of
> > the change (e.g., it won't hide it from the menu)
> > * it's happening dynamically, so i'm not convinced the page doesn't get
> stamped
> > first before hiding
> > * should we hide this in guest mode via settings_ui.js
> 
> Ugh. The way we handle pageVisibility is confusing. We use it one way for
tests,
> a different way for gues tmode.
> 
> Rather than make a major change to that code, I will just add a separate
> property in settings_ui instead. (This was the wrong place for this
regardless,
> I didn't realize settings_ui was at the top of the tree).

Yeah... I don't know if there's a non-sucky way of doing this without fixing
pageVisibility.

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

https://codereview.chromium.org/2541923002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.js:45: var visibility =
/** @type{boolean|undefined} */ (
nit: space after @type?

[stevenjb, 2016-12-02 16:54:58]

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

[stevenjb, 2016-12-02 16:54:59]

+lhchavez@ for propagating flags to LaunchAndroidSettingsApp() in
android_apps_handler.cc

[stevenjb, 2016-12-02 17:43:56]

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

https://codereview.chromium.org/2541923002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/basic_page/basic_page.js:45: var visibility =
/** @type{boolean|undefined} */ (
On 2016/12/02 03:47:44, michaelpg wrote:
> nit: space after @type?

Done.

[Luis Héctor Chávez, 2016-12-02 19:29:21]

ARC-specific event propagation lgtm (with one nit: s/Arc++/ARC/g, in both the
all the code and commit description).

[stevenjb, 2016-12-02 23:58:51]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-02 23:58:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2541923002/#ps80001
(title: "Rebase + nit")

[commit-bot: I haz the power, 2016-12-02 23:59:50]

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, 2016-12-03 00:10:48]

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

[commit-bot: I haz the power, 2016-12-03 00:10:50]

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

[stevenjb, 2016-12-05 18:24:16]

Description was changed from

==========
MD Settings: Add Google Play Store (Arc++) section

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

to

==========
MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true
==========

[stevenjb, 2016-12-05 18:24:22]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-05 18:24:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2541923002/#ps100001
(title: "Fix tests")

[commit-bot: I haz the power, 2016-12-05 18:24:44]

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, 2016-12-05 21:19:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480962262244560,
"parent_rev": "1f81c6914e6fdca0a24142c41e2455dfd548fd57", "commit_rev":
"819fbf522cca07926730e614dd7e11e48039e934"}

[commit-bot: I haz the power, 2016-12-05 21:20:01]

Description was changed from

==========
MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true
==========

to

==========
MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-12-05 21:20:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-05 21:24:14]

Description was changed from

==========
MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true
==========

to

==========
MD Settings: Add Google Play Store (Arc++) section

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

For linter bug http://crbug.com/567770
NOPRESUBMIT=true

Committed: https://crrev.com/ee01341a740fc754b49a7d3d37fa991d090db384
Cr-Commit-Position: refs/heads/master@{#436406}
==========

[commit-bot: I haz the power, 2016-12-05 21:24:15]

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