Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559573002:
Disable UI options that allow new tabs before FRE. (Closed)
DescriptionDisable UI options that allow new tabs before FRE.
Prior to completing the FRE on Android, we should disallow
UI options that allow creating new tabs in Chrome.
This disables "Web Search" from the action mode menu and
also the various "Open in X" menu options.
BUG=671149
Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b
Committed: https://crrev.com/b467bb5751deeedd0238fe4ea22c1832470fc284
Cr-Original-Commit-Position: refs/heads/master@{#437670}
Cr-Commit-Position: refs/heads/master@{#438340}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Nits #Patch Set 3 : Fix tests #Patch Set 4 : Fix tests for reals #Patch Set 5 : Fixed downstream and fixed tests #Messages
Total messages: 43 (22 generated)
tedchoc@chromium.org changed reviewers: + asvitkine@chromium.org, yusufo@chromium.org
PTAL Either one of you can review it...it is a race!
lgtm
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm - looks like Yusuf beat to me to the review, but some suggestions for comments below https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:42: int allowedActionModes = ActionModeCallbackHelper.MENU_ITEM_PROCESS_TEXT Nit: Maybe worth adding a comment here. https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:276: // Hide all items that could spawn additional tabs until FRE has been completed. Can you expand this comment to mention how these items are made visible again?
The CQ bit was unchecked by tedchoc@chromium.org
https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java (right): https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java:42: int allowedActionModes = ActionModeCallbackHelper.MENU_ITEM_PROCESS_TEXT On 2016/12/06 21:42:35, Alexei Svitkine wrote: > Nit: Maybe worth adding a comment here. Done. https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2559573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:276: // Hide all items that could spawn additional tabs until FRE has been completed. On 2016/12/06 21:42:35, Alexei Svitkine wrote: > Can you expand this comment to mention how these items are made visible again? The menu is constructed every time you call it, so there is no need to set them visible again. I'd argue the comment is unnecessary in this file since we mark all items as invisible so commenting here would imply we needed every where.
SG - sorry, wasn't familiar with the menu code. On Tue, Dec 6, 2016 at 4:56 PM, <tedchoc@chromium.org> wrote: > > https://codereview.chromium.org/2559573002/diff/1/chrome/ > android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java > File > chrome/android/java/src/org/chromium/chrome/browser/ > ChromeActionModeCallback.java > (right): > > https://codereview.chromium.org/2559573002/diff/1/chrome/ > android/java/src/org/chromium/chrome/browser/ > ChromeActionModeCallback.java#newcode42 > chrome/android/java/src/org/chromium/chrome/browser/ > ChromeActionModeCallback.java:42: > int allowedActionModes = ActionModeCallbackHelper.MENU_ITEM_PROCESS_TEXT > On 2016/12/06 21:42:35, Alexei Svitkine wrote: > > Nit: Maybe worth adding a comment here. > > Done. > > https://codereview.chromium.org/2559573002/diff/1/chrome/ > android/java/src/org/chromium/chrome/browser/contextmenu/ > ChromeContextMenuPopulator.java > File > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ > ChromeContextMenuPopulator.java > (right): > > https://codereview.chromium.org/2559573002/diff/1/chrome/ > android/java/src/org/chromium/chrome/browser/contextmenu/ > ChromeContextMenuPopulator.java#newcode276 > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ > ChromeContextMenuPopulator.java:276: > // Hide all items that could spawn additional tabs until FRE has been > completed. > On 2016/12/06 21:42:35, Alexei Svitkine wrote: > > Can you expand this comment to mention how these items are made > visible again? > > The menu is constructed every time you call it, so there is no need to > set them visible again. I'd argue the comment is unnecessary in this > file since we mark all items as invisible so commenting here would imply > we needed every where. > > https://codereview.chromium.org/2559573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2559573002/#ps20001 (title: "Nits")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/12/07 23:28:41, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) Got the offline 👍 from yusufo@ that the test changes look ok to mark FRE as complete. The instrumentation tests pass ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE which only prevents FRE but the system believes it has not run.
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2559573002/#ps40001 (title: "Fix tests")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tedchoc@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 unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2559573002/#ps60001 (title: "Fix tests for reals")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/08 06:14:08, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) The tests were failing because marking first run as complete results in the test being opted into the default finch groups (i.e. elderberry for herb). To work around this, I marked FRE as complete after the initial activity has been loaded (which means native has already started and the users are not auto placed in finch groups).
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481318963385460,
"parent_rev": "3a2258bd2eadd49804576be9f937bb01cd8cb663", "commit_rev":
"a02366f1aef48d61d0c65c6030fc744bd2e52814"}
Message was sent while issue was closed.
Description was changed from ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 ========== to ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Review-Url: https://codereview.chromium.org/2559573002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2566963002/ by perezju@chromium.org. The reason for reverting is: Appears to have broken clank builds, and its blocking M57 Dev release.
Message was sent while issue was closed.
Description was changed from ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Review-Url: https://codereview.chromium.org/2559573002 ========== to ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670}
Message was sent while issue was closed.
Description was changed from ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} ========== to ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} ==========
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2559573002/#ps80001 (title: "Fixed downstream and fixed tests")
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": 80001, "attempt_start_ts": 1481670733497860,
"parent_rev": "7aee73d522469c8be9bc118eb506d4ba69a564df", "commit_rev":
"b59e0817b00adc3f7e577a00807ae486ee54c745"}
Message was sent while issue was closed.
Description was changed from ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} ========== to ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} Review-Url: https://codereview.chromium.org/2559573002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Cr-Commit-Position: refs/heads/master@{#437670} Review-Url: https://codereview.chromium.org/2559573002 ========== to ========== Disable UI options that allow new tabs before FRE. Prior to completing the FRE on Android, we should disallow UI options that allow creating new tabs in Chrome. This disables "Web Search" from the action mode menu and also the various "Open in X" menu options. BUG=671149 Committed: https://crrev.com/68b6254c56e0a535e687aeaa1d9d46c25c9ddd9b Committed: https://crrev.com/b467bb5751deeedd0238fe4ea22c1832470fc284 Cr-Original-Commit-Position: refs/heads/master@{#437670} Cr-Commit-Position: refs/heads/master@{#438340} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b467bb5751deeedd0238fe4ea22c1832470fc284 Cr-Commit-Position: refs/heads/master@{#438340} |
