Data channel implementation for SessionDependencyFactory.

Data channel implementation for SessionDependencyFactory.

ConvertUTF8ToJavaString(...).Release()) relaced by ....obj() to avoid
local references overflow on the signaling thread stack. Once it is not
java called code local reference table is not cleared automatically.
Since there are 1-2 local reference in a method it's easier to
rely on the ~ScopedJavaLocalRef that create a ScopedJavaLocalFrame.

BUG=383418
Committed: https://crrev.com/f94b2243265fd0385f5048790a84644842b2d2c6
Cr-Commit-Position: refs/heads/master@{#304631}

[SeRya, 2014-11-17 22:55:53]

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

[SeRya, 2014-11-17 22:55:54]

PTAL

[mnaganov (inactive), 2014-11-18 03:39:00]

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:44:
ConvertUTF8ToJavaString(env, sdp_mid).obj(),
Yeah, this looks unusual compared to the rest of the Chromium code, but I think,
I'm understanding the reason -- as you are doing calls into Java initiated from
the native side, JVM will not be clearing local references automatically, as it
does in the case of a Java -> native call. Thus, not releasing the string
reference and relying on ~ScopedJavaLocalRef to perform local reference cleanup
looks like the right thing to do.

But, I'm concerned that when adding another function, it will be easy to forget
about this. As I don't see a solution that could be enforced on the language
level, maybe at least add a comment on the top of the class which would warn
about the need to delete local references in the callbacks?

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:77:
observer_(observer.Pass()) {
You should also initialize 'open_' here, as otherwise it will be in a random
state until the first assignment.

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:98:
observer_->OnMessage(buffer.data.data(), buffer.size());
nit: indentation

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:136: void
SendMessage(void* data, size_t length, bool binary) {
is_binary

[SeRya, 2014-11-18 06:57:27]

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:44:
ConvertUTF8ToJavaString(env, sdp_mid).obj(),
On 2014/11/18 03:38:59, mnaganov (cr) wrote:
> Yeah, this looks unusual compared to the rest of the Chromium code, but I
think,
> I'm understanding the reason -- as you are doing calls into Java initiated
from
> the native side, JVM will not be clearing local references automatically, as
it
> does in the case of a Java -> native call. Thus, not releasing the string
> reference and relying on ~ScopedJavaLocalRef to perform local reference
cleanup
> looks like the right thing to do.
> 
> But, I'm concerned that when adding another function, it will be easy to
forget
> about this. As I don't see a solution that could be enforced on the language
> level, maybe at least add a comment on the top of the class which would warn
> about the need to delete local references in the callbacks?

Done.

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:77:
observer_(observer.Pass()) {
On 2014/11/18 03:38:59, mnaganov (cr) wrote:
> You should also initialize 'open_' here, as otherwise it will be in a random
> state until the first assignment.

That's OK. Instance is not used until InitState called on signaling thread.

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:98:
observer_->OnMessage(buffer.data.data(), buffer.size());
On 2014/11/18 03:38:59, mnaganov (cr) wrote:
> nit: indentation

Done.

https://codereview.chromium.org/719043002/diff/20001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:136: void
SendMessage(void* data, size_t length, bool binary) {
On 2014/11/18 03:38:59, mnaganov (cr) wrote:
> is_binary

Done.

[mnaganov (inactive), 2014-11-18 16:41:17]

LGTM % comment

https://codereview.chromium.org/719043002/diff/40001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/719043002/diff/40001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:29: *
ConvertUTF8ToJavaString(env, ...).obj()
Looks good, but I'd re-arrange this as:

"For instance, instead of ...Release(), please use ...obj() or
ScopedJavaLocalFrame."

[SeRya, 2014-11-18 16:58:08]

https://codereview.chromium.org/719043002/diff/40001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/719043002/diff/40001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:29: *
ConvertUTF8ToJavaString(env, ...).obj()
On 2014/11/18 16:41:17, mnaganov (cr) wrote:
> Looks good, but I'd re-arrange this as:
> 
> "For instance, instead of ...Release(), please use ...obj() or
> ScopedJavaLocalFrame."

Done.

[SeRya, 2014-11-18 16:58:16]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-11-18 16:59:58]

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

[commit-bot: I haz the power, 2014-11-18 17:39:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-18 17:40:35]

Patchset 4 (id:??) landed as
https://crrev.com/f94b2243265fd0385f5048790a84644842b2d2c6
Cr-Commit-Position: refs/heads/master@{#304631}