Introduce moderate binding

Introduce moderate binding

Moderate binding is used to keep background renderers which ran in the
foreground recently as important as a visible process.
To do that, a binding with Context.BIND_AUTO_CREATE is made when a strong
binding is released.

When a device is suffered on low memory, the moderate bindings will be
trimed with a specific ratio according to the severity in LRU order.

When a device is suffered critically or Chrome itself goes into the
background, all the moderate bindings will be released.

For now, this feature is experimental, and so it will be controlled as field trial.

And UMA histograms - "Android.ModerateBindingCount", "Tab.RendererCrashStatus"
and "Tab.TotalTabCount.BeforeLeavingApp" - are added as metrics.

BUG=485867
Committed: https://crrev.com/ad111397b2762a791cd19e1a7340074ff7e595c0
Cr-Commit-Position: refs/heads/master@{#335598}

[Jaekyun Seok (inactive), 2015-06-09 21:12:35]

jaekyun@chromium.org changed reviewers:
+ mariakhomenko@chromium.org, ppi@chromium.org, yfriedman@chromium.org

[Jaekyun Seok (inactive), 2015-06-09 21:12:35]

mariakhomenko@, yfriedman@ and ppi@, please review this CL.

[Yaron, 2015-06-11 16:54:35]

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:568: if
(!(activityState == ActivityState.PAUSED
Can you pull this condition out to helper fn. (also used above)

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:571:
UmaSessionStats.logBackgroundRendererCrashOnForegroundApp();
Please just use RecordUserAction directly.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:308:
RecordHistogram.recordCountHistogram(
Why not handle both codepaths in application?

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:37:
private static final float DEFAULUT_MODERATE_BINDING_REDUCE_RATIO = 0.25f;
DEFAULT_

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:119:
String trialName = FieldTrialList.findFullName(MODERATE_BINDING_TRIAL_NAME);
extract all this to static helper function

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BindingManager.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManager.java:85:
* Starts moderate binding support.
I think this warrants a little more detail. Possibly refer to the design doc?

Also the name seems a little off to me as it's not clear that you're enabling
the whole subsystem. {start,enable}ModerateBindingManagement ?

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

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:35:
private class ModerateBindingPool extends LruCache<Integer, ManagedConnection> {
Can this be static?

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:158:
if (mModerateBindingPool != null && !connection.isStrongBindingBound()
Shouldn't this be !connection.isModerateBindingBound?

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

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:66:
private ChildServiceConnection mModerateBinding = null;
Add comment

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:23:
public class BindingManagerImplTest extends InstrumentationTestCase {
Please add tests for your new code.

[Jaekyun Seok (inactive), 2015-06-12 06:50:20]

PTAL.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:568: if
(!(activityState == ActivityState.PAUSED
On 2015/06/11 16:54:34, Yaron wrote:
> Can you pull this condition out to helper fn. (also used above)

I realized that I should have checked the application state instead of activity
state. I fixed it.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:571:
UmaSessionStats.logBackgroundRendererCrashOnForegroundApp();
On 2015/06/11 16:54:34, Yaron wrote:
> Please just use RecordUserAction directly.

Done.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:308:
RecordHistogram.recordCountHistogram(
On 2015/06/11 16:54:34, Yaron wrote:
> Why not handle both codepaths in application?

Because accessing ChromeTabbedActivity in ChromeMobileApplication seems a little
dirty logic.

Anyway, I will move this into ChromeMobileApplication.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:37:
private static final float DEFAULUT_MODERATE_BINDING_REDUCE_RATIO = 0.25f;
On 2015/06/11 16:54:34, Yaron wrote:
> DEFAULT_

Done.

https://codereview.chromium.org/1146813010/diff/100001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:119:
String trialName = FieldTrialList.findFullName(MODERATE_BINDING_TRIAL_NAME);
On 2015/06/11 16:54:34, Yaron wrote:
> extract all this to static helper function

Done.

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BindingManager.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManager.java:85:
* Starts moderate binding support.
On 2015/06/11 16:54:34, Yaron wrote:
> I think this warrants a little more detail. Possibly refer to the design doc?
> 
> Also the name seems a little off to me as it's not clear that you're enabling
> the whole subsystem. {start,enable}ModerateBindingManagement ?

Done.

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

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:35:
private class ModerateBindingPool extends LruCache<Integer, ManagedConnection> {
On 2015/06/11 16:54:34, Yaron wrote:
> Can this be static?

Done.

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:158:
if (mModerateBindingPool != null && !connection.isStrongBindingBound()
On 2015/06/11 16:54:34, Yaron wrote:
> Shouldn't this be !connection.isModerateBindingBound?

This is to confirm whether a strong binding is actually unbound or not because
ChildProcessConnectionImpl has call count for addStrongBinding, and so just
calling removeStrongBinding doesn't guarantee that a strong binding gets
unbound.

And both a strong binding and a moderate binding can't be being bound at the
same time because the latter is removed when the former is added.

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

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:66:
private ChildServiceConnection mModerateBinding = null;
On 2015/06/11 16:54:35, Yaron wrote:
> Add comment

Done.

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/1146813010/diff/100001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:23:
public class BindingManagerImplTest extends InstrumentationTestCase {
On 2015/06/11 16:54:35, Yaron wrote:
> Please add tests for your new code.

Done.

[Jaekyun Seok (inactive), 2015-06-15 06:05:49]

Ping?

[Maria, 2015-06-16 01:33:02]

https://codereview.chromium.org/1146813010/diff/120001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/120001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:152:
if (lowReduceRatio > highReduceRatio) lowReduceRatio = highReduceRatio;
I think it may be safer to do 
Math.max(0, Math.min(1, highReduceRatio)) and 
Math.max(0, Math.min(highReduceRatio, lowReduceRatio))

to ensure all your values are between 0 and 1.

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:370:
if (!mOnTesting) {
is this needed because we aren't loading native in the unit test?

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:431:
if (mModerateBindingPool != null) mModerateBindingPool.onLowMemory();
I am not a big fan of having testing logic in production code. Would it be
possible to use a mock context instead and send low memory signals directly from
the test?

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:133:
* Attaches a moderate binding that will make the service as a visible process.
nit: reword this. How about "Attaches a moderate binding that will give the
service the priority of a visible process, but keep the priority below a
strongly bound process."

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:67:
// while the app is in the foreground. It will be being bound only while the app
is in the
nit: "be being bound" -> "stay bound"

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:76:
private static final String TAG = "cr.ChildProcessConnect";
why change this tag?

https://codereview.chromium.org/1146813010/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:16997: +<histogram
name="MobileBindingManager.ModerateBindingCount">
We prefer not to start new top level namespaces for histograms if possible, I
would consider adding this to an existing one.

[Jaekyun Seok (inactive), 2015-06-16 02:40:32]

PTAL.

https://codereview.chromium.org/1146813010/diff/120001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/120001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:152:
if (lowReduceRatio > highReduceRatio) lowReduceRatio = highReduceRatio;
On 2015/06/16 01:33:02, Maria wrote:
> I think it may be safer to do 
> Math.max(0, Math.min(1, highReduceRatio)) and 
> Math.max(0, Math.min(highReduceRatio, lowReduceRatio))
> 
> to ensure all your values are between 0 and 1.

Done.

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:370:
if (!mOnTesting) {
On 2015/06/16 01:33:02, Maria wrote:
> is this needed because we aren't loading native in the unit test?

Yes, tests crash without this.

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:431:
if (mModerateBindingPool != null) mModerateBindingPool.onLowMemory();
On 2015/06/16 01:33:02, Maria wrote:
> I am not a big fan of having testing logic in production code. Would it be
> possible to use a mock context instead and send low memory signals directly
from
> the test?

Done.

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:133:
* Attaches a moderate binding that will make the service as a visible process.
On 2015/06/16 01:33:02, Maria wrote:
> nit: reword this. How about "Attaches a moderate binding that will give the
> service the priority of a visible process, but keep the priority below a
> strongly bound process."

Done.

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

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:67:
// while the app is in the foreground. It will be being bound only while the app
is in the
On 2015/06/16 01:33:02, Maria wrote:
> nit: "be being bound" -> "stay bound"

Done.

https://codereview.chromium.org/1146813010/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:76:
private static final String TAG = "cr.ChildProcessConnect";
On 2015/06/16 01:33:02, Maria wrote:
> why change this tag?

presubmit required this like the followings.

Please define your tags using the suggested format: .
"private static final String TAG = "cr.<package tag>".
See base/android/java/src/org/chromium/base/README_logging.md or contact
dgn@chromium.org for more info.

https://codereview.chromium.org/1146813010/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:16997: +<histogram
name="MobileBindingManager.ModerateBindingCount">
On 2015/06/16 01:33:02, Maria wrote:
> We prefer not to start new top level namespaces for histograms if possible, I
> would consider adding this to an existing one.

I changed it to "Android.ModerateBindingCount".

[gone, 2015-06-16 22:23:43]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org, dtrainor@chromium.org

[gone, 2015-06-16 22:23:44]

dtrainor@ for ChromeTabbedActivity/TabModelSelector/Multi-window madness.

[gone, 2015-06-16 22:25:54]

On 2015/06/16 22:23:44, dfalcantara wrote:
> dtrainor@ for ChromeTabbedActivity/TabModelSelector/Multi-window madness.

He said something about being able to use the TabWindowManager, but he'll
comment when he gets back.

[David Trainor- moved to gerrit, 2015-06-16 22:48:23]

If you're having trouble with this working in MultiWindow check TabWindowManager
(you might have to add a new method to just query the selector for a given
Activity).  Although it looks like ChromeActivity#getTabModelSelector() might
just return the right one in this case anyway.  I'd just check to make sure :).

[Jaekyun Seok (inactive), 2015-06-17 00:36:05]

PTAL.

On 2015/06/16 22:48:23, David Trainor wrote:
> If you're having trouble with this working in MultiWindow check
TabWindowManager
> (you might have to add a new method to just query the selector for a given
> Activity).  Although it looks like ChromeActivity#getTabModelSelector() might
> just return the right one in this case anyway.  I'd just check to make sure
:).

Thanks, David. I should have considered multiple instances of
ChromeTabbedActivity.

But I need to record the total tab count of all ChromeTabbedActivity in that
case.

So I changed codes to get the sum of the total tab count of each ChromeActivity
in the tabbed mode.

[Yaron, 2015-06-17 02:56:12]

lgtm

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

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:281:
private boolean mOnTesting;
final

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:554:
int i;
inline in for-loop's initializer

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:573:
assertEquals(i >= pair.second, connections[i].isModerateBindingBound());
You should add a meaningful error message (i.e. which memory signal fired)
otherwise there's no way to know which case failed

[Jaekyun Seok (inactive), 2015-06-17 04:35:46]

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

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:281:
private boolean mOnTesting;
On 2015/06/17 02:56:12, Yaron wrote:
> final

Done.

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:554:
int i;
On 2015/06/17 02:56:12, Yaron wrote:
> inline in for-loop's initializer

Done.

https://codereview.chromium.org/1146813010/diff/180001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:573:
assertEquals(i >= pair.second, connections[i].isModerateBindingBound());
On 2015/06/17 02:56:12, Yaron wrote:
> You should add a meaningful error message (i.e. which memory signal fired)
> otherwise there's no way to know which case failed

Done.

[Jaekyun Seok (inactive), 2015-06-17 04:37:00]

jaekyun@chromium.org changed reviewers:
+ asvitkine@chromium.org, mpearson@chromium.org

[Jaekyun Seok (inactive), 2015-06-17 04:37:01]

asvitkine@ and mpearson@, please review changes in
tools/metrics/actions/actions.xml and tools/metrics/histograms/histograms.xml.

[Alexei Svitkine (slow), 2015-06-17 16:57:23]

Besides my comments below, please also change the CL description to not use
"Finch", which is an internal codename.

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:128:
|| MODERATE_BINDING_DISABLED_VALUE.equalsIgnoreCase(trialName)) {
Can you make this a param as well?

(Or check StartsWith, but since you're using params already, that's a better
solution.)

This lets you define a Control group that has the same behavior as Disabled,
which is our best practice when you experiment at < 50%.

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:144:
if (!TextUtils.isEmpty(highReduceRatioValue)) {
Nit: Maybe make a helper function to avoid the extra boilerplate?

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:7515: <action
name="MobileBackgroundRendererCrashedOnForegroundApp">
What's the reason for making this an action rather than a boolean histogram?

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:271: +<histogram
name="Android.ModerateBindingCount">
Nit: add units=

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:275: +    the foreground.
Mention when this is logged.

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44057: +<histogram
name="Tab.TotalTabCountBeforeLeavingApp">
Nit: Add units=

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44061: +    foreground.
Mention when this is logged.

[Mark P, 2015-06-17 18:05:23]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[Mark P, 2015-06-17 18:05:42]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-06-17 18:05:43]

Since Alexei is reviewing this, I removed myself from the reviewer list.

--mark

[Jaekyun Seok (inactive), 2015-06-17 22:31:51]

PTAL.

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:128:
|| MODERATE_BINDING_DISABLED_VALUE.equalsIgnoreCase(trialName)) {
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Can you make this a param as well?
> 
> (Or check StartsWith, but since you're using params already, that's a better
> solution.)
> 
> This lets you define a Control group that has the same behavior as Disabled,
> which is our best practice when you experiment at < 50%.

Done.

https://codereview.chromium.org/1146813010/diff/200001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:144:
if (!TextUtils.isEmpty(highReduceRatioValue)) {
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Nit: Maybe make a helper function to avoid the extra boilerplate?

Done.

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:7515: <action
name="MobileBackgroundRendererCrashedOnForegroundApp">
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> What's the reason for making this an action rather than a boolean histogram?

We are already using an action named "MobileRendererCrashed" to log renderer
crash in the foreground;
https://cs.corp.google.com/#clankium/src/chrome/browser/android/metrics/uma_s....

So I introduced another action to log renderer crash in the background. Do you
think that using a histogram is properer?

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:271: +<histogram
name="Android.ModerateBindingCount">
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Nit: add units=

Done.

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:275: +    the foreground.
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Mention when this is logged.

Done.

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44057: +<histogram
name="Tab.TotalTabCountBeforeLeavingApp">
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Nit: Add units=

Done.

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44061: +    foreground.
On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> Mention when this is logged.

Done.

[Jaekyun Seok (inactive), 2015-06-17 22:32:40]

On 2015/06/17 16:57:23, Alexei Svitkine wrote:
> Besides my comments below, please also change the CL description to not use
> "Finch", which is an internal codename.

I've updated the CL description as well.

[Mark P, 2015-06-17 22:46:48]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[Maria, 2015-06-18 01:19:10]

lgtm

[ppi, 2015-06-18 14:28:28]

ppi@chromium.org changed reviewers:
- ppi@chromium.org

[ppi, 2015-06-18 14:28:29]

I'll defer to Maria and Yaron, the idea sounds great:).

r:-ppi
cc:ppi

[Alexei Svitkine (slow), 2015-06-18 15:34:33]

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:7515: <action
name="MobileBackgroundRendererCrashedOnForegroundApp">
On 2015/06/17 22:31:50, Jaekyun Seok wrote:
> On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> > What's the reason for making this an action rather than a boolean histogram?
> 
> We are already using an action named "MobileRendererCrashed" to log renderer
> crash in the foreground;
>
https://cs.corp.google.com/#clankium/src/chrome/browser/android/metrics/uma_s....
> 
> So I introduced another action to log renderer crash in the background. Do you
> think that using a histogram is properer?

Yeah, in my opinion it would be better to combine the two actions into a single
enumerated histogram with two buckets.

Histograms are better supported by our dashboards (e.g. allow filters) and are
also more efficient.

[Jaekyun Seok (inactive), 2015-06-18 22:41:18]

PTAL.

I modified codes to use a histogram named "Tab.RendererCrashStatus" instead of
an action.
But I didn't removed the action named "MobileRendererCrashed" because it might
be being used somewhere; I left TODO for the future refactoring.

On 2015/06/18 15:34:33, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
> File tools/metrics/actions/actions.xml (right):
> 
>
https://codereview.chromium.org/1146813010/diff/200001/tools/metrics/actions/...
> tools/metrics/actions/actions.xml:7515: <action
> name="MobileBackgroundRendererCrashedOnForegroundApp">
> On 2015/06/17 22:31:50, Jaekyun Seok wrote:
> > On 2015/06/17 16:57:22, Alexei Svitkine wrote:
> > > What's the reason for making this an action rather than a boolean
histogram?
> > 
> > We are already using an action named "MobileRendererCrashed" to log renderer
> > crash in the foreground;
> >
>
https://cs.corp.google.com/#clankium/src/chrome/browser/android/metrics/uma_s....
> > 
> > So I introduced another action to log renderer crash in the background. Do
you
> > think that using a histogram is properer?
> 
> Yeah, in my opinion it would be better to combine the two actions into a
single
> enumerated histogram with two buckets.
> 
> Histograms are better supported by our dashboards (e.g. allow filters) and are
> also more efficient.

[Alexei Svitkine (slow), 2015-06-19 17:17:57]

Looks good overall, a few more suggestions.

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java
(right):

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java:413:
if (activity == null) continue;
Nit: Not necessarily, since the check below will be false if activity is null:

http://stackoverflow.com/questions/2950319/is-null-check-needed-before-callin...

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java:418:
tabModelSelector == null ? 0 : tabModelSelector.getTotalTabCount();
Nit: I think it's cleaner for this to be a regular if.

if (tabModelSelector != null)
  totalTabCount += tabModelSelector.getTotalTabCount();

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:138:
if (Boolean.parseBoolean(disabledValue)) return;
I suggest making the param be "enabled" instead of "disabled".

Then, you don't need the findFullName() check above, because if there's no
trial, then "enabled" won't have a value and thus will be false and this check
will cause the early return in that case.

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44066: +    recorded when a renderer
crashes.
Mention that this is platform specific (and which platforms.)

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44196: +<histogram
name="Tab.TotalTabCountBeforeLeavingApp" units="tabs">
Nit: Suggest renaming to Tab.TotalTabCount.BeforeLeavingApp.

That way, if you add another time when you want to record number of tabs, you
can use a different suffix and the two can be grouped together by our
dashboards.

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44201: +    background.
Ditto about mentioning platform.

[Jaekyun Seok (inactive), 2015-06-22 01:48:57]

PTAL.

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java
(right):

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java:413:
if (activity == null) continue;
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> Nit: Not necessarily, since the check below will be false if activity is null:
> 
>
http://stackoverflow.com/questions/2950319/is-null-check-needed-before-callin...

Done.

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java:418:
tabModelSelector == null ? 0 : tabModelSelector.getTotalTabCount();
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> Nit: I think it's cleaner for this to be a regular if.
> 
> if (tabModelSelector != null)
>   totalTabCount += tabModelSelector.getTotalTabCount();

Done.

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/1146813010/diff/240001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/DeferredStartupHandler.java:138:
if (Boolean.parseBoolean(disabledValue)) return;
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> I suggest making the param be "enabled" instead of "disabled".
> 
> Then, you don't need the findFullName() check above, because if there's no
> trial, then "enabled" won't have a value and thus will be false and this check
> will cause the early return in that case.

Done.

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44066: +    recorded when a renderer
crashes.
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> Mention that this is platform specific (and which platforms.)

Done.

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44196: +<histogram
name="Tab.TotalTabCountBeforeLeavingApp" units="tabs">
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> Nit: Suggest renaming to Tab.TotalTabCount.BeforeLeavingApp.
> 
> That way, if you add another time when you want to record number of tabs, you
> can use a different suffix and the two can be grouped together by our
> dashboards.

Done.

https://codereview.chromium.org/1146813010/diff/240001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44201: +    background.
On 2015/06/19 17:17:56, Alexei Svitkine wrote:
> Ditto about mentioning platform.

Done.

[Jaekyun Seok (inactive), 2015-06-22 03:17:52]

PTAL.

Two build errors occurred after rebasing this CL to the latest origin/master.

So I fixed them.

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

https://codereview.chromium.org/1146813010/diff/300001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:82:
} else {
I realized that LruCache.trimToSize is available only since Android API 17. So I
added my own logic to trim LruCache for lower APIs.

https://codereview.chromium.org/1146813010/diff/300001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java
(right):

https://codereview.chromium.org/1146813010/diff/300001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:558:
String message = "Failed for the level=" + pair.first;
String.format required Locale, and so I modified codes to add contents without
using it.

[Alexei Svitkine (slow), 2015-06-22 14:58:52]

lgtm

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

https://codereview.chromium.org/1146813010/diff/300001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:84:
for (Map.Entry<Integer, ManagedConnection> entry : snapshot().entrySet()) {
Nit: Add a comment explaining why this makes sense (i.e. that snapshot() returns
things ordered from least recently to most recently accessed).

[Jaekyun Seok (inactive), 2015-06-22 21:24:48]

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

https://codereview.chromium.org/1146813010/diff/300001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:84:
for (Map.Entry<Integer, ManagedConnection> entry : snapshot().entrySet()) {
On 2015/06/22 14:58:52, Alexei (OOO June 24 - July 6) wrote:
> Nit: Add a comment explaining why this makes sense (i.e. that snapshot()
returns
> things ordered from least recently to most recently accessed).

Done.

[Jaekyun Seok (inactive), 2015-06-23 00:45:02]

The CQ bit was checked by jaekyun@chromium.org

[Jaekyun Seok (inactive), 2015-06-23 00:45:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, mariakhomenko@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1146813010/#ps320001
(title: "Add comments")

[commit-bot: I haz the power, 2015-06-23 00:45:57]

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

[commit-bot: I haz the power, 2015-06-23 00:52:34]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-06-23 00:53:54]

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

[zbowling_chromium, 2015-06-23 04:40:56]

On 2015/06/23 00:53:54, commit-bot: I haz the power wrote:
> Patchset 17 (id:??) landed as
> https://crrev.com/ad111397b2762a791cd19e1a7340074ff7e595c0
> Cr-Commit-Position: refs/heads/master@{#335598}

After this landed, a test related to this started failing on my CL try bots:
https://codereview.chromium.org/1203573002/. Not sure why it's not failing here.

[Jaekyun Seok (inactive), 2015-06-23 04:47:00]

On 2015/06/23 04:40:56, zbowling_chromium wrote:
> On 2015/06/23 00:53:54, commit-bot: I haz the power wrote:
> > Patchset 17 (id:??) landed as
> > https://crrev.com/ad111397b2762a791cd19e1a7340074ff7e595c0
> > Cr-Commit-Position: refs/heads/master@{#335598}
> 
> After this landed, a test related to this started failing on my CL try bots:
> https://codereview.chromium.org/1203573002/. Not sure why it's not failing
here.

The failed tests (TabsTest#testLastClosedUndoableTabGetsHidden) isn't related to
this CL.
Actually this CL couldn't have any impact because the feature is disabled by
default.