Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880203002:
Add Google Activity Controls preference (Closed)
DescriptionAdd Google Activity Controls preference
This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference.
BUG=595349
Committed: https://crrev.com/3088202c4058652630f0bb784952a8e4550e5378
Cr-Commit-Position: refs/heads/master@{#388608}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #Patch Set 3 : add user action #
Total comments: 4
Patch Set 4 : #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : #
Total comments: 2
Messages
Total messages: 65 (32 generated)
Description was changed from ========== format format activity control BUG= ========== to ========== Add Google Activity Control preference This CL adds Google Activity control preference in setting sign in account management page and add a icon for sync preference. BUG=595349 ==========
Description was changed from ========== Add Google Activity Control preference This CL adds Google Activity control preference in setting sign in account management page and add a icon for sync preference. BUG=595349 ========== to ========== Add Google Activity Control preference This CL adds Google Activity control preference in setting sign in account management page and an icon for sync preference. BUG=595349 ==========
Description was changed from ========== Add Google Activity Control preference This CL adds Google Activity control preference in setting sign in account management page and an icon for sync preference. BUG=595349 ========== to ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management page and an icon for sync preference. BUG=595349 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management page and an icon for sync preference. BUG=595349 ========== to ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference. BUG=595349 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/80001
gogerald@chromium.org changed reviewers: + bauerb@chromium.org, yusufo@chromium.org
bauerb@chromium.org: Please review changes in chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java chrome/android/java/strings/android_chrome_strings.grd yusufo@chromium.org: Please review changes in chrome/android/java/res/xml/account_management_preferences.xml
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
https://codereview.chromium.org/1880203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:306: Udc.UdcApi Um… you do realize UDC is not a public Google Play Services API? That would also explain your compile errors: the API only exists in the internal copy of the client library (which is the one for first parties). What you want to do here is abstract this away behind an interface, with an implementation upstream that only uses public APIs (if that is possible; otherwise fail as gracefully as possible, or just hide the whole thing), and an implementation downstream that uses first-party APIs. See ChromeApplication and its internal subclass for examples how to do that and select the right implementation.
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/120001
https://codereview.chromium.org/1880203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:306: Udc.UdcApi On 2016/04/14 08:32:30, Bernhard (OOO until Apr 18) wrote: > Um… you do realize UDC is not a public Google Play Services API? That would also > explain your compile errors: the API only exists in the internal copy of the > client library (which is the one for first parties). > > What you want to do here is abstract this away behind an interface, with an > implementation upstream that only uses public APIs (if that is possible; > otherwise fail as gracefully as possible, or just hide the whole thing), and an > implementation downstream that uses first-party APIs. See ChromeApplication and > its internal subclass for examples how to do that and select the right > implementation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
gogerald@chromium.org changed reviewers: + dgn@chromium.org
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/120001
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 gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/140001
Patchset #2 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:294: if (controller != null) { So we'll just do nothing in a ChromePublic build? That seems a bit less than ideal. Is the settings accessible on a website? If so, we could maybe open that? If there is absolutely no way to do something here in a public build, then we should probably just hide the UI element. https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/GoogleActivityController.java (right): https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/GoogleActivityController.java:19: public void openWaaSettings(Activity activity, String accountName); Using the abbreviation does not work well here (it sounds a bit like a baby crying, TBH 😉). openWebAndAppActivitySettings() would be quite a mouthful, but really, that's Java for ya, and it is definitely clearer.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:294: if (controller != null) { On 2016/04/18 15:25:48, Bernhard Bauer wrote: > So we'll just do nothing in a ChromePublic build? That seems a bit less than > ideal. Is the settings accessible on a website? If so, we could maybe open that? > If there is absolutely no way to do something here in a public build, then we > should probably just hide the UI element. Done. https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/GoogleActivityController.java (right): https://codereview.chromium.org/1880203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/GoogleActivityController.java:19: public void openWaaSettings(Activity activity, String accountName); On 2016/04/18 15:25:48, Bernhard Bauer wrote: > Using the abbreviation does not work well here (it sounds a bit like a baby > crying, TBH 😉). openWebAndAppActivitySettings() would be quite a mouthful, but > really, that's Java for ya, and it is definitely clearer. Done.
Thanks! Almost there :) https://codereview.chromium.org/1880203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:299: // Open Google account activity controls settings web page if controller is not Move this into an implementation of the GoogleActivityController interface?
resources lgtm
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/220001
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/240001
https://codereview.chromium.org/1880203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java (right): https://codereview.chromium.org/1880203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementFragment.java:299: // Open Google account activity controls settings web page if controller is not On 2016/04/20 16:46:51, Bernhard Bauer wrote: > Move this into an implementation of the GoogleActivityController interface? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks!
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/1880203002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880203002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880203002/240001
Message was sent while issue was closed.
Description was changed from ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference. BUG=595349 ========== to ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference. BUG=595349 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
https://codereview.chromium.org/1880203002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java (right): https://codereview.chromium.org/1880203002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java:36: "https://myaccount.google.com/activitycontrols/search"; That URL is 404 for me. Do you mean https://myaccount.google.com/privacy#activitycontrols ?
Message was sent while issue was closed.
https://codereview.chromium.org/1880203002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java (right): https://codereview.chromium.org/1880203002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/UrlConstants.java:36: "https://myaccount.google.com/activitycontrols/search"; On 2016/04/21 11:12:14, dgn wrote: > That URL is 404 for me. Do you mean > https://myaccount.google.com/privacy#activitycontrols ? no, this url is not live yet, but it gonna be live soon,
Message was sent while issue was closed.
Description was changed from ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference. BUG=595349 ========== to ========== Add Google Activity Controls preference This CL adds Google Activity Controls preference in setting sign in account management screen and an icon for sync preference. BUG=595349 Committed: https://crrev.com/3088202c4058652630f0bb784952a8e4550e5378 Cr-Commit-Position: refs/heads/master@{#388608} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3088202c4058652630f0bb784952a8e4550e5378 Cr-Commit-Position: refs/heads/master@{#388608} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
