Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174453002:
Don't require FRE for connecting to custom tabs service (Closed)
DescriptionDon't require FRE for connecting to custom tabs service
We used to return null on bind and avoid connecting in custom tabs
service if FRE is not completed. This change allows bind and connection
but returns false on any code that require network or browser UI
interaction. This way the client can still register a new session and
get callbacks.
BUG=630339
Committed: https://crrev.com/59441af612e4c6922a412b22a783cf8cae3cdcf6
Cr-Commit-Position: refs/heads/master@{#407206}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Nits #Messages
Total messages: 15 (8 generated)
yusufo@chromium.org changed reviewers: + lizeb@chromium.org
The CQ bit was checked by yusufo@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.
https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java (right): https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:23: private Intent mLaunchIntent; nit: mBindIntent is clearer. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:42: boolean firstRunNecessary = FirstRunFlowSequencer Can we move this to a separate method? I know it's silly to move a single line to a method, but can you also free the intent when the answer is "first run ok"? Something like: private boolean isFirstRunDone() { if (mBindIntent == null) return true; boolean firstRunNecessary = FirstRunFlowSequencer .checkIfFirstRunIsNecessary(getApplicationContext(), mBindIntent) != null; if (!firstRunNecessary) { mBindIntent = null; return true; } return false; } It prevents us from leaking the Intent, and makes the checks cheaper. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:56: boolean firstRunNecessary = FirstRunFlowSequencer see above. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:69: boolean firstRunNecessary = FirstRunFlowSequencer see above.
Oh, and LGTM as well with the nits above. On 2016/07/22 12:52:11, Benoit L wrote: > https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java > (right): > > https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:23: > private Intent mLaunchIntent; > nit: mBindIntent is clearer. > > https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:42: > boolean firstRunNecessary = FirstRunFlowSequencer > Can we move this to a separate method? > I know it's silly to move a single line to a method, but can you also free the > intent when the answer is "first run ok"? > > Something like: > > private boolean isFirstRunDone() { > if (mBindIntent == null) return true; > boolean firstRunNecessary = FirstRunFlowSequencer > .checkIfFirstRunIsNecessary(getApplicationContext(), mBindIntent) != > null; > if (!firstRunNecessary) { > mBindIntent = null; > return true; > } > return false; > } > > > It prevents us from leaking the Intent, and makes the checks cheaper. > > https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:56: > boolean firstRunNecessary = FirstRunFlowSequencer > see above. > > https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:69: > boolean firstRunNecessary = FirstRunFlowSequencer > see above.
https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java (right): https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:23: private Intent mLaunchIntent; On 2016/07/22 12:52:10, Benoit L wrote: > nit: mBindIntent is clearer. Done. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:42: boolean firstRunNecessary = FirstRunFlowSequencer On 2016/07/22 12:52:10, Benoit L wrote: > Can we move this to a separate method? > I know it's silly to move a single line to a method, but can you also free the > intent when the answer is "first run ok"? > > Something like: > > private boolean isFirstRunDone() { > if (mBindIntent == null) return true; > boolean firstRunNecessary = FirstRunFlowSequencer > .checkIfFirstRunIsNecessary(getApplicationContext(), mBindIntent) != > null; > if (!firstRunNecessary) { > mBindIntent = null; > return true; > } > return false; > } > > > It prevents us from leaking the Intent, and makes the checks cheaper. Done. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:56: boolean firstRunNecessary = FirstRunFlowSequencer On 2016/07/22 12:52:10, Benoit L wrote: > see above. Done. https://codereview.chromium.org/2174453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java:69: boolean firstRunNecessary = FirstRunFlowSequencer On 2016/07/22 12:52:10, Benoit L wrote: > see above. Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2174453002/#ps20001 (title: "Nits")
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Don't require FRE for connecting to custom tabs service We used to return null on bind and avoid connecting in custom tabs service if FRE is not completed. This change allows bind and connection but returns false on any code that require network or browser UI interaction. This way the client can still register a new session and get callbacks. BUG=630339 ========== to ========== Don't require FRE for connecting to custom tabs service We used to return null on bind and avoid connecting in custom tabs service if FRE is not completed. This change allows bind and connection but returns false on any code that require network or browser UI interaction. This way the client can still register a new session and get callbacks. BUG=630339 Committed: https://crrev.com/59441af612e4c6922a412b22a783cf8cae3cdcf6 Cr-Commit-Position: refs/heads/master@{#407206} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/59441af612e4c6922a412b22a783cf8cae3cdcf6 Cr-Commit-Position: refs/heads/master@{#407206} |
