Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

Issue 2873853002: arc: Handle ARC events in MD Settings (Closed)

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.

Description

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/+/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)
khmel
Hi Steven PTAL Thanks! https://codereview.chromium.org/2873853002/diff/1/chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc File chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc (left): https://codereview.chromium.org/2873853002/diff/1/chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc#oldcode52 chrome/browser/ui/webui/settings/chromeos/android_apps_handler.cc:52: void AndroidAppsHandler::OnAppReadyChanged(const std::string& app_id, ARC ...
3 years, 7 months ago (2017-05-10 00:35:13 UTC) #3
stevenjb
https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js File chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js#newcode14 chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:14: * appSettingsAvailable: boolean, nit: document each of these. I ...
3 years, 7 months ago (2017-05-10 16:47:03 UTC) #20
khmel
Thank you Steven for comments. please find updated version https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js File chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js#newcode14 chrome/browser/resources/settings/android_apps_page/android_apps_browser_proxy.js:14: ...
3 years, 7 months ago (2017-05-10 21:48:13 UTC) #21
stevenjb
https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/20001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode60 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 ...
3 years, 7 months ago (2017-05-10 22:35:45 UTC) #22
khmel
https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode57 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 ...
3 years, 7 months ago (2017-05-11 00:10:30 UTC) #23
stevenjb
https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode57 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 ...
3 years, 7 months ago (2017-05-11 16:58:14 UTC) #24
khmel
Thanks for comment. PTAL updated version https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/40001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode57 chrome/browser/resources/settings/android_apps_page/android_apps_page.js:57: if (!this.parentElement) On ...
3 years, 7 months ago (2017-05-11 17:32:22 UTC) #25
stevenjb
OK, just a few more comments. Thanks for your patience. https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_page.html File chrome/browser/resources/settings/android_apps_page/android_apps_page.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_page.html#newcode56 ...
3 years, 7 months ago (2017-05-11 17:46:54 UTC) #26
khmel
PTAL https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_page.html File chrome/browser/resources/settings/android_apps_page/android_apps_page.html (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_page.html#newcode56 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: > ...
3 years, 7 months ago (2017-05-11 19:09:46 UTC) #27
stevenjb
https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js (right): https://codereview.chromium.org/2873853002/diff/100001/chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js#newcode99 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 ...
3 years, 7 months ago (2017-05-11 19:40:44 UTC) #28
stevenjb
https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui/settings/android_apps_page_test.js File chrome/test/data/webui/settings/android_apps_page_test.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/test/data/webui/settings/android_apps_page_test.js#newcode118 chrome/test/data/webui/settings/android_apps_page_test.js:118: androidAppsPage.discardForTest(); On 2017/05/11 19:40:43, stevenjb wrote: > Instead of ...
3 years, 7 months ago (2017-05-11 19:48:38 UTC) #29
khmel
Thanks Steven for chat! Done as we agreed. PTAL https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/120001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode84 chrome/browser/resources/settings/android_apps_page/android_apps_page.js:84: ...
3 years, 7 months ago (2017-05-11 20:26:42 UTC) #30
stevenjb
lgtm w/ minor change https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode37 chrome/browser/resources/settings/android_apps_page/android_apps_page.js:37: /** @private {!WebUIListener} */ This ...
3 years, 7 months ago (2017-05-11 20:33:51 UTC) #31
khmel
Thanks! https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js File chrome/browser/resources/settings/android_apps_page/android_apps_page.js (right): https://codereview.chromium.org/2873853002/diff/140001/chrome/browser/resources/settings/android_apps_page/android_apps_page.js#newcode37 chrome/browser/resources/settings/android_apps_page/android_apps_page.js:37: /** @private {!WebUIListener} */ On 2017/05/11 20:33:51, stevenjb ...
3 years, 7 months ago (2017-05-11 20:38:11 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873853002/160001
3 years, 7 months ago (2017-05-11 20:39:45 UTC) #35
commit-bot: I haz the power
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_compilation/builds/8540)
3 years, 7 months ago (2017-05-11 20:56:59 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873853002/180001
3 years, 7 months ago (2017-05-11 21:14:16 UTC) #40
commit-bot: I haz the power
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_asan_rel_ng/builds/368672)
3 years, 7 months ago (2017-05-11 22:23:43 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873853002/180001
3 years, 7 months ago (2017-05-11 22:25:32 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873853002/200001
3 years, 7 months ago (2017-05-12 03:16:40 UTC) #51
commit-bot: I haz the power
3 years, 7 months ago (2017-05-12 03:22:21 UTC) #54
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/7bb2e7bc7d31c75b0b7f7b384add...

Powered by Google App Engine
This is Rietveld 408576698