Let BaseChromiumApplication manage the tracing controller

Let BaseChromiumApplication manage the tracing controller

- Reduce duplicated code;
- Allow tracing during the whole life cycle of the application (TODO:
  support tracing before the native library is loaded);
- Avoid conflict between multiple tracing controllers.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228839

[Sami, 2013-10-10 16:36:41]

https://codereview.chromium.org/26740005/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/26740005/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:32:
getTracingController().unregisterReceiver(this);
Is this backwards by any chance? :)

[Xianzhu, 2013-10-10 16:42:15]

https://codereview.chromium.org/26740005/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/26740005/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:32:
getTracingController().unregisterReceiver(this);
On 2013/10/10 16:36:41, Sami wrote:
> Is this backwards by any chance? :)

Right. I was just wondering why the change didn't work :)

[Xianzhu, 2013-10-10 18:32:32]

-sky, +skyostil

[Yaron, 2013-10-11 09:08:49]

Sorry for being dim but can you elaborate on how this enables us to avoid a
conflict between tracing controllers?

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:15:
* Basic application functionality that should be shared among all browser
applications.
... based on the content layer

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:17:
public class BaseChromiumApplication extends Application {
I'd say this ought to be ChromiumContentApplication (or just ContentApplication)

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:34:
getTracingController().registerReceiver(this);
This now happens way super early (i.e. the first thing that's ever done). It's
probably only a few ms but does it really need to be this early?

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:247:
// TODO(wangxianzhu): Support startTracing before native library is loaded
joc: how do you plan to accomplish that? Send messages to atrace from java?

[Sami, 2013-10-11 14:25:27]

On 2013/10/11 09:08:49, Yaron wrote:
> Sorry for being dim but can you elaborate on how this enables us to avoid a
> conflict between tracing controllers?

Correct me if I'm wrong, but after this patch there will ever only be one
TracingControllerAndroid instance in the app, so no chance of conflicts.

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:34:
getTracingController().registerReceiver(this);
On 2013/10/11 09:08:49, Yaron wrote:
> This now happens way super early (i.e. the first thing that's ever done). It's
> probably only a few ms but does it really need to be this early?

Right, let's be careful about regressing start-up time. Is there a way to defer
this to when the UI thread is idle again? If not, maybe a fixed delay would work
too, since I think we have other ways to trace the start-up sequence.

[Xianzhu, 2013-10-11 18:10:33]

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:15:
* Basic application functionality that should be shared among all browser
applications.
On 2013/10/11 09:08:49, Yaron wrote:
> ... based on the content layer

Done.

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:17:
public class BaseChromiumApplication extends Application {
On 2013/10/11 09:08:49, Yaron wrote:
> I'd say this ought to be ChromiumContentApplication (or just
ContentApplication)

Done.

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/app/BaseChromiumApplication.java:34:
getTracingController().registerReceiver(this);
Done with IdleHandler.

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/26740005/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:247:
// TODO(wangxianzhu): Support startTracing before native library is loaded
On 2013/10/11 09:08:49, Yaron wrote:
> joc: how do you plan to accomplish that? Send messages to atrace from java?

I saw two ways (more likely we'll take 2):
1. Let TraceEvent buffer the Java events before the native library is loaded.
2. Let Java events go to atrace. In a thread discussing future WebView
trace_event, one idea is that 'systrace webview' only collects the Java trace
events (through atrace); and chrome://inspect or chrome://tracing on Desktop to
remotely collect Java/native combined traces.

However, I removed the check because now ContentApplication only calls
registerReceiver after the library is loaded.

[Yaron, 2013-10-15 17:01:34]

Sami: how did you previously get multiple instances? For the most part we only
have a single instance of the activity. That's only invalidated in rare cases (I
believe locale change is one)

[Sami, 2013-10-15 17:08:39]

On 2013/10/15 17:01:34, Yaron wrote:
> Sami: how did you previously get multiple instances? For the most part we only
> have a single instance of the activity. That's only invalidated in rare cases
(I
> believe locale change is one)

I didn't see them in practice, but I can see how it could happen in theory if we
are rapidly switching between activities. For example if you go to Settings =>
Help you get an EmbedContentViewActivity which has another instance of
TracingControllerAndroid.

There are also some multi-window OEM hacks that could run into this case, and at
some we might hit it with WebView.

[Xianzhu, 2013-10-15 17:12:05]

There will be two instances when there is a Main activity and an
EmbedContentViewActivity. There is also rare chances that both instances has
broadcast receiver registered if the EmbedContentViewActivity doesn't fully
cover the Main activity (though this can be avoided if we register/unregsister
the receiver in Main activity onResume/onPause instead of onStop/onStart).

I think the main purpose of this CL is to avoid the duplicated code in 4 places
(ChromiumTestShellActivity, ContentShellActivity, Main activity and
EmbedContentViewActivity). Last 2 are in internal CL 44937.

[Xianzhu, 2013-10-15 17:18:21]

On 2013/10/15 17:08:39, Sami wrote:
> On 2013/10/15 17:01:34, Yaron wrote:
> > Sami: how did you previously get multiple instances? For the most part we
only
> > have a single instance of the activity. That's only invalidated in rare
cases
> (I
> > believe locale change is one)
> 
> I didn't see them in practice, but I can see how it could happen in theory if
we
> are rapidly switching between activities. For example if you go to Settings =>
> Help you get an EmbedContentViewActivity which has another instance of
> TracingControllerAndroid.
> 
> There are also some multi-window OEM hacks that could run into this case, and
at
> some we might hit it with WebView.

I think there will be two instances (but normally only one has receiver
registered) if there is a Main activity and an EmbedContentViewActivity. There
seems still a chance that both of 2 instances have receiver registered if the
EmbedContentViewActivity doesn't fully cover the Main activity (though this can
be avoided if we use onResume/onPause instead of onStop/onStart in main
activity).

My main purpose of this CL is to reduce the duplicated code in 4 places
(ChromiumTestShellActivity, ContentShellActivity, Main activity,
EmbedContentViewActivity). The last two are in internal CL 44937.

[Yaron, 2013-10-15 17:18:23]

Right. thanks!

I understood about code-reuse but was confused on missing signals lgtm

[commit-bot: I haz the power, 2013-10-15 17:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26740005/29001

[Xianzhu, 2013-10-15 17:29:36]

Sorry for the duplicated comments. When I submitted the first one it seemed lost
so I typed it again, and now both show up :-|

[commit-bot: I haz the power, 2013-10-15 23:35:41]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-10-15 23:36:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/26740005/61001

[commit-bot: I haz the power, 2013-10-16 02:41:21]

Change committed as 228839