customtabs: Refactor CustomTabsConnection to simplify locking.

customtabs: Refactor CustomTabsConnection to simplify locking.

This refactoring of CustomTabsConnection has two benefits:
- Move the client and session handling logic away from CustomTabsConnection.
- Simplify locking in CustomTabsConnection, as it is all handled by
  ClientManager.

Some methods in CustomTabsConnection are now simple pass-through. These
will be removed in a subsequent CL. Leaving these here means that no
test code is affected.

Committed: https://crrev.com/475d09792533426eef213af553019eb543b1abbc
Cr-Commit-Position: refs/heads/master@{#351146}

[Benoit L, 2015-09-24 14:46:47]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2015-09-24 15:54:08]

thanks!

initial set of comments to make locking simpler (and more expensive haha)

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(right):

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:30:
/** Manages the clients state for Custom Tabs. */
s/clients state/clients' state/

or:

s/clients state/state of the clients/

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:31:
class ClientManager {
a comment should also say that this class is threadsafe

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:40:
public final int mUid;
android styleguid asks to name public fields in camelCase (i.e. without a
leading 'm')

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:44:
private ServiceConnection mServiceConnection;
this is a keepalive connection, right? It would be more readable to name it
mKeepAliveConnection.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:59:
private Referrer constructReferrer(Context context) {
looks like a static method

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:117:
cleanupAlreadyLocked(session);
locks in java are reentrant, we paid a price for it already in the design of the
language, so reentering is cheap for any lock.

I would suggest just making cleanupSession synchronized, and just call it in
cleanupAll enough times.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:158:
public void registerLaunch(IBinder session, String url) {
please just make it synchronized for simplicity, recording histograms is
shortlived

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:230:
if (ok) params.setServiceConnection(connection);
it is nontrivial to verify that dontKeepAliveForSession is not called while this
is being executed, so readers would be concerned with a race condition. Why not
making the whole method synchronized?

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:243:
mContext.unbindService(connection);
why do we want to unbind outside synchronization block? is it avoiding some
measurable bottleneck?

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:259:
synchronized (this) {
public synchronized void

[Benoit L, 2015-09-24 16:42:14]

Thanks!
All done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(right):

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:30:
/** Manages the clients state for Custom Tabs. */
On 2015/09/24 15:54:08, pasko wrote:
> s/clients state/clients' state/
> 
> or:
> 
> s/clients state/state of the clients/

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:31:
class ClientManager {
On 2015/09/24 15:54:08, pasko wrote:
> a comment should also say that this class is threadsafe

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:40:
public final int mUid;
On 2015/09/24 15:54:08, pasko wrote:
> android styleguid asks to name public fields in camelCase (i.e. without a
> leading 'm')

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:44:
private ServiceConnection mServiceConnection;
On 2015/09/24 15:54:08, pasko wrote:
> this is a keepalive connection, right? It would be more readable to name it
> mKeepAliveConnection.

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:59:
private Referrer constructReferrer(Context context) {
On 2015/09/24 15:54:08, pasko wrote:
> looks like a static method

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:117:
cleanupAlreadyLocked(session);
On 2015/09/24 15:54:08, pasko wrote:
> locks in java are reentrant, we paid a price for it already in the design of
the
> language, so reentering is cheap for any lock.
> 
> I would suggest just making cleanupSession synchronized, and just call it in
> cleanupAll enough times.

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:158:
public void registerLaunch(IBinder session, String url) {
On 2015/09/24 15:54:08, pasko wrote:
> please just make it synchronized for simplicity, recording histograms is
> shortlived

Done.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:230:
if (ok) params.setServiceConnection(connection);
On 2015/09/24 15:54:08, pasko wrote:
> it is nontrivial to verify that dontKeepAliveForSession is not called while
this
> is being executed, so readers would be concerned with a race condition. Why
not
> making the whole method synchronized?

Made sure that the method is called from the UI thread (which is currently the
case).

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:243:
mContext.unbindService(connection);
On 2015/09/24 15:54:08, pasko wrote:
> why do we want to unbind outside synchronization block? is it avoiding some
> measurable bottleneck?

This is interacting with the framework and potentially a remote application, so
I prefer to keep it out.

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:259:
synchronized (this) {
On 2015/09/24 15:54:08, pasko wrote:
> public synchronized void

Done.

[pasko, 2015-09-25 15:58:42]

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(right):

https://codereview.chromium.org/1370473002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:230:
if (ok) params.setServiceConnection(connection);
On 2015/09/24 16:42:14, Benoit L wrote:
> On 2015/09/24 15:54:08, pasko wrote:
> > it is nontrivial to verify that dontKeepAliveForSession is not called while
> this
> > is being executed, so readers would be concerned with a race condition. Why
> not
> > making the whole method synchronized?
> 
> Made sure that the method is called from the UI thread (which is currently the
> case).

But this breaks incapsulation. Please make this class threadsafe instead.

[Benoit L, 2015-09-28 16:31:42]

Done, thank you.
PTAL.

[pasko, 2015-09-28 17:51:57]

lgtm, thank you for actually updating it, I almost forgot about it, but now I
really enjoy additional clarity. Thank you.

[Benoit L, 2015-09-28 17:57:57]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2015-09-28 17:59:52]

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

[commit-bot: I haz the power, 2015-09-28 18:42:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-28 18:42:26]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Benoit L, 2015-09-28 19:05:16]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2015-09-28 19:05:17]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1370473002/#ps100001
(title: "Quiet, Findbugs!")

[commit-bot: I haz the power, 2015-09-28 19:14:24]

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

[commit-bot: I haz the power, 2015-09-28 21:01:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-28 21:02:49]

Patchset 6 (id:??) landed as
https://crrev.com/475d09792533426eef213af553019eb543b1abbc
Cr-Commit-Position: refs/heads/master@{#351146}