Check first run finished before custom tabs feature is used

Check first run finished before custom tabs feature is used

This avoid connecting to custom tabs service or launching a
custom tab before
the user acknowledges ToS.

BUG=515654
Committed: https://crrev.com/83b2aa790bfd6c468bc9fe094d1d49db91fe01e0
Cr-Commit-Position: refs/heads/master@{#342937}

[Yusuf, 2015-08-11 18:48:38]

yusufo@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Maria, 2015-08-11 18:54:52]

I am not sure this is a good user experience. Say I get a new phone and I am an
active twitter/facebook/whatever user, chances are I might end up wanting a
ChromeTab long before I launch Chrome for the first time from the home icon.

It seems like we need some sort of flow for accepting ToS via ChromeTab. This
seems like it would be a pretty common case and it'll look broken.

https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:244:
if (!FirstRunStatus.getFirstRunFlowComplete(getApplicationContext())) return
false;
Should you be using the logic in checkIfFirstRunIsNecessary() instead?

[Maria, 2015-08-11 18:55:26]

On 2015/08/11 18:54:52, Maria wrote:
> I am not sure this is a good user experience. Say I get a new phone and I am
an
> active twitter/facebook/whatever user, chances are I might end up wanting a
> ChromeTab long before I launch Chrome for the first time from the home icon.
> 
> It seems like we need some sort of flow for accepting ToS via ChromeTab. This
> seems like it would be a pretty common case and it'll look broken.
> 
>
https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
> (right):
> 
>
https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:244:
> if (!FirstRunStatus.getFirstRunFlowComplete(getApplicationContext())) return
> false;
> Should you be using the logic in checkIfFirstRunIsNecessary() instead?

s/ChromeTab/CustomTab/g

[Yusuf, 2015-08-11 20:29:10]

Changed it so that we actually go through first run.

https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1274953006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:244:
if (!FirstRunStatus.getFirstRunFlowComplete(getApplicationContext())) return
false;
On 2015/08/11 18:54:52, Maria wrote:
> Should you be using the logic in checkIfFirstRunIsNecessary() instead?

Done. and done in the above code as well. I didn't realize this would create
testing issues.

[Maria, 2015-08-11 21:09:17]

I like this approach much better! Just some nits

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:196:
return;
maybe just make it part of the if/else if/else statement below and then the
return will happen after in one place for all of these?

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:257:
private void handleCustomTabActivityIntent() {
how about calling this launchCustomTabActivity() instead to be similar to
launchTabbedMode/launchDocumentActivity/etc.

I also think it perhaps makes sense to call finish() outside this code -- once
again for consistency with other code paths.

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:269:
return;
don't need return here

[Yusuf, 2015-08-11 21:49:57]

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
(right):

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:196:
return;
On 2015/08/11 21:09:16, Maria wrote:
> maybe just make it part of the if/else if/else statement below and then the
> return will happen after in one place for all of these?

Done.

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:257:
private void handleCustomTabActivityIntent() {
On 2015/08/11 21:09:16, Maria wrote:
> how about calling this launchCustomTabActivity() instead to be similar to
> launchTabbedMode/launchDocumentActivity/etc.
> 
> I also think it perhaps makes sense to call finish() outside this code -- once
> again for consistency with other code paths.

Done.

https://codereview.chromium.org/1274953006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:269:
return;
On 2015/08/11 21:09:16, Maria wrote:
> don't need return here

Done.

[Maria, 2015-08-11 23:02:05]

lgtm

[Yusuf, 2015-08-11 23:03:01]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2015-08-11 23:03:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274953006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274953006/40001

[commit-bot: I haz the power, 2015-08-11 23:41:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-11 23:42:34]

Patchset 3 (id:??) landed as
https://crrev.com/83b2aa790bfd6c468bc9fe094d1d49db91fe01e0
Cr-Commit-Position: refs/heads/master@{#342937}