|
|
Created:
3 years, 7 months ago by khmel Modified:
3 years, 7 months ago Reviewers:
stevenjb CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Handle ARC events in MD Settings
This provides handling ARC events.
TURN ON button should be available in case ARC is disabled and unmanaged.
Once ARC is enabled TURN ON button is hidden and APPS section is shown.
APPS section shows link to ARC Settings app once Settings app appears in
the system. While Settings app is not registered tehn link to ARC Settings
is hidden. In case user click on ARC Settings link and Settings app is not
ready yet (ARC is still booting) then Settings app is started in deferred
mode (spinning animation on shelf). Disabling ARC hides APPS section and
TURN ON button is active again.
BUG=720173
, b/37523579, b/29244888
TEST=Browser test extended, manually on device for managed/unmanaged
cases.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2873853002
Cr-Commit-Position: refs/heads/master@{#471206}
Committed: https://chromium.googlesource.com/chromium/src/+/7bb2e7bc7d31c75b0b7f7b384add49df00d69259
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix annotations #
Total comments: 13
Patch Set 3 : updatge #
Total comments: 11
Patch Set 4 : up #
Total comments: 2
Patch Set 5 : changed tap to click / rebased #Patch Set 6 : move hide logic to subpage #
Total comments: 21
Patch Set 7 : comments addressed #
Total comments: 5
Patch Set 8 : attached/detached approach #
Total comments: 2
Patch Set 9 : nit #Patch Set 10 : fix annotations #Patch Set 11 : fix test #Messages
Total messages: 54 (33 generated)
Description was changed from ========== arc: Handle ARC events in MD Settings This provides handling ARC events. TURN ON button should be available in case ARC is disabled and unmanaged. Once ARC is enabled TURN ON button is hidden and APPS section is shown. APPS section shows link to ARC Settings app once Settings app appears in the system. While Settings app is not registered tehn link to ARC Settings is hidden. In case user click on ARC Settings link and Settings app is not ready yet (ARC is still booting) then Settings app is started in deferred mode (spinning animation on shelf). Disabling ARC hides APPS section and TURN ON button is active again. BUG=720173, b/37523579, b/29244888 TEST=Browser test extended, manually on device for managed/unmanaged cases. ========== to ========== arc: Handle ARC events in MD Settings This provides handling ARC events. TURN ON button should be available in case ARC is disabled and unmanaged. Once ARC is enabled TURN ON button is hidden and APPS section is shown. APPS section shows link to ARC Settings app once Settings app appears in the system. While Settings app is not registered tehn link to ARC Settings is hidden. In case user click on ARC Settings link and Settings app is not ready yet (ARC is still booting) then Settings app is started in deferred mode (spinning animation on shelf). Disabling ARC hides APPS section and TURN ON button is active again. BUG=720173, b/37523579, b/29244888 TEST=Browser test extended, manually on device for managed/unmanaged cases. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
khmel@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven PTAL Thanks! https://codereview.chromium.org/2873853002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (left): https://codereview.chromium.org/2873853002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:52: void AndroidAppsHandler::OnAppReadyChanged(const std::string& app_id, ARC can run apps in deferred mode. We can ignore ready flag.
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:14: * appSettingsAvailable: boolean, nit: document each of these. I know we use 'Play Store' for the user facing messaging, but I still think of the feature availability as 'ARC'. also: maybe 'settingsAppAvailable'? https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:59: if (settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) Combine these ifs. https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:60: settings.navigateTo(settings.Route.ANDROID_APPS); nit: settings.navigateToPreviousRoute(); https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:76: ArcAppListPrefs::Get(profile_)->IsRegistered(arc::kSettingsAppId)); I definitely prefer settingsAppAvailable since we refer to it by kSettingsAppId. https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:36: appsInfo.appSettingsAvailable = appSettingsAvailable; nit: var appsInfo = { playStoreEnabled: playStoreEnabled, appSettingsAvailable: appSettingsAvailable, }; https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:127: // leads to tap ignoring and test failure. Simulate tap asynchronously. Instead of using setTimeout, try this: return Polymer.Base.async(function() { MockInteractions.tap(remove); Polymer.dom.flush(); ssertTrue(dialog.open); });
Thank you Steven for comments. please find updated version https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:14: * appSettingsAvailable: boolean, On 2017/05/10 16:47:03, stevenjb wrote: > nit: document each of these. I know we use 'Play Store' for the user facing > messaging, but I still think of the feature availability as 'ARC'. > also: maybe 'settingsAppAvailable'? Some times ago we changed meaning ARC available to Play Store available. There is current work to use ARC without the Play Store and we distinguish Play Store and ARC availability. Also renamed. https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:59: if (settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) On 2017/05/10 16:47:03, stevenjb wrote: > Combine these ifs. Done. https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:60: settings.navigateTo(settings.Route.ANDROID_APPS); On 2017/05/10 16:47:03, stevenjb wrote: > nit: settings.navigateToPreviousRoute(); Done. My concern was what if user directly opens ANDROID_APPS_DETAILS. IIUC this would be possible to do by providing direct URL in settings. https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:76: ArcAppListPrefs::Get(profile_)->IsRegistered(arc::kSettingsAppId)); On 2017/05/10 16:47:03, stevenjb wrote: > I definitely prefer settingsAppAvailable since we refer to it by kSettingsAppId. Renamed it as in your previous comment. https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:36: appsInfo.appSettingsAvailable = appSettingsAvailable; On 2017/05/10 16:47:03, stevenjb wrote: > nit: > var appsInfo = { > playStoreEnabled: playStoreEnabled, > appSettingsAvailable: appSettingsAvailable, > }; Done. https://codereview.chromium.org/2873853002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:127: // leads to tap ignoring and test failure. Simulate tap asynchronously. On 2017/05/10 16:47:03, stevenjb wrote: > Instead of using setTimeout, try this: > > return Polymer.Base.async(function() { > MockInteractions.tap(remove); > Polymer.dom.flush(); > ssertTrue(dialog.open); > }); Nice, thanks for sharing this info. I tried to find something similar . However did not help :(. Tap simulation ignored: function tap(node, options) { // Respect nodes that are disabled in the UI. if (window.getComputedStyle(node)['pointer-events'] === 'none') return; https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) in test this causes multiple listener invocation and as result multiple navigateToPreviousRoute (which is async) https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:138: if (settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) { Need to go back after test. Otherwise multiple setup calls stack several app_detailed locations and it is impossible to validate that navigation actually happens. https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:170: return whenNodeCanInteract(remove).then(function() { Well, Polymer.async did not help. also SetTimeot 0 does not work always. I found only this way to wait until remove button is active.
https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:60: settings.navigateTo(settings.Route.ANDROID_APPS); On 2017/05/10 21:48:12, khmel wrote: > On 2017/05/10 16:47:03, stevenjb wrote: > > nit: settings.navigateToPreviousRoute(); > > Done. > My concern was what if user directly opens ANDROID_APPS_DETAILS. IIUC this would > be possible to do by providing direct URL in settings. In that case we would just return to the main Settings page, which is fine, but in general using navigateToPreviousRoute() is a bit more robust. https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) On 2017/05/10 21:48:13, khmel wrote: > in test this causes multiple listener invocation and as result multiple > navigateToPreviousRoute (which is async) This is weird, we should avoid it. The test to settings.getCurrentRoute() should be sufficient? If not, try adding an observer to androidAppsInfo in the subpage and moving the test there? In practice, we don't necessarily need this. Any UI action that would cause this change could close the details page, and if playStoreEnabled changes to false while the details page is shown, it's not the end of the world if it remains open. https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:138: if (settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) { On 2017/05/10 21:48:13, khmel wrote: > Need to go back after test. Otherwise multiple setup calls stack several > app_detailed locations and it is impossible to validate that navigation actually > happens. Ugh. Can we just navigate directly to ANDROID_APPS_PAGE in setup instead? https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:170: return whenNodeCanInteract(remove).then(function() { On 2017/05/10 21:48:13, khmel wrote: > Well, Polymer.async did not help. also SetTimeot 0 does not work always. I found > only this way to wait until remove button is active. Ugh. Timeout loops like that tend to be fragile in tests. This is a lot of subtle / complex change. I would want to bring in some folks more familiar with our test infrastructure rather than adding yet another complicated solution. I have worked around issues with 'tap' before by calling the tap handler directly. Can we try doing that instead?
https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) On 2017/05/10 22:35:44, stevenjb wrote: > On 2017/05/10 21:48:13, khmel wrote: > > in test this causes multiple listener invocation and as result multiple > > navigateToPreviousRoute (which is async) > > This is weird, we should avoid it. The test to settings.getCurrentRoute() should > be sufficient? > > If not, try adding an observer to androidAppsInfo in the subpage and moving the > test there? > > In practice, we don't necessarily need this. Any UI action that would cause this > change could close the details page, and if playStoreEnabled changes to false > while the details page is shown, it's not the end of the world settings.navigateToPreviousRoute() is async (windows.back) so settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) will be true several times. it's not the end of the world but the end of the ideal world :) Frankly speaking that looks really weird. I can close ARC OptIn window. This disables ARC but settings still show "Remove Android Apps" We have to handle this case. https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:138: if (settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) { On 2017/05/10 22:35:44, stevenjb wrote: > On 2017/05/10 21:48:13, khmel wrote: > > Need to go back after test. Otherwise multiple setup calls stack several > > app_detailed locations and it is impossible to validate that navigation > actually > > happens. > > Ugh. Can we just navigate directly to ANDROID_APPS_PAGE in setup instead? Yes, but this would increase history stack. https://codereview.chromium.org/2873853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:170: return whenNodeCanInteract(remove).then(function() { On 2017/05/10 22:35:44, stevenjb wrote: > On 2017/05/10 21:48:13, khmel wrote: > > Well, Polymer.async did not help. also SetTimeot 0 does not work always. I > found > > only this way to wait until remove button is active. > > Ugh. Timeout loops like that tend to be fragile in tests. > > This is a lot of subtle / complex change. I would want to bring in some folks > more familiar with our test infrastructure rather than adding yet another > complicated solution. > > I have worked around issues with 'tap' before by calling the tap handler > directly. Can we try doing that instead? > What is your suggestion? .click is not visible. .pressAndReleaseKeyOn is async and also required waiting.
https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) On 2017/05/11 00:10:30, khmel wrote: > On 2017/05/10 22:35:44, stevenjb wrote: > > On 2017/05/10 21:48:13, khmel wrote: > > > in test this causes multiple listener invocation and as result multiple > > > navigateToPreviousRoute (which is async) > > > > This is weird, we should avoid it. The test to settings.getCurrentRoute() > should > > be sufficient? > > > > If not, try adding an observer to androidAppsInfo in the subpage and moving > the > > test there? > > > > In practice, we don't necessarily need this. Any UI action that would cause > this > > change could close the details page, and if playStoreEnabled changes to false > > while the details page is shown, it's not the end of the world > > settings.navigateToPreviousRoute() is async (windows.back) > so settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) will be > true several times. > > it's not the end of the world but the end of the ideal world :) > Frankly speaking that looks really weird. I can close ARC OptIn window. This > disables ARC but settings still show "Remove Android Apps" > We have to handle this case. When would that ever happen in practice? Again, the best solution is probably to put the logic in the sub page instead. https://codereview.chromium.org/2873853002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:166: MockInteractions.tap(remove); Just call: subpage.onRemoveTap_(); Then you shouldn't need whenNodeCanInteract. We don't need to test 'tap' behavior here. The problem is with the test structure not the page itself, and whenNodeCanInteract is a pretty complex (and I think maybe potentially fragile) workaround.
Thanks for comment. PTAL updated version https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) On 2017/05/11 16:58:14, stevenjb wrote: > On 2017/05/11 00:10:30, khmel wrote: > > On 2017/05/10 22:35:44, stevenjb wrote: > > > On 2017/05/10 21:48:13, khmel wrote: > > > > in test this causes multiple listener invocation and as result multiple > > > > navigateToPreviousRoute (which is async) > > > > > > This is weird, we should avoid it. The test to settings.getCurrentRoute() > > should > > > be sufficient? > > > > > > If not, try adding an observer to androidAppsInfo in the subpage and moving > > the > > > test there? > > > > > > In practice, we don't necessarily need this. Any UI action that would cause > > this > > > change could close the details page, and if playStoreEnabled changes to > false > > > while the details page is shown, it's not the end of the world > > > > settings.navigateToPreviousRoute() is async (windows.back) > > so settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) will be > > true several times. > > > > it's not the end of the world but the end of the ideal world :) > > Frankly speaking that looks really weird. I can close ARC OptIn window. This > > disables ARC but settings still show "Remove Android Apps" > > We have to handle this case. > > When would that ever happen in practice? > > Again, the best solution is probably to put the logic in the sub page instead. I described when this happens above. For example open details page. Disable ARC not from settings, for example closing ARC OptIn window when it shows start page, LSO page or error page. ARC is disabled not but details settings are shown. Moved it to subpage. https://codereview.chromium.org/2873853002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/android_apps_page_test.js:166: MockInteractions.tap(remove); On 2017/05/11 16:58:14, stevenjb wrote: > Just call: > > subpage.onRemoveTap_(); > > Then you shouldn't need whenNodeCanInteract. > > We don't need to test 'tap' behavior here. The problem is with the test > structure not the page itself, and whenNodeCanInteract is a pretty complex (and > I think maybe potentially fragile) workaround. Changed to remove.click(). It seems more close what .tap does. WDYT
OK, just a few more comments. Thanks for your patience. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.html:56: android_apps_info="[[androidAppsInfo_]]" prefs="{{prefs}}"> This should still be android-apps-info. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:18: hidden="[[!android_apps_info.settingsAppAvailable]]"> androidAppsInfo.settingsAppAvailable https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:20: androidAppsInfo: Object, androidAppsInfo: { type: Object, observer: 'onAndroidAppsInfoUpdate_', }, https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:45: }, No need for this. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:51: androidAppsInfoUpdate_: function(info) { onAndroidAppsInfoUpdate_: function() { https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:52: if (!info.playStoreEnabled && if (this.androidAppsInfo.playStoreEnabled && https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:53: settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) { This test should no longer be necessary since the subpage will be templated out (i.e. will be removed) when the route is not ANDROID_APPS_DETAILS. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:99: settings.navigateToPreviousRoute(); This should already be navigate back when we disable arc, which is why I'm not sure when/why the code above is needed. https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:103: return promise; nit: return new Promise( ... https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:144: remove.click(); We avoid using click(). If this works... that's fine I guess, but I would still prefer to just call onRemoveTap_ directly.
PTAL https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.html:56: android_apps_info="[[androidAppsInfo_]]" prefs="{{prefs}}"> On 2017/05/11 17:46:54, stevenjb wrote: > This should still be android-apps-info. Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html:18: hidden="[[!android_apps_info.settingsAppAvailable]]"> On 2017/05/11 17:46:54, stevenjb wrote: > androidAppsInfo.settingsAppAvailable Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:20: androidAppsInfo: Object, On 2017/05/11 17:46:54, stevenjb wrote: > androidAppsInfo: { > type: Object, > observer: 'onAndroidAppsInfoUpdate_', > }, Nice, was not aware about this possibility. Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:45: }, On 2017/05/11 17:46:54, stevenjb wrote: > No need for this. Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:51: androidAppsInfoUpdate_: function(info) { On 2017/05/11 17:46:54, stevenjb wrote: > onAndroidAppsInfoUpdate_: function() { Done. However I have to discard root app element from testing to avoid problem when this multiple observers from android_apps_page change this info and as result multiple back requests are issued which breaks the test. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:52: if (!info.playStoreEnabled && On 2017/05/11 17:46:54, stevenjb wrote: > if (this.androidAppsInfo.playStoreEnabled && Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:53: settings.getCurrentRoute() == settings.Route.ANDROID_APPS_DETAILS) { On 2017/05/11 17:46:54, stevenjb wrote: > This test should no longer be necessary since the subpage will be templated out > (i.e. will be removed) when the route is not ANDROID_APPS_DETAILS. Good point, right. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:99: settings.navigateToPreviousRoute(); On 2017/05/11 17:46:54, stevenjb wrote: > This should already be navigate back when we disable arc, which is why I'm not > sure when/why the code above is needed. This goes back only in case when ARC is disabled from settings, but not from other points. For example when ARC OptIn page is closed. https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:103: return promise; On 2017/05/11 17:46:54, stevenjb wrote: > nit: return new Promise( ... Done. https://codereview.chromium.org/2873853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:144: remove.click(); On 2017/05/11 17:46:54, stevenjb wrote: > We avoid using click(). If this works... that's fine I guess, but I would still > prefer to just call onRemoveTap_ directly. Done.
https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:99: settings.navigateToPreviousRoute(); On 2017/05/11 19:09:46, khmel wrote: > On 2017/05/11 17:46:54, stevenjb wrote: > > This should already be navigate back when we disable arc, which is why I'm not > > sure when/why the code above is needed. > > This goes back only in case when ARC is disabled from settings, but not from > other points. For example when ARC OptIn page is closed. Ah, OK, that is the use case I was missing. https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:84: }, There is a better way to do this, see comments in test. (BTW, if we did do this we would also need to declare listener_). https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:118: androidAppsPage.discardForTest(); Instead of adding extra test specific code to the production JS, we should be able to address this by moving: androidAppsPage = document.createElement('settings-android-apps-page'); document.body.appendChild(androidAppsPage); and androidAppsPage.remove(); From the AndroidAppsPageTests suite setup/teardown to each individual suite setup/teardown. (This was an oversight when I introduced the subpage).
https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:118: androidAppsPage.discardForTest(); On 2017/05/11 19:40:43, stevenjb wrote: > Instead of adding extra test specific code to the production JS, we should be > able to address this by moving: > > androidAppsPage = document.createElement('settings-android-apps-page'); > document.body.appendChild(androidAppsPage); > > and > > androidAppsPage.remove(); > > From the AndroidAppsPageTests suite setup/teardown to each individual suite > setup/teardown. > > (This was an oversight when I introduced the subpage). (Note there are 3 sub-suites, 'Main Page' 'SubPage' and 'Enforced', you'll need to move these to each sub suite, but I just verified that works OK and it -should- address the problem you were seeing. Feel free to ping me if it does not).
Thanks Steven for chat! Done as we agreed. PTAL https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:84: }, On 2017/05/11 19:40:43, stevenjb wrote: > There is a better way to do this, see comments in test. > (BTW, if we did do this we would also need to declare listener_). Done as chatted. https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/android_apps_page_test.js:118: androidAppsPage.discardForTest(); On 2017/05/11 19:48:37, stevenjb wrote: > On 2017/05/11 19:40:43, stevenjb wrote: > > Instead of adding extra test specific code to the production JS, we should be > > able to address this by moving: > > > > androidAppsPage = document.createElement('settings-android-apps-page'); > > document.body.appendChild(androidAppsPage); > > > > and > > > > androidAppsPage.remove(); > > > > From the AndroidAppsPageTests suite setup/teardown to each individual suite > > setup/teardown. > > > > (This was an oversight when I introduced the subpage). > > (Note there are 3 sub-suites, 'Main Page' 'SubPage' and 'Enforced', you'll need > to move these to each sub suite, but I just verified that works OK and it > -should- address the problem you were seeing. Feel free to ping me if it does > not). > Done as we discussed in chat. attached/detached
lgtm w/ minor change https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:37: /** @private {!WebUIListener} */ This can just be a member variable like browserProxy_: /** @private {!WebUIListener} */ listener_: null,
Thanks! https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/android_apps_page/android_apps_page.js:37: /** @private {!WebUIListener} */ On 2017/05/11 20:33:51, stevenjb wrote: > This can just be a member variable like browserProxy_: > > /** @private {!WebUIListener} */ > listener_: null, Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2873853002/#ps160001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2873853002/#ps180001 (title: "fix annotations")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2873853002/#ps200001 (title: "fix test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494558969675140, "parent_rev": "a3079e2f1ea9974caebcdebd00c1c33dbaa662e0", "commit_rev": "7bb2e7bc7d31c75b0b7f7b384add49df00d69259"}
Message was sent while issue was closed.
Description was changed from ========== arc: Handle ARC events in MD Settings This provides handling ARC events. TURN ON button should be available in case ARC is disabled and unmanaged. Once ARC is enabled TURN ON button is hidden and APPS section is shown. APPS section shows link to ARC Settings app once Settings app appears in the system. While Settings app is not registered tehn link to ARC Settings is hidden. In case user click on ARC Settings link and Settings app is not ready yet (ARC is still booting) then Settings app is started in deferred mode (spinning animation on shelf). Disabling ARC hides APPS section and TURN ON button is active again. BUG=720173, b/37523579, b/29244888 TEST=Browser test extended, manually on device for managed/unmanaged cases. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== arc: Handle ARC events in MD Settings This provides handling ARC events. TURN ON button should be available in case ARC is disabled and unmanaged. Once ARC is enabled TURN ON button is hidden and APPS section is shown. APPS section shows link to ARC Settings app once Settings app appears in the system. While Settings app is not registered tehn link to ARC Settings is hidden. In case user click on ARC Settings link and Settings app is not ready yet (ARC is still booting) then Settings app is started in deferred mode (spinning animation on shelf). Disabling ARC hides APPS section and TURN ON button is active again. BUG=720173, b/37523579, b/29244888 TEST=Browser test extended, manually on device for managed/unmanaged cases. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2873853002 Cr-Commit-Position: refs/heads/master@{#471206} Committed: https://chromium.googlesource.com/chromium/src/+/7bb2e7bc7d31c75b0b7f7b384add... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/7bb2e7bc7d31c75b0b7f7b384add... |