Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237293003:
[FRE] Change the First Run Experience launching behavior (Closed)
|
Created:
4 years, 4 months ago by gogerald1 Modified:
4 years, 4 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, lizeb+watch-custom-tabs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[FRE] Change the First Run Experience launching behavior.
1, launches the Generic First Run Experience when the Lightweight First Run Experience is active.
2, launches the Generic First Run Experience when the Generic First Run Experience is active.
3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode.
4, launches the Generic First Run Experience for View Intents from GSA.
5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps.
6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience.
7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong.
BUG=631453, 627065, 626998, 627003, 628096
Committed: https://crrev.com/818a437efeedba4acb19f90b5c049fa57bbb0d6b
Cr-Commit-Position: refs/heads/master@{#412542}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Messages
Total messages: 53 (46 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== headers draft make sign in activity singleton BUG= ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from Apps except GSA. BUG= ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from Apps except GSA. BUG= ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG= ==========
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG= ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG=631453,627065,626998,627003 ==========
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG=631453,627065,626998,627003 ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG=631453,627065,626998,627003,628096 ==========
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. BUG=631453,627065,626998,627003,628096 ==========
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, Only force trigger the Generic First Run Experience when Chrome is started via Chrome icon. BUG=631453,627065,626998,627003,628096 ==========
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, Only force trigger the Generic First Run Experience when Chrome is started via Chrome icon. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, Only force trigger the Generic First Run Experience when Chrome is started via Chrome icon if things went wrong. BUG=631453,627065,626998,627003,628096 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, Only force trigger the Generic First Run Experience when Chrome is started via Chrome icon if things went wrong. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:130001) has been deleted
Patchset #1 (id:150001) has been deleted
Patchset #1 (id:170001) has been deleted
Patchset #1 (id:190001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [FRE] Change the logic of launching the First Run Experience. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the First Run Experience launching behavior. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ==========
gogerald@chromium.org changed reviewers: + bauerb@chromium.org
Hi, PTAL,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Uff, the logic in this is getting pretty complicated, but I guess that's inherent to the requirements… :-/ https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:174: public static final String SKIP_THE_FIRST_RUN_EXPERIENCE = "skip_the_first_run_experience"; Nit: I would actually use a more succinct style of wording for the constant name and leave out the article, i.e. just SKIP_FIRST_RUN_EXPERIENCE. For comments the article is definitely correct though! https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:538: } else if (activity instanceof LightweightFirstRunActivity) { You don't need an `else` if the previous branch has a `continue`. https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (left): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:237: final boolean fromChromeIcon = Is there a reason you're moving this out to the call sites? https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:239: if (!forLightweightFre) { Invert the check to put the positive branch first?
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/v2/patch-status/codereview.chromium.or...
Yes, the logic gets complicated, I've tried to simplify requirements and implementation, but looks very difficult to get simpler. https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:174: public static final String SKIP_THE_FIRST_RUN_EXPERIENCE = "skip_the_first_run_experience"; On 2016/08/16 23:20:08, Bernhard Bauer wrote: > Nit: I would actually use a more succinct style of wording for the constant name > and leave out the article, i.e. just SKIP_FIRST_RUN_EXPERIENCE. For comments the > article is definitely correct though! Acknowledged. https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:538: } else if (activity instanceof LightweightFirstRunActivity) { On 2016/08/16 23:20:08, Bernhard Bauer wrote: > You don't need an `else` if the previous branch has a `continue`. Done. https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (left): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:237: final boolean fromChromeIcon = On 2016/08/16 23:20:08, Bernhard Bauer wrote: > Is there a reason you're moving this out to the call sites? Done. just made the parameter a little specific, may be keep the original parameter better here to avoid duplication, https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2237293003/diff/210001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:239: if (!forLightweightFre) { On 2016/08/16 23:20:08, Bernhard Bauer wrote: > Invert the check to put the positive branch first? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [FRE] Change the First Run Experience launching behavior. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the First Run Experience launching behavior. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== [FRE] Change the First Run Experience launching behavior. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 ========== to ========== [FRE] Change the First Run Experience launching behavior. 1, launches the Generic First Run Experience when the Lightweight First Run Experience is active. 2, launches the Generic First Run Experience when the Generic First Run Experience is active. 3, lets ChromeTabbedActivity checks and launches the Generic First Run activity for tabbed mode. 4, launches the Generic First Run Experience for View Intents from GSA. 5, launches the Lightweight First Run Experience for View Intents with URLs from other Apps. 6, skip the Generic First Run Experience in ChromeTabbedActivity if ChromeLauncherActivity has launched the Lightweight First Run Experience. 7, force trigger the Generic First Run Experience when Chrome is started via Chrome icon or intent from GSA if things went wrong. BUG=631453,627065,626998,627003,628096 Committed: https://crrev.com/818a437efeedba4acb19f90b5c049fa57bbb0d6b Cr-Commit-Position: refs/heads/master@{#412542} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/818a437efeedba4acb19f90b5c049fa57bbb0d6b Cr-Commit-Position: refs/heads/master@{#412542} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
