DevTools socket tunnel.

Implementation of DevTools socket tunneling service for remote debugging.

Test APK in addition to instrumentation tests contains launching activity and foreground service what let duplicate ChromeShell debug socket to a differently named socket. If a device with running ChromeShell and DevToolsBridgeTest's service connect to a Desktop chrome (with USB debugging enabled) then chrome://inspect#devices will show 2 ChromeChell items. Both are debuggable (https://codereview.chromium.org/521573002/ should be applied to Desktop chrome to make the process less surprising). It's suitable for manual tests.

This CL doesn't care of a few known issues:
1. Data channel buffer overflow. Data channel automatically closed in this case.
2. Uncontrolled number of pending sockets. DevTools throttles opening sockets and and keeps sockets that exceeds a threshold hanging.
3. Uncontrolled number of threads (it's actually not a problem if #2 solved, thanks to DevTools throttling).

To keep this CL reasonably simple this issues will be addressed later.

TEST=org.chromium.components.devtools_bridge.SocketTunnelServerTest, see description fro manual testing.
BUG=383418
Committed: https://crrev.com/15044609893faf9b2c2a533aa9a136f288ea20a1
Cr-Commit-Position: refs/heads/master@{#296360}

Committed: https://crrev.com/6c788d675dba4ec185375863cbc53c0803cc4b3e
Cr-Commit-Position: refs/heads/master@{#296377}

[SeRya, 2014-09-04 13:57:21]

Patchset #1 (id:20001) has been deleted

[SeRya, 2014-09-11 17:32:22]

serya@chromium.org changed reviewers:
+ mnaganov@chromium.org

[mnaganov (inactive), 2014-09-12 13:37:30]

Here are the issues I have noticed so far. As the patch is quite big, I'll
revisit it later.

https://codereview.chromium.org/517233002/diff/100001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/517233002/diff/100001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml:36: android:label="Debug web
pages"
Does this need a clarification, like "from external apps"?

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:16:
public abstract class AbstractDataChannel {
Do we need to mention here that data channel is threadsafe?

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:22:
void onStateChange(boolean open);
Does it make sense to use an enum here -- for clarity, and for potential
extension?

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:26:
// the reference to the buffer must not outlive the invokation.
typo: invocation

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:79:
// For writing in socket wothout blocking signaling thread.
typo: without

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:132:
// When integrity of data channel is broken then best way to survive is to close
it.
Do we also need to unbind here?

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:52:
boolean hasConnections() {
nit: use @VisibleForTesting

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:123:
if (mSignalingThread.get() != Thread.currentThread())
I think you should compare thread IDs instead of comparing object references.

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SignalingThreadMock.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SignalingThreadMock.java:21:
private static final int EXECUTION_TIME_LIMIT_MS = 5000;
I think, we need to use ScalableTimeout, because tests can run in "slow" mode
under Valgrind.

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelClient.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelClient.java:111:
boolean hasConnections() {
nit: It's already in a test

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java:58:
return new String(buffer, 0, position);
Hmmm, why are sure that it converts it into UTF-8 if you don't specify charset?

[SeRya, 2014-09-15 15:20:01]

Comments addressed, looking forward for the next portion of comments.

https://codereview.chromium.org/517233002/diff/100001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/517233002/diff/100001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml:36: android:label="Debug web
pages"
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> Does this need a clarification, like "from external apps"?

Theoretically this label may be shown when user installs an external
application. It will be among requested permissions (not sure it's the case for
signature protected permissions). So text like "Debug web pages in Chrome Shell"
would be more appropriate. In reality it's hardly someone needs to publish an
application with what permission. When you install an app through ADB the
message is not shown.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:16:
public abstract class AbstractDataChannel {
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> Do we need to mention here that data channel is threadsafe?

It's worth to be mentioned indeed.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:22:
void onStateChange(boolean open);
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> Does it make sense to use an enum here -- for clarity, and for potential
> extension?

Done.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:26:
// the reference to the buffer must not outlive the invokation.
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> typo: invocation

Done.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:79:
// For writing in socket wothout blocking signaling thread.
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> typo: without

Done.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:132:
// When integrity of data channel is broken then best way to survive is to close
it.
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> Do we also need to unbind here?

I don't think so. Unbind method designated for class users. Client code would be
more complicated if tunnel may suddenly unbind itself.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:52:
boolean hasConnections() {
On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> nit: use @VisibleForTesting

Let's rather make it public.
https://sites.google.com/a/google.com/codehealth/refactoring-gallery/-visible...

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:123:
if (mSignalingThread.get() != Thread.currentThread())
On 2014/09/12 13:37:30, mnaganov (cr) wrote:
> I think you should compare thread IDs instead of comparing object references.

Please explain why. I see no real benefits but see a small drawback: reference
has natural placeholder for unknown value: null. There is no such for integer.

I actually spotted another issue looking at your comment. Single threaded
executor (in contrast with WebRTC's signaling thread) does not guarantee that
executing thread will always be the same. I have never seen it in manual or
automatic tests but it safer to manage threads manually. So I updated
SignalingThreadMock. If executor change the thread this check would fail
(doesn't matter reference or ID checked).

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SignalingThreadMock.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SignalingThreadMock.java:21:
private static final int EXECUTION_TIME_LIMIT_MS = 5000;
On 2014/09/12 13:37:30, mnaganov (cr) wrote:
> I think, we need to use ScalableTimeout, because tests can run in "slow" mode
> under Valgrind.

There is technical issue with adding dependency on base_javatests. Next patch
will add dependency on libjingle_peerconnection_so. Having both native libraries
simultaneously would be headache. However as I proposed in email conversation
finally it won't depend on Java API and dependency on base_javatests won't be an
issue. So I added TODO for now.

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelClient.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelClient.java:111:
boolean hasConnections() {
On 2014/09/12 13:37:30, mnaganov (cr) wrote:
> nit: It's already in a test

Done.

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java:58:
return new String(buffer, 0, position);
On 2014/09/12 13:37:30, mnaganov (cr) wrote:
> Hmmm, why are sure that it converts it into UTF-8 if you don't specify
charset?

Right, it's better to specify charset.

[mnaganov (inactive), 2014-09-15 16:35:22]

Looks better! Some more comments.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:52:
boolean hasConnections() {
On 2014/09/15 15:20:00, SeRya wrote:
> On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> > nit: use @VisibleForTesting
> 
> Let's rather make it public.
>
https://sites.google.com/a/google.com/codehealth/refactoring-gallery/-visible...

Still not see it in PS6.

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/120001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:123:
if (mSignalingThread.get() != Thread.currentThread())
On 2014/09/15 15:20:00, SeRya wrote:
> On 2014/09/12 13:37:30, mnaganov (cr) wrote:
> > I think you should compare thread IDs instead of comparing object
references.
> 
> Please explain why. I see no real benefits but see a small drawback: reference
> has natural placeholder for unknown value: null. There is no such for integer.
> 
> I actually spotted another issue looking at your comment. Single threaded
> executor (in contrast with WebRTC's signaling thread) does not guarantee that
> executing thread will always be the same. I have never seen it in manual or
> automatic tests but it safer to manage threads manually. So I updated
> SignalingThreadMock. If executor change the thread this check would fail
> (doesn't matter reference or ID checked).

I was thinking that "Thread.currentThread" is not required to return the same
object every time (at least, I didn't see any explicit requirements for this),
but it looks like this will interact poorly with ThreadGroup.enumerate(), so
this will not make much sense.

Also, the existing Java code in Chromium seems to compare thread ids. But if it
seems more convenient for you to compare object references, let's leave as is.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File components/devtools_bridge.gyp (right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
nit: remove "(c)"

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:15:
* Data channel is thread safe (excep the dispose method).
typo: "except"

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:53:
// Structure of data packet:
Is it true that data packets with empty payload are allowed? If yes, you may
mention it here, otherwise prohibit at building / decoding.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:156:
ByteBuffer packet = ByteBuffer.allocateDirect(3);
I think it's worth adding a constant for the control packet size, and then using
it here and in 'decodePacket'.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:43:
// Safe to access mConnection once AbstractDataChannel.Observer detached.
What is "mConnection" -- did you mean "mClientConnections"? Also, by "safe to
access" do you mean that this can be done on any thread, so no
"checkCalledOnSignalingThread()" is needed or something else?

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:122:
assert mServerConnections.containsKey(mId);
Hmm, if you can have concurrent modifications of "mServerConnections", then this
assert can be out of date at the time you call "remove". Does it make sense?

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File components/devtools_bridge/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/AndroidManifest.xml:2: <!--
Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of
nit: remove "(c)"

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/LocalTunnelBridgeTest.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/LocalTunnelBridgeTest.java:50:
mBridge.dispose();
Should also reset the reference just in case.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java:196:
new int[] { 1, 5, 100, 1000, SocketTunnelBase.READING_BUFFER_SIZE * 2 };
If data packets with empty payload are allowed, should also test for 0.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/tests/DebugActivity.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/tests/DebugActivity.java:34:
"5. Open chroome://inspect#devices on desktop Chrome.\n" +
typo: "chrome:"

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java:20:
private static final String CHARSET = "UTF8";
nit: I think "UTF-8" is more standard spelling. "UTF8" is kind of an alias
defined in JDK. See
http://www.iana.org/assignments/character-sets/character-sets.xhtml

[SeRya, 2014-09-15 18:16:22]

Comments addressed.

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:52:
boolean hasConnections() {
On 2014/09/15 16:35:21, mnaganov (cr) wrote:
> On 2014/09/15 15:20:00, SeRya wrote:
> > On 2014/09/12 13:37:29, mnaganov (cr) wrote:
> > > nit: use @VisibleForTesting
> > 
> > Let's rather make it public.
> >
>
https://sites.google.com/a/google.com/codehealth/refactoring-gallery/-visible...
> 
> Still not see it in PS6.

Sory. Lost this change. Fixed now.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:15:
* Data channel is thread safe (excep the dispose method).
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> typo: "except"

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:53:
// Structure of data packet:
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> Is it true that data packets with empty payload are allowed? If yes, you may
> mention it here, otherwise prohibit at building / decoding.

There is no way empty data packet could be created when reading socket but there
is nothing wrong with incoming empty packet. I don't feel what comment would add
any useful knowledge.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:156:
ByteBuffer packet = ByteBuffer.allocateDirect(3);
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> I think it's worth adding a constant for the control packet size, and then
using
> it here and in 'decodePacket'.

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:43:
// Safe to access mConnection once AbstractDataChannel.Observer detached.
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> What is "mConnection" -- did you mean "mClientConnections"? Also, by "safe to
> access" do you mean that this can be done on any thread, so no
> "checkCalledOnSignalingThread()" is needed or something else?

I meant mClientConnections indeed. Fixed.

Binding and unbinding to/from a data channel must be synchronized by an owner
(as well as calling all public methods). When data channel bound callback may be
processed on signaling thread. Once observer detached (this operation
synchronized with signaling thread) interference with what callback no longer
possible.

The unbind method called on owner's thread (not necessary on the same thread
object created or started, but owner is responsible for synchronization if that
different threads).

I updated the comments hoping it's helps understand.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelServer.java:122:
assert mServerConnections.containsKey(mId);
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> Hmm, if you can have concurrent modifications of "mServerConnections", then
this
> assert can be out of date at the time you call "remove". Does it make sense?

Items could be added concurrently but not removed. It's why this assert may not
be out of date any why remove may not fail.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File components/devtools_bridge/android/javatests/AndroidManifest.xml (right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/AndroidManifest.xml:2: <!--
Copyright (c) 2014 The Chromium Authors. All rights reserved. Use of
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> nit: remove "(c)"

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/LocalTunnelBridgeTest.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/LocalTunnelBridgeTest.java:50:
mBridge.dispose();
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> Should also reset the reference just in case.

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java:196:
new int[] { 1, 5, 100, 1000, SocketTunnelBase.READING_BUFFER_SIZE * 2 };
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> If data packets with empty payload are allowed, should also test for 0.

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/tests/DebugActivity.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/tests/DebugActivity.java:34:
"5. Open chroome://inspect#devices on desktop Chrome.\n" +
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> typo: "chrome:"

Done.

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java
(right):

https://codereview.chromium.org/517233002/diff/140001/components/devtools_bri...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/TestUtils.java:20:
private static final String CHARSET = "UTF8";
On 2014/09/15 16:35:22, mnaganov (cr) wrote:
> nit: I think "UTF-8" is more standard spelling. "UTF8" is kind of an alias
> defined in JDK. See
> http://www.iana.org/assignments/character-sets/character-sets.xhtml

Done.

[mnaganov (inactive), 2014-09-16 11:30:18]

LGTM

[SeRya, 2014-09-16 11:50:29]

serya@chromium.org changed reviewers:
+ erikwright@chromium.org, tedchoc@chromium.org

[SeRya, 2014-09-16 11:50:30]

tedchoc@chromium.org: Please review changes in 

chrome/android/... Review of other android related stuffs (what's everithing in
this CL) are also very welcome.

erikwright@chromium.org: Please review changes in 

components/*

[Ted C, 2014-09-18 01:39:53]

lgtm for chrome/android

No idea if you care about the style nits, but just some android pointers.

https://codereview.chromium.org/517233002/diff/160001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/517233002/diff/160001/build/all.gyp#newcode820
build/all.gyp:820:
'../components/devtools_bridge.gyp:devtools_bridge_tests_apk',
looks like the reset are alpha order if that matters

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:41:
public abstract void registerObserver(Observer observer);
as a general comment, we try to javadoc all non-private functions, and it looks
like in general most are not.

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:126:
throw new RuntimeException("Must be called on signaling thread");
in java land, braces are always required unless the statement and conditional
can all fit on a single line

applies in a few places

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:155:

remove extra blank line here

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:218:
implements AbstractDataChannel.Observer {
instead of aligning with extends, it should be 8 from the start of the previous
line ("private final"...)

[SeRya, 2014-09-18 07:04:42]

Tad, thank you for reviewing java code. I do care about style nits. It's my
first non-trivial Java code review so sometimes confuse with various style
guides.

https://codereview.chromium.org/517233002/diff/160001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/517233002/diff/160001/build/all.gyp#newcode820
build/all.gyp:820:
'../components/devtools_bridge.gyp:devtools_bridge_tests_apk',
On 2014/09/18 01:39:52, Ted C wrote:
> looks like the reset are alpha order if that matters

Done.

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java
(right):

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/AbstractDataChannel.java:41:
public abstract void registerObserver(Observer observer);
On 2014/09/18 01:39:52, Ted C wrote:
> as a general comment, we try to javadoc all non-private functions, and it
looks
> like in general most are not.

Added javadoc to the methods with non-trivial semantics.

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:126:
throw new RuntimeException("Must be called on signaling thread");
On 2014/09/18 01:39:53, Ted C wrote:
> in java land, braces are always required unless the statement and conditional
> can all fit on a single line
> 
> applies in a few places

Fixed where found.

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:155:

On 2014/09/18 01:39:52, Ted C wrote:
> remove extra blank line here

Done.

https://codereview.chromium.org/517233002/diff/160001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:218:
implements AbstractDataChannel.Observer {
On 2014/09/18 01:39:52, Ted C wrote:
> instead of aligning with extends, it should be 8 from the start of the
previous
> line ("private final"...)

Done.

[Ted C, 2014-09-19 22:59:53]

lgtm

https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:68:
// data channel automatically. TODO: WebRTC limit supposed to be removed.
TODO's should have a name in them:

TODO(username): WebRTC limit...

[erikwright (departed), 2014-09-23 13:56:55]

On 2014/09/19 22:59:53, Ted C wrote:
> lgtm
> 
>
https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
> File
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
> (right):
> 
>
https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:68:
> // data channel automatically. TODO: WebRTC limit supposed to be removed.
> TODO's should have a name in them:
> 
> TODO(username): WebRTC limit...

is mnaganov approving the overall Java code in components/devtools_bridge?

[mnaganov (inactive), 2014-09-23 14:41:01]

On 2014/09/23 13:56:55, erikwright wrote:
> On 2014/09/19 22:59:53, Ted C wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
> > File
> >
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
> >
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:68:
> > // data channel automatically. TODO: WebRTC limit supposed to be removed.
> > TODO's should have a name in them:
> > 
> > TODO(username): WebRTC limit...
> 
> is mnaganov approving the overall Java code in components/devtools_bridge?

Yes, I'm approving it. I've just thought, that perhaps we need to add an entry
into OWNERS for components/devtools_bridge.gyp, and also put OWNERS and maybe
README file into components/devtools_bridge. Sergey, can you please do that?

[SeRya, 2014-09-23 18:15:08]

Patchset #9 (id:200001) has been deleted

[SeRya, 2014-09-23 18:20:59]

OWNERS and README added

https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java
(right):

https://codereview.chromium.org/517233002/diff/180001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SocketTunnelBase.java:68:
// data channel automatically. TODO: WebRTC limit supposed to be removed.
On 2014/09/19 22:59:52, Ted C wrote:
> TODO's should have a name in them:
> 
> TODO(username): WebRTC limit...

Done.

[erikwright (departed), 2014-09-23 19:31:03]

LGTM with two nits.

https://codereview.chromium.org/517233002/diff/240001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/517233002/diff/240001/components/OWNERS#newco...
components/OWNERS:34: per-file devtools_bridge.gyp=serya@chromium.org
alphabetize this section

https://codereview.chromium.org/517233002/diff/240001/components/devtools_bri...
File components/devtools_bridge/OWNERS (right):

https://codereview.chromium.org/517233002/diff/240001/components/devtools_bri...
components/devtools_bridge/OWNERS:1: serya@chromium.org
alphabetize this list

[SeRya, 2014-09-24 05:41:09]

Patchset #12 (id:280001) has been deleted

[SeRya, 2014-09-24 05:42:56]

Patchset #11 (id:260001) has been deleted

[SeRya, 2014-09-24 06:59:02]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-09-24 06:59:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/517233002/300001

[SeRya, 2014-09-24 07:04:21]

https://codereview.chromium.org/517233002/diff/240001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/517233002/diff/240001/components/OWNERS#newco...
components/OWNERS:34: per-file devtools_bridge.gyp=serya@chromium.org
On 2014/09/23 19:31:03, erikwright wrote:
> alphabetize this section

Done.

https://codereview.chromium.org/517233002/diff/240001/components/devtools_bri...
File components/devtools_bridge/OWNERS (right):

https://codereview.chromium.org/517233002/diff/240001/components/devtools_bri...
components/devtools_bridge/OWNERS:1: serya@chromium.org
On 2014/09/23 19:31:03, erikwright wrote:
> alphabetize this list

Done.

[commit-bot: I haz the power, 2014-09-24 07:13:52]

Committed patchset #11 (id:300001) as 9a1c1763431d8aa91f560f047077e244ddca99d5

[commit-bot: I haz the power, 2014-09-24 07:14:30]

Patchset 11 (id:??) landed as
https://crrev.com/15044609893faf9b2c2a533aa9a136f288ea20a1
Cr-Commit-Position: refs/heads/master@{#296360}

[phoglund_chromium, 2014-09-24 07:31:33]

A revert of this CL (patchset #11 id:300001) has been created in
https://codereview.chromium.org/597063002/ by phoglund@chromium.org.

The reason for reverting is: Fails to compile on Android:
https://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builde...

devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java:291:
cannot find symbol
symbol: method sleep()
                sleep();
                ^.

[mnaganov (inactive), 2014-09-24 07:34:41]

On 2014/09/24 07:31:33, phoglund wrote:
> A revert of this CL (patchset #11 id:300001) has been created in
> https://codereview.chromium.org/597063002/ by mailto:phoglund@chromium.org.
> 
> The reason for reverting is: Fails to compile on Android:
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builde...
> 
>
devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java:291:
> cannot find symbol
> symbol: method sleep()
>                 sleep();
>                 ^.

Yeah. That was actually caught by android_clang_dbg trybot:
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...

I'm wondering how did commit-bot land it.

[SeRya, 2014-09-24 08:22:51]

On 2014/09/24 07:34:41, mnaganov (cr) wrote:
> On 2014/09/24 07:31:33, phoglund wrote:
> > A revert of this CL (patchset #11 id:300001) has been created in
> > https://codereview.chromium.org/597063002/ by mailto:phoglund@chromium.org.
> > 
> > The reason for reverting is: Fails to compile on Android:
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builde...
> > 
> >
>
devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/SocketTunnelServerTest.java:291:
> > cannot find symbol
> > symbol: method sleep()
> >                 sleep();
> >                 ^.
> 
> Yeah. That was actually caught by android_clang_dbg trybot:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...
> 
> I'm wondering how did commit-bot land it.

It's strange. The code looks correct. The trybot failed on the first attempt but
succeeded on the second one. And now failed again.

[SeRya, 2014-09-24 09:48:09]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-09-24 09:48:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/517233002/320001

[commit-bot: I haz the power, 2014-09-24 09:54:49]

Committed patchset #12 (id:320001) as fb4d6e742f921daa8b1258bb420e2e420866913d

[commit-bot: I haz the power, 2014-09-24 09:55:27]

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