Use Chromium's logging utility for content files

Use Chromium's logging utility for content files

This patch modifies all the files under content/public/android/java
to use org.chromium.base.Log instead of android.util.Log.
It also changes the log messges as per the guidelines.

BUG=472152
Committed: https://crrev.com/fbcb7c45bd634d5c9b09acdc1b3efd008d6f9970
Cr-Commit-Position: refs/heads/master@{#335910}

[AKVT, 2015-06-16 08:48:52]

ajith.v@samsung.com changed reviewers:
+ ajith.v@samsung.com

[AKVT, 2015-06-16 08:48:54]

Few nits marked.

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:50:
private static final String TAG = "cr.ChildProcessLaunch";
cr.ChildProcessService

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:224:
Log.i(TAG, "Destroying ChildProcessService pid= %d", Process.myPid());
nit: pid=%d

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:275:
Log.i(TAG, "Initializing chromium process, singleProcess= %b", singleProcess);
nit: singleProcess=%b

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:72:
private static final String TAG = "cr.ChildProcessLaunch";
cr.ChildProcessConnection

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:183:
Log.w(TAG, "onServiceDisconnected (crash or killed by oom): pid= %d", mPid);
pid=%d

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:42:
private static final String TAG = "cr.ChildProcessLaunch";
cr.ChildProcessLauncher

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:573:
Log.d(TAG, "Setting up connection to process: slot= %d",
nit: slot=%d

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:594:
Log.d(TAG, "on connect callback, pid=%d context=%d callbackType=%d",
context=%ld ?

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:716:
Log.w(TAG, "%s, pid=%d", message, pid);
nit: "%s ",

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:281:
Log.e(TAG, "Error calling handler for smart clip data: ", e);
Log.e(TAG, "Error calling handler for smart clip data: %s", e); or
Log.e(TAG, "Error calling handler for smart clip data: %s", e.toString());

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:147:
Log.w(TAG, "Bad URI %s", intentUrl, ex);
Bad URI %s Exception=%s", intentUrl, ex.toString());

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java:27:
private static final String TAG = "cr.PepperPlugin";
cr.PepperPluginManager

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:199:
private static final String TAG = "cr.ScreenOrientation";
I think we should keep public class name ScreenOrientationListener as it is.

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:25:
private static final String TAG = "cr.ScreenOrientation";
cr.ScreenOrientationProvider

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:45:
private static final String TAG = "cr.Tracing";
cr.TracingControllerAndroid

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:53:
private static final String TAG = "cr.crypto";
cr.CipherFactory

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]" ,  text, selectionStart,
nit: no space before ,

[%s] [%b]" , -> [%s] [%b]",

[ams, 2015-06-16 10:43:53]

Addressed the review comments.
Please take a look again.

On 2015/06/16 08:48:54, AKV away wrote:
> Few nits marked.
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:50:
> private static final String TAG = "cr.ChildProcessLaunch";
> cr.ChildProcessService

Changed to cr.ChildProcService. Since there is a 23 car length limit for the
TAG, changed 'process' to 'proc' everywhere.

> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:224:
> Log.i(TAG, "Destroying ChildProcessService pid= %d", Process.myPid());
> nit: pid=%d

Done.

> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:275:
> Log.i(TAG, "Initializing chromium process, singleProcess= %b", singleProcess);
> nit: singleProcess=%b
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:72:
> private static final String TAG = "cr.ChildProcessLaunch";
> cr.ChildProcessConnection
> 

Done. Changed to 'cr.ChildProcConnection'.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:183:
> Log.w(TAG, "onServiceDisconnected (crash or killed by oom): pid= %d", mPid);
> pid=%d
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:42:
> private static final String TAG = "cr.ChildProcessLaunch";
> cr.ChildProcessLauncher
> 

Done. Changed to 'cr.ChildProcLauncher'.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:573:
> Log.d(TAG, "Setting up connection to process: slot= %d",
> nit: slot=%d
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:594:
> Log.d(TAG, "on connect callback, pid=%d context=%d callbackType=%d",
> context=%ld ?
> 

%d applies to Long as well.
Ref: http://docs.oracle.com/javase/6/docs/api/java/util/Formatter.html#syntax


>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:716:
> Log.w(TAG, "%s, pid=%d", message, pid);
> nit: "%s ",
> 

Kept the same formatting as original one. Since message is a sring, to keep it
separate from pid, I felt ',' makes sense here.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
> content/public/android/java/src/org/chromium/content/browser/ContentView.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ContentView.java:281:
> Log.e(TAG, "Error calling handler for smart clip data: ", e);
> Log.e(TAG, "Error calling handler for smart clip data: %s", e); or
> Log.e(TAG, "Error calling handler for smart clip data: %s", e.toString());
> 

It is internally using below format for Log:
android.util.Log.e(String tag, String msg, Throwable tr)

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:147:
> Log.w(TAG, "Bad URI %s", intentUrl, ex);
> Bad URI %s Exception=%s", intentUrl, ex.toString());
> 

Same as above.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java:27:
> private static final String TAG = "cr.PepperPlugin";
> cr.PepperPluginManager
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationListener.java:199:
> private static final String TAG = "cr.ScreenOrientation";
> I think we should keep public class name ScreenOrientationListener as it is.
> 

Kept the same due to 23 character length limitation on TAG.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/ScreenOrientationProvider.java:25:
> private static final String TAG = "cr.ScreenOrientation";
> cr.ScreenOrientationProvider
> 

Same as above.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:45:
> private static final String TAG = "cr.Tracing";
> cr.TracingControllerAndroid
> 

Changed to 'cr.TracingController' due to length limitation.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:53:
> private static final String TAG = "cr.crypto";
> cr.CipherFactory
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
> Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]" ,  text,
selectionStart,
> nit: no space before ,
> 
> [%s] [%b]" , -> [%s] [%b]",

Done.

[AKVT, 2015-06-16 11:36:23]

peer review looks good to me!

https://codereview.chromium.org/1178253005/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1178253005/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]",  text, selectionStart,
very nit: remove second space from [%b]",  text -> [%b]", text

[ams, 2015-06-16 13:25:58]

amit.srkr@samsung.com changed reviewers:
+ benm@chromium.org, dgn@chromium.org, jdduke@chromium.org
- ajith.v@samsung.com

[ams, 2015-06-16 13:26:41]

On 2015/06/16 11:36:23, AKV away wrote:
> peer review looks good to me!
> 
>
https://codereview.chromium.org/1178253005/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
> Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]",  text, selectionStart,
> very nit: remove second space from [%b]",  text -> [%b]", text

Done.

[ams, 2015-06-16 13:30:58]

PTAL

[dgn, 2015-06-16 14:59:36]

Naming nit: one of the goals of with the logging changes was to make it easier
to enable/disable logs, by making some logical groups. I don't know the code
there, but looking at the filenames, maybe something like cr.content.media,
cr.content.contentView, etc. Please keep in mind that Log.v and Log.d will
prepend the filename/line number to the log message. So having the full
classname as tag might not always be useful.
But again, it's up to you since I don't know this code. The current names work,
that remark is mostly for awareness.

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:212:
Log.w(TAG, "%s startup failed: %s", MAIN_THREAD_NAME, e.toString());
Just passing e is better, here and in many places below. Passing an object for
"%s" calls String.valueOf, which will handle null pointers and call toString()
if non null. And if the Log call is removed, the toString() call is not made at
all.

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
(right):

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:166:
Log.d(TAG, "%s", (hasVideo ? "resource has video" : "resource doesn't have
video"));
I think the original code should work here. Log.d(String, String) is still
available.

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:33:
private static final boolean DEBUG = false;
not: Not necessary anymore. Replacing the `if (DEBUG) Log.w(..)` by `Log.d(...)`
should have the same effect, except that you will be able to unable logs at
runtime in debug builds. I left it as nit since the current form will at least
work in release builds, and also properly compiles out the logs when
DEBUG=false. Both work fine.

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
(right):

https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:36:
private static final boolean DEBUG = false;  // Always check in as false!
nit:Same as above

[dgn, 2015-06-17 09:21:13]

non owner lgtm % stray toString() calls.

https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java
(right):

https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java:102:
Log.e(TAG, "Can't get service information from %s", info.toString());
toString()

https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
(right):

https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:299:
Log.e(TAG, "Unexpected intent: %s", intent.toString());
toString()

[ams, 2015-06-17 09:45:30]

@dgn,

Thanks for the review. 
I have adressed few nits and for few, I would need Content owners' opinion.

On 2015/06/16 14:59:36, dgn wrote:
> Naming nit: one of the goals of with the logging changes was to make it easier
> to enable/disable logs, by making some logical groups. I don't know the code
> there, but looking at the filenames, maybe something like cr.content.media,
> cr.content.contentView, etc. Please keep in mind that Log.v and Log.d will
> prepend the filename/line number to the log message. So having the full
> classname as tag might not always be useful.
> But again, it's up to you since I don't know this code. The current names
work,
> that remark is mostly for awareness.
> 

Even I had the same opinion. 

But one concern was that only Log.v & Log.d prints the file name, not other Log
patterns. Many of these codes are using Log.i and Log.w frequently.

Also there are diverse functions being done in some of these files. Once we add
"cr.content", we have limited space available (23 characters in total) for
further submodules. Hence difficult to create sub-module like
"cr.content.screenorientation".

May be for some of the files, we can use "cr.content.contentView" like TAGs,
whereas others skip the "content" part.
I would like to have some opinion from Content owners for these before making
any change.

>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:212:
> Log.w(TAG, "%s startup failed: %s", MAIN_THREAD_NAME, e.toString());
> Just passing e is better, here and in many places below. Passing an object for
> "%s" calls String.valueOf, which will handle null pointers and call toString()
> if non null. And if the Log call is removed, the toString() call is not made
at
> all.
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:166:
> Log.d(TAG, "%s", (hasVideo ? "resource has video" : "resource doesn't have
> video"));
> I think the original code should work here. Log.d(String, String) is still
> available.
> 

Kept original version.

>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:33:
> private static final boolean DEBUG = false;
> not: Not necessary anymore. Replacing the `if (DEBUG) Log.w(..)` by
`Log.d(...)`
> should have the same effect, except that you will be able to unable logs at
> runtime in debug builds. I left it as nit since the current form will at least
> work in release builds, and also properly compiles out the logs when
> DEBUG=false. Both work fine.
> 

The flexibility to use the logs in release builds and compiling out the logs is
really useful.
I think, I should again wait for Content file owner's opinion on this.

>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:36:
> private static final boolean DEBUG = false;  // Always check in as false!
> nit:Same as above

Same as above.

[ams, 2015-06-17 09:46:15]

On 2015/06/17 09:21:13, dgn wrote:
> non owner lgtm % stray toString() calls.
> 
>
https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/PepperPluginManager.java:102:
> Log.e(TAG, "Can't get service information from %s", info.toString());
> toString()
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/60001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/TracingControllerAndroid.java:299:
> Log.e(TAG, "Unexpected intent: %s", intent.toString());
> toString()

Done.

[ams, 2015-06-17 09:48:15]

@jdduke,

PTAL.

Also please suggest if I should add any other owner.

[jdduke (slow), 2015-06-17 19:37:14]

jdduke@chromium.org changed reviewers:
+ tedchoc@chromium.org
- benm@chromium.org

[jdduke (slow), 2015-06-17 19:37:15]

In general this looks fine but I'm technically just an owner for input-related
changes. +tedchoc for content/.

As an aside, how do the variadic "Object..." arguments work in Java? If an int
is one of the arguments, will that cause it to be boxed into an Object? If so,
that's kind of unfortunate with the lazy formatting here =/.

[ams, 2015-06-18 07:25:08]

On 2015/06/17 19:37:15, jdduke wrote:
> In general this looks fine but I'm technically just an owner for input-related
> changes. +tedchoc for content/.
> 
> As an aside, how do the variadic "Object..." arguments work in Java? If an int
> is one of the arguments, will that cause it to be boxed into an Object? If so,
> that's kind of unfortunate with the lazy formatting here =/.

In this case, it seems autoboxing takes place.

As javadoc says: "There is a strong synergy between autoboxing and varargs"
ref: http://docs.oracle.com/javase/7/docs/technotes/guides/language/varargs.html

[ams, 2015-06-18 07:25:39]

@TedC,

PTAL.

[Ted C, 2015-06-19 20:21:01]

On 2015/06/18 07:25:39, ams wrote:
> @TedC,
> 
> PTAL.

lgtm

[dgn, 2015-06-19 21:44:50]

https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:298:
Log.e(TAG, "Not a valid surfaceObject: %s" + surfaceObject);
s/+/,

https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:168:
if (DEBUG) {
if debug log warning? shouldn't that just be `Log.d()?`

https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]", text, selectionStart,
I think this should be "updateState [%s] [%s %s] [%s %s] [%b]"

[ams, 2015-06-22 10:49:51]

PTAL

On 2015/06/19 21:44:50, dgn wrote:
>
https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:298:
> Log.e(TAG, "Not a valid surfaceObject: %s" + surfaceObject);
> s/+/,
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:168:
> if (DEBUG) {
> if debug log warning? shouldn't that just be `Log.d()?`

Done.

> 
>
https://codereview.chromium.org/1178253005/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:169:
> Log.w(TAG, "updateState [%s] [%s] [%s] [%s] [%s] [%b]", text, selectionStart,
> I think this should be "updateState [%s] [%s %s] [%s %s] [%b]"

Done.

[dgn, 2015-06-22 11:17:07]

non owner lgtm.

Thanks!

[ams, 2015-06-22 12:58:34]

@TedC, 

PTAL again.

[Ted C, 2015-06-22 16:02:45]

just to offer the opposing opinion, but to Jared's point let's keep things
restricted where object allocations can happen frequently unless there is a
really strong need to remove it

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:241:
+ "%d", pid);
put a space before the % to keep it consistent with how it was

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:167:
Log.d(TAG, "updateState [%s] [%s %s] [%s %s] [%b]", text, selectionStart,
the DEBUG guards were preventing autoboxing object allocation, so I'm not sure
this is the best thing to remove

In this case, they are all strings so it is fine, but any of the number things
below are going to do more than before.

Unless we have a really strong need to remove the guards, let's keep them in
place especially since many of these code paths will be hit much more frequently
than other places in this patch.

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
(right):

https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:100:
Log.d(TAG, "will try and cleanup; max = %d", sRefs.size());
same comment as the other file

[dgn, 2015-06-22 16:17:10]

On 2015/06/22 at 16:02:45, tedchoc wrote:
> just to offer the opposing opinion, but to Jared's point let's keep things
restricted where object allocations can happen frequently unless there is a
really strong need to remove it

Ok, understood.

[ams, 2015-06-23 11:07:21]

@TedC, PTAL.

I have reverted the DEBUG -> Log.d conversion for both the files as suggested.
Also addressed the nits.

On 2015/06/22 16:02:45, Ted C wrote:
> just to offer the opposing opinion, but to Jared's point let's keep things
> restricted where object allocations can happen frequently unless there is a
> really strong need to remove it
> 
>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:241:
> + "%d", pid);
> put a space before the % to keep it consistent with how it was
> 

Done.

>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:167:
> Log.d(TAG, "updateState [%s] [%s %s] [%s %s] [%b]", text, selectionStart,
> the DEBUG guards were preventing autoboxing object allocation, so I'm not sure
> this is the best thing to remove
> 
> In this case, they are all strings so it is fine, but any of the number things
> below are going to do more than before.
> 
> Unless we have a really strong need to remove the guards, let's keep them in
> place especially since many of these code paths will be hit much more
frequently
> than other places in this patch.
> 

Reverted last changes to keep DEBUG usage same as earlier.

>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
> (right):
> 
>
https://codereview.chromium.org/1178253005/diff/120001/content/public/android...
>
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:100:
> Log.d(TAG, "will try and cleanup; max = %d", sRefs.size());
> same comment as the other file

Ditto.

[Ted C, 2015-06-23 17:26:31]

lgtm

[ams, 2015-06-23 17:39:56]

The CQ bit was checked by amit.srkr@samsung.com

[ams, 2015-06-23 17:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/1178253005/#ps160001
(title: "few more nits")

[commit-bot: I haz the power, 2015-06-23 17:41:22]

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

[commit-bot: I haz the power, 2015-06-23 17:45:13]

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

[commit-bot: I haz the power, 2015-06-23 17:45:15]

Exceeded global retry quota

[ams, 2015-06-24 02:23:08]

The CQ bit was checked by amit.srkr@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 02:23:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178253005/160001

[commit-bot: I haz the power, 2015-06-24 02:26:47]

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

[commit-bot: I haz the power, 2015-06-24 02:26:49]

Dry run: Exceeded global retry quota

[ams, 2015-06-24 11:37:51]

The CQ bit was checked by amit.srkr@samsung.com

[ams, 2015-06-24 11:37:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/1178253005/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-24 11:38:11]

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

[commit-bot: I haz the power, 2015-06-24 12:38:09]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-24 12:38:52]

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

[jdduke (slow), 2015-06-24 12:40:37]

On 2015/06/24 12:38:52, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/fbcb7c45bd634d5c9b09acdc1b3efd008d6f9970
> Cr-Commit-Position: refs/heads/master@{#335910}

Thanks, and please remember the guidelines here if you end up making similar
changes elsewhere.

[ams, 2015-06-24 12:42:26]

On 2015/06/24 12:40:37, jdduke wrote:
> On 2015/06/24 12:38:52, commit-bot: I haz the power wrote:
> > Patchset 10 (id:??) landed as
> > https://crrev.com/fbcb7c45bd634d5c9b09acdc1b3efd008d6f9970
> > Cr-Commit-Position: refs/heads/master@{#335910}
> 
> Thanks, and please remember the guidelines here if you end up making similar
> changes elsewhere.

Sure, thanks!