Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062083002:
Implements the Lightweight First Run Experience (Closed)
DescriptionImplements the Lightweight First Run Experience
Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App).
And show ToS dialog before loading URL in Chrome.
Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS.
Check screenshot in comment #64 of http://crbug.com/450423.
BUG=450423, 612879
Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee
Cr-Commit-Position: refs/heads/master@{#403702}
Patch Set 1 #Patch Set 2 : format #
Total comments: 6
Patch Set 3 : address comments #
Total comments: 4
Patch Set 4 : address comments #
Total comments: 6
Patch Set 5 : address comments and update behaviors #Patch Set 6 : format #Patch Set 7 : rebase #Messages
Total messages: 76 (45 generated)
Description was changed from ========== c version draft BUG= ========== to ========== c version draft BUG= ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
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
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062083002/120001
Description was changed from ========== c version draft BUG= ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ==========
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except intents from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ==========
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except intents from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ==========
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted. BUG=612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. BUG=612879 ==========
Patchset #1 (id:120001) 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/2062083002/140001
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/2062083002/160001
gogerald@chromium.org changed reviewers: + bauerb@chromium.org, yusufo@chromium.org
yusufo@chromium.org: Please review changes in *.xml and customtabs/ bauerb@chromium.org: Please review changes in the rest of the files
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. BUG=612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=612879 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:492: IntentHandler.determineExternalIntentSource(getPackageName(), getIntent())); You pass this to checkIfFirstRunIsNecessary() every single time. Could that method just determine the external intent source? https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:239: } else { The else is not necessary if you have a return statement in the preceding branch. https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:69: // Skip sign in if Chrome is neighter started via Chrome icon nor GSA (Google Search App). Nit: "neither"?
Patchset #3 (id:180001) 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/2062083002/200001
https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:492: IntentHandler.determineExternalIntentSource(getPackageName(), getIntent())); On 2016/06/16 16:35:36, Bernhard Bauer wrote: > You pass this to checkIfFirstRunIsNecessary() every single time. Could that > method just determine the external intent source? Done. https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:239: } else { On 2016/06/16 16:35:36, Bernhard Bauer wrote: > The else is not necessary if you have a return statement in the preceding > branch. Done. https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java (right): https://codereview.chromium.org/2062083002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunSignInProcessor.java:69: // Skip sign in if Chrome is neighter started via Chrome icon nor GSA (Google Search App). On 2016/06/16 16:35:36, Bernhard Bauer wrote: > Nit: "neither"? Done.
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:220: * @param fromIntent That was used to launch Chrome. It contains the information of the client "The intent that was used [...]". https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241: ? ExternalAppId.OTHER We only use this to whether whether the opening package is GSA, so we could just inline that check: (fromIntent != null) && IntentHandler.determineExternalIntentSource(context.getPackageName(), fromIntent) == ExternalAppId.GSA
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:220: * @param fromIntent That was used to launch Chrome. It contains the information of the client On 2016/06/17 09:48:15, Bernhard Bauer wrote: > "The intent that was used [...]". Done. https://codereview.chromium.org/2062083002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241: ? ExternalAppId.OTHER On 2016/06/17 09:48:16, Bernhard Bauer wrote: > We only use this to whether whether the opening package is GSA, so we could just > inline that check: > > (fromIntent != null) && > IntentHandler.determineExternalIntentSource(context.getPackageName(), > fromIntent) == ExternalAppId.GSA Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062083002/220001
LGTM w/ a formatting nit: https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:240: if (fromChromeIcon || (fromIntent != null Nit: I would probably break after the `||` (i.e. don't try to align subexpressions *and* indent eight spaces on break, as you'll end up squashed to the right).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241: && IntentHandler.determineExternalIntentSource( This part is rather fragile as we assume that GSA sends this extra in all exit points all the time (which is definitely not the case right now). APPLICATION_ID extra is more a recommendation to include than a requirement for launching a VIEW intent or a custom tab. I would much rather we use CustomTabsConnection#extractCreatorPackage for this. https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunStatus.java (right): https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunStatus.java:56: public static boolean getSkipWelcomePage(Context context) { I would say shouldSkipWelcomePage is more the norm and more readable.
Patchset #6 (id:260001) has been deleted
Patchset #5 (id:240001) 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/2062083002/280001
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/2062083002/300001
Hi, please take another look, 1, addressed the comments. 2, update string according to requirement. 3, add a new preference for lightweight first run experience to avoid dependency of native library. 4, launch the first run experience for view intents with urls before loading the urls in the background. https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:240: if (fromChromeIcon || (fromIntent != null On 2016/06/17 15:15:53, Bernhard Bauer wrote: > Nit: I would probably break after the `||` (i.e. don't try to align > subexpressions *and* indent eight spaces on break, as you'll end up squashed to > the right). Done. https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241: && IntentHandler.determineExternalIntentSource( On 2016/06/20 10:00:24, Yusuf wrote: > This part is rather fragile as we assume that GSA sends this extra in all exit > points all the time (which is definitely not the case right now). APPLICATION_ID > extra is more a recommendation to include than a requirement for launching a > VIEW intent or a custom tab. > > I would much rather we use CustomTabsConnection#extractCreatorPackage for this. GSA is ready to send APPLICATION_ID in the latest version, keep using IntentHandler.determineExternalIntentSource as we discussed. https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunStatus.java (right): https://codereview.chromium.org/2062083002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunStatus.java:56: public static boolean getSkipWelcomePage(Context context) { On 2016/06/20 10:00:24, Yusuf wrote: > I would say shouldSkipWelcomePage is more the norm and more readable. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Show ToS dialog before loading URL in Chrome Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ==========
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). Show ToS dialog before loading URL in Chrome Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ==========
lgtm thank you
anthonyvd@chromium.org changed reviewers: + anthonyvd@chromium.org
Hi everyone, Ganggui has been on vacation since this CL was l-g-t-m'd. Since the team would like to get this in before M53 branch, I'd like to hit commit this afternoon. Please let me know if you have any issue with that, although it looks like this looks good to everyone. Thanks!
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2062083002/#ps300001 (title: "format")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2062083002/#ps320001 (title: "rebase")
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 ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702}
Message was sent while issue was closed.
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshots in the bug. BUG=450423, 612879 Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702} ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshot in comment #64 of Issue 450423. BUG=450423, 612879 Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702} ==========
Message was sent while issue was closed.
Description was changed from ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshot in comment #64 of Issue 450423. BUG=450423, 612879 Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702} ========== to ========== Implements the Lightweight First Run Experience Only show ToS dialog if Chrome is opened via VIEW intents except from GSA (Google Search App). And show ToS dialog before loading URL in Chrome. Always show Welcome page with ToS if Chrome is opened via Chrome icon and the user has not accepted ToS. Check screenshot in comment #64 of http://crbug.com/450423. BUG=450423, 612879 Committed: https://crrev.com/57cee59c3b7566f14a67b7a982447f5ea3b6bbee Cr-Commit-Position: refs/heads/master@{#403702} ========== |
