Measure time spent in SharedPrefs-loading StrictMode violations.

Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262, 562189
Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}

Committed: https://crrev.com/09bb92eb700b228eb17a176504b4486769aafe55
Cr-Commit-Position: refs/heads/master@{#371623}

[hartmanng, 2016-01-21 20:22:10]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189
==========

[hartmanng, 2016-01-21 20:22:10]

hartmanng@chromium.org changed reviewers:
+ wnwen@chromium.org

[hartmanng, 2016-01-21 20:23:14]

hartmanng@chromium.org changed reviewers:
- wnwen@chromium.org

[hartmanng, 2016-01-21 21:58:50]

hartmanng@chromium.org changed reviewers:
+ yfriedman@chromium.org

[hartmanng, 2016-01-21 21:58:51]

Yaron, please take a look.

Note that this depends on Peter's CL
(https://codereview.chromium.org/1583233004/) and will have to be rebased after
that lands.

[Yaron, 2016-01-21 22:10:25]

I think my comment actually applies to both of these

https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:224:
SystemClock.elapsedRealtime() - time, TimeUnit.MILLISECONDS);
If you follow the chain of calls from this, I don't think native is guaranteed
to be loaded so this could crash

[hartmanng, 2016-01-22 19:14:00]

PTAL

https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:224:
SystemClock.elapsedRealtime() - time, TimeUnit.MILLISECONDS);
On 2016/01/21 22:10:25, Yaron wrote:
> If you follow the chain of calls from this, I don't think native is guaranteed
> to be loaded so this could crash

Should be fixed, see my comments in patch set 6.

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
(right):

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java:198:
try {
This try/catch shouldn't be necessary as far as I know, but if somehow native
isn't loaded by now for a potential reason I don't know about, it's probably
better not to crash.

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:223:
try {
ActivityAssigner doesn't appear to have a convenient spot where native is
guaranteed to be loaded, like DocumentTabModelImpl does. Since I expect the disk
read time to be similar here as in DocumentTabModelImpl, I propose that we just
wrap it in a try/catch, and hope to get some data every once in a while. If not,
we'll still have the DocumentMode prefs data.

Alternatively, since ActivityAssigner is a singleton, it should be relatively
convenient to call into it from some other class when we're sure native has
loaded - so we could proceed that way to try to increase the likelihood of
getting real data back.

[hartmanng, 2016-01-22 19:28:42]

On 2016/01/22 19:14:00, hartmanng wrote:
> PTAL
> 
>
https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
> (right):
> 
>
https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:224:
> SystemClock.elapsedRealtime() - time, TimeUnit.MILLISECONDS);
> On 2016/01/21 22:10:25, Yaron wrote:
> > If you follow the chain of calls from this, I don't think native is
guaranteed
> > to be loaded so this could crash
> 
> Should be fixed, see my comments in patch set 6.
> 
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
> (right):
> 
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java:198:
> try {
> This try/catch shouldn't be necessary as far as I know, but if somehow native
> isn't loaded by now for a potential reason I don't know about, it's probably
> better not to crash.
> 
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
> (right):
> 
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:223:
> try {
> ActivityAssigner doesn't appear to have a convenient spot where native is
> guaranteed to be loaded, like DocumentTabModelImpl does. Since I expect the
disk
> read time to be similar here as in DocumentTabModelImpl, I propose that we
just
> wrap it in a try/catch, and hope to get some data every once in a while. If
not,
> we'll still have the DocumentMode prefs data.
> 
> Alternatively, since ActivityAssigner is a singleton, it should be relatively
> convenient to call into it from some other class when we're sure native has
> loaded - so we could proceed that way to try to increase the likelihood of
> getting real data back.


Actually you may want to hold off on taking another look for a few minutes -
ActivityAssigner will need to be rebased very shortly on top of
https://codereview.chromium.org/1606373002/

[hartmanng, 2016-01-25 16:43:42]

On 2016/01/22 19:28:42, hartmanng wrote:
> On 2016/01/22 19:14:00, hartmanng wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/80001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:224:
> > SystemClock.elapsedRealtime() - time, TimeUnit.MILLISECONDS);
> > On 2016/01/21 22:10:25, Yaron wrote:
> > > If you follow the chain of calls from this, I don't think native is
> guaranteed
> > > to be loaded so this could crash
> > 
> > Should be fixed, see my comments in patch set 6.
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java:198:
> > try {
> > This try/catch shouldn't be necessary as far as I know, but if somehow
native
> > isn't loaded by now for a potential reason I don't know about, it's probably
> > better not to crash.
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:223:
> > try {
> > ActivityAssigner doesn't appear to have a convenient spot where native is
> > guaranteed to be loaded, like DocumentTabModelImpl does. Since I expect the
> disk
> > read time to be similar here as in DocumentTabModelImpl, I propose that we
> just
> > wrap it in a try/catch, and hope to get some data every once in a while. If
> not,
> > we'll still have the DocumentMode prefs data.
> > 
> > Alternatively, since ActivityAssigner is a singleton, it should be
relatively
> > convenient to call into it from some other class when we're sure native has
> > loaded - so we could proceed that way to try to increase the likelihood of
> > getting real data back.
> 
> 
> Actually you may want to hold off on taking another look for a few minutes -
> ActivityAssigner will need to be rebased very shortly on top of
> https://codereview.chromium.org/1606373002/

Sorry for the delay, the tree hadn't rolled by EOD Friday - I wanted to wait and
make sure the trybots are all green.

PTAL

[Yaron, 2016-01-25 20:41:27]

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
(right):

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java:198:
try {
On 2016/01/22 19:14:00, hartmanng wrote:
> This try/catch shouldn't be necessary as far as I know, but if somehow native
> isn't loaded by now for a potential reason I don't know about, it's probably
> better not to crash.

I would just guard this on isNativeInitialized and remove try/catch. If this
object has a native counterpart, you're fine to report and unsatisfiedlinkerror
means something is really wrong.

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:226:
}
I would add a comment like: // Intentionally ignored - it's ok to miss recording
the metric occasionally.

That said, have you verified from chrome://histograms locally that this is
sometimes loaded and it's just a race - not that it'll never happen?

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:227:
} catch (ClassCastException exception) {
why is this here? oh! why duplicate this? just add measurement where it's used
below. Perhaps you could extract a helper fn to keep the flow clearer

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:278:
prefs.getInt(PREF_NUM_SAVED_ENTRIES, 0);
is there actually a string mode issue here? I don't see it

[hartmanng, 2016-01-25 22:19:27]

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
(right):

https://codereview.chromium.org/1618973002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java:198:
try {
On 2016/01/25 20:41:27, Yaron wrote:
> On 2016/01/22 19:14:00, hartmanng wrote:
> > This try/catch shouldn't be necessary as far as I know, but if somehow
native
> > isn't loaded by now for a potential reason I don't know about, it's probably
> > better not to crash.
> 
> I would just guard this on isNativeInitialized and remove try/catch. If this
> object has a native counterpart, you're fine to report and
unsatisfiedlinkerror
> means something is really wrong.

Done.

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:226:
}
On 2016/01/25 20:41:27, Yaron wrote:
> I would add a comment like: // Intentionally ignored - it's ok to miss
recording
> the metric occasionally.
> 
> That said, have you verified from chrome://histograms locally that this is
> sometimes loaded and it's just a race - not that it'll never happen?

I actually haven't been able to get this to record anything locally... My
justification for keeping it in was that we were actually never able to
reproduce this strict mode violation locally at all - but it happened
consistently on bots. I'm not 100% sure we'll ever actually get data back from
here, but I guess I was just assuming it was a weird timing issue like the
strict mode violation was.

I _had_ verified that we are able to get to this code without problems while
running some of the instrumentation tests, though now that I think about it that
might not necessarily mean anything. Also now that you pointed it out... Since I
included Peter's disableForTests() method, it _definitely_ didn't mean anything
and appears to be failing in the tests too, when I don't disable it.

If you know of a place in this class where it's more likely for native to be
initialized (I don't see anything obvious), I can try to move the histogram code
there. Otherwise we can go with my comment from the previous patch set:

> Alternatively, since ActivityAssigner is a singleton, it should be relatively
> convenient to call into it from some other class when we're sure native has
> loaded - so we could proceed that way to try to increase the likelihood of
> getting real data back.

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:227:
} catch (ClassCastException exception) {
On 2016/01/25 20:41:27, Yaron wrote:
> why is this here? oh! why duplicate this? just add measurement where it's used
> below.

done.


> Perhaps you could extract a helper fn to keep the flow clearer

It's actually not _all_ that bad... it's at least easier to read now than when I
had the whole separate try block, imo. I can refactor into a helper function if
you want though.

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:278:
prefs.getInt(PREF_NUM_SAVED_ENTRIES, 0);
On 2016/01/25 20:41:27, Yaron wrote:
> is there actually a string mode issue here? I don't see it

hmm, no, I don't think there is. I think I mistook storeActivityList for
restoreActivityList at some point.

[Yaron, 2016-01-26 19:18:46]

lgtm

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:226:
}
On 2016/01/25 22:19:27, hartmanng wrote:
> On 2016/01/25 20:41:27, Yaron wrote:
> > I would add a comment like: // Intentionally ignored - it's ok to miss
> recording
> > the metric occasionally.
> > 
> > That said, have you verified from chrome://histograms locally that this is
> > sometimes loaded and it's just a race - not that it'll never happen?
> 
> I actually haven't been able to get this to record anything locally... My
> justification for keeping it in was that we were actually never able to
> reproduce this strict mode violation locally at all - but it happened
> consistently on bots. I'm not 100% sure we'll ever actually get data back from
> here, but I guess I was just assuming it was a weird timing issue like the
> strict mode violation was.
> 
> I _had_ verified that we are able to get to this code without problems while
> running some of the instrumentation tests, though now that I think about it
that
> might not necessarily mean anything. Also now that you pointed it out... Since
I
> included Peter's disableForTests() method, it _definitely_ didn't mean
anything
> and appears to be failing in the tests too, when I don't disable it.
> 
> If you know of a place in this class where it's more likely for native to be
> initialized (I don't see anything obvious), I can try to move the histogram
code
> there. Otherwise we can go with my comment from the previous patch set:
> 
> > Alternatively, since ActivityAssigner is a singleton, it should be
relatively
> > convenient to call into it from some other class when we're sure native has
> > loaded - so we could proceed that way to try to increase the likelihood of
> > getting real data back.

RecordHistogram definitely would fail in the tests without Peter's disabling
switch b/c the test is a subclass of InstrumentationTestCase. That means there's
no activity/chrome startup - not even native loaded, it's mostly just a unit
test. Looking at crbug/562189 though, it appears that this was triggering. Ok,
let's give this a spin for a few dev cycles but can hopefully remove it soon

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:227:
} catch (ClassCastException exception) {
On 2016/01/25 22:19:27, hartmanng wrote:
> On 2016/01/25 20:41:27, Yaron wrote:
> > why is this here? oh! why duplicate this? just add measurement where it's
used
> > below.
> 
> done.
> 
> 
> > Perhaps you could extract a helper fn to keep the flow clearer
> 
> It's actually not _all_ that bad... it's at least easier to read now than when
I
> had the whole separate try block, imo. I can refactor into a helper function
if
> you want though.

you're right, this is totally fine

[hartmanng, 2016-01-26 19:22:34]

On 2016/01/26 19:18:46, Yaron wrote:
> lgtm
> 

Thanks!

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java
(right):

https://codereview.chromium.org/1618973002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java:226:
}
On 2016/01/26 19:18:46, Yaron wrote:
> On 2016/01/25 22:19:27, hartmanng wrote:
> > On 2016/01/25 20:41:27, Yaron wrote:
> > > I would add a comment like: // Intentionally ignored - it's ok to miss
> > recording
> > > the metric occasionally.
> > > 
> > > That said, have you verified from chrome://histograms locally that this is
> > > sometimes loaded and it's just a race - not that it'll never happen?
> > 
> > I actually haven't been able to get this to record anything locally... My
> > justification for keeping it in was that we were actually never able to
> > reproduce this strict mode violation locally at all - but it happened
> > consistently on bots. I'm not 100% sure we'll ever actually get data back
from
> > here, but I guess I was just assuming it was a weird timing issue like the
> > strict mode violation was.
> > 
> > I _had_ verified that we are able to get to this code without problems while
> > running some of the instrumentation tests, though now that I think about it
> that
> > might not necessarily mean anything. Also now that you pointed it out...
Since
> I
> > included Peter's disableForTests() method, it _definitely_ didn't mean
> anything
> > and appears to be failing in the tests too, when I don't disable it.
> > 
> > If you know of a place in this class where it's more likely for native to be
> > initialized (I don't see anything obvious), I can try to move the histogram
> code
> > there. Otherwise we can go with my comment from the previous patch set:
> > 
> > > Alternatively, since ActivityAssigner is a singleton, it should be
> relatively
> > > convenient to call into it from some other class when we're sure native
has
> > > loaded - so we could proceed that way to try to increase the likelihood of
> > > getting real data back.
> 
> RecordHistogram definitely would fail in the tests without Peter's disabling
> switch b/c the test is a subclass of InstrumentationTestCase. That means
there's
> no activity/chrome startup - not even native loaded, it's mostly just a unit
> test. Looking at crbug/562189 though, it appears that this was triggering. Ok,
> let's give this a spin for a few dev cycles but can hopefully remove it soon

sgtm, I don't think there's any reason we'd need to keep this around
indefinitely.

[hartmanng, 2016-01-26 19:22:45]

hartmanng@chromium.org changed reviewers:
+ asvitkine@chromium.org

[hartmanng, 2016-01-26 19:22:46]

+asvitkine@ for histograms.

[Alexei Svitkine (slow), 2016-01-26 19:49:56]

lgtm

[hartmanng, 2016-01-26 19:56:06]

On 2016/01/26 19:49:56, Alexei Svitkine (very slow) wrote:
> lgtm

Thanks!

[hartmanng, 2016-01-26 19:56:12]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2016-01-26 19:57:15]

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

[commit-bot: I haz the power, 2016-01-26 20:05:38]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189
==========

[commit-bot: I haz the power, 2016-01-26 20:05:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-26 20:07:13]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

[commit-bot: I haz the power, 2016-01-26 20:07:14]

Patchset 9 (id:??) landed as
https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}

[jbudorick, 2016-01-26 20:29:40]

On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> Patchset 9 (id:??) landed as
> https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> Cr-Commit-Position: refs/heads/master@{#371573}

This broke compilation:
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...

manually reverted in
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...

The trybots passed because your try jobs ran before Peter's CL landed.

[jbudorick, 2016-01-26 20:30:22]

On 2016/01/26 20:29:40, jbudorick wrote:
> On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > Patchset 9 (id:??) landed as
> > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > Cr-Commit-Position: refs/heads/master@{#371573}
> 
> This broke compilation:
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> 
> manually reverted in
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> 
> The trybots passed because your try jobs ran before Peter's CL landed.

(er, forgot to link Peter's CL: https://codereview.chromium.org/1635153002)

[Peter Wen, 2016-01-26 20:33:01]

On 2016/01/26 20:30:22, jbudorick wrote:
> On 2016/01/26 20:29:40, jbudorick wrote:
> > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > Patchset 9 (id:??) landed as
> > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > Cr-Commit-Position: refs/heads/master@{#371573}
> > 
> > This broke compilation:
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > 
> > manually reverted in
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > 
> > The trybots passed because your try jobs ran before Peter's CL landed.
> 
> (er, forgot to link Peter's CL: https://codereview.chromium.org/1635153002)

Opps! Sorry about that, completely forgot your CL also changes
DocumentTabModelImpl.

[Yaron, 2016-01-26 20:34:27]

On 2016/01/26 20:33:01, Peter Wen wrote:
> On 2016/01/26 20:30:22, jbudorick wrote:
> > On 2016/01/26 20:29:40, jbudorick wrote:
> > > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > > Patchset 9 (id:??) landed as
> > > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > > Cr-Commit-Position: refs/heads/master@{#371573}
> > > 
> > > This broke compilation:
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > > 
> > > manually reverted in
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > > 
> > > The trybots passed because your try jobs ran before Peter's CL landed.
> > 
> > (er, forgot to link Peter's CL: https://codereview.chromium.org/1635153002)
> 
> Opps! Sorry about that, completely forgot your CL also changes
> DocumentTabModelImpl.

how did it get through CQ?

[jbudorick, 2016-01-26 20:37:15]

On 2016/01/26 20:34:27, Yaron wrote:
> On 2016/01/26 20:33:01, Peter Wen wrote:
> > On 2016/01/26 20:30:22, jbudorick wrote:
> > > On 2016/01/26 20:29:40, jbudorick wrote:
> > > > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > > > Patchset 9 (id:??) landed as
> > > > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > > > Cr-Commit-Position: refs/heads/master@{#371573}
> > > > 
> > > > This broke compilation:
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > > > 
> > > > manually reverted in
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > > > 
> > > > The trybots passed because your try jobs ran before Peter's CL landed.

^ this is how it got through the CQ

> > > 
> > > (er, forgot to link Peter's CL:
https://codereview.chromium.org/1635153002)
> > 
> > Opps! Sorry about that, completely forgot your CL also changes
> > DocumentTabModelImpl.
> 
> how did it get through CQ?

[jbudorick, 2016-01-26 20:38:21]

On 2016/01/26 20:37:15, jbudorick wrote:
> On 2016/01/26 20:34:27, Yaron wrote:
> > On 2016/01/26 20:33:01, Peter Wen wrote:
> > > On 2016/01/26 20:30:22, jbudorick wrote:
> > > > On 2016/01/26 20:29:40, jbudorick wrote:
> > > > > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > > > > Patchset 9 (id:??) landed as
> > > > > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > > > > Cr-Commit-Position: refs/heads/master@{#371573}
> > > > > 
> > > > > This broke compilation:
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > > > > 
> > > > > manually reverted in
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > > > > 
> > > > > The trybots passed because your try jobs ran before Peter's CL landed.
> 
> ^ this is how it got through the CQ

(the CQ will reuse try job results within 24 hours iirc)

> 
> > > > 
> > > > (er, forgot to link Peter's CL:
> https://codereview.chromium.org/1635153002)
> > > 
> > > Opps! Sorry about that, completely forgot your CL also changes
> > > DocumentTabModelImpl.
> > 
> > how did it get through CQ?

[hartmanng, 2016-01-26 20:40:13]

On 2016/01/26 20:38:21, jbudorick wrote:
> On 2016/01/26 20:37:15, jbudorick wrote:
> > On 2016/01/26 20:34:27, Yaron wrote:
> > > On 2016/01/26 20:33:01, Peter Wen wrote:
> > > > On 2016/01/26 20:30:22, jbudorick wrote:
> > > > > On 2016/01/26 20:29:40, jbudorick wrote:
> > > > > > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > > > > > Patchset 9 (id:??) landed as
> > > > > > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > > > > > Cr-Commit-Position: refs/heads/master@{#371573}
> > > > > > 
> > > > > > This broke compilation:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > > > > > 
> > > > > > manually reverted in
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > > > > > 
> > > > > > The trybots passed because your try jobs ran before Peter's CL
landed.
> > 
> > ^ this is how it got through the CQ
> 
> (the CQ will reuse try job results within 24 hours iirc)
> 
> > 
> > > > > 
> > > > > (er, forgot to link Peter's CL:
> > https://codereview.chromium.org/1635153002)
> > > > 
> > > > Opps! Sorry about that, completely forgot your CL also changes
> > > > DocumentTabModelImpl.
> > > 
> > > how did it get through CQ?

Sorry about the breakage, thanks for catching it, John!

[hartmanng, 2016-01-26 20:54:25]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

[hartmanng, 2016-01-26 21:08:53]

The CQ bit was checked by hartmanng@chromium.org

[hartmanng, 2016-01-26 21:08:53]

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

[commit-bot: I haz the power, 2016-01-26 21:09:19]

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

[Yaron, 2016-01-26 21:45:34]

On 2016/01/26 20:38:21, jbudorick wrote:
> On 2016/01/26 20:37:15, jbudorick wrote:
> > On 2016/01/26 20:34:27, Yaron wrote:
> > > On 2016/01/26 20:33:01, Peter Wen wrote:
> > > > On 2016/01/26 20:30:22, jbudorick wrote:
> > > > > On 2016/01/26 20:29:40, jbudorick wrote:
> > > > > > On 2016/01/26 20:07:14, commit-bot: I haz the power wrote:
> > > > > > > Patchset 9 (id:??) landed as
> > > > > > > https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
> > > > > > > Cr-Commit-Position: refs/heads/master@{#371573}
> > > > > > 
> > > > > > This broke compilation:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d...
> > > > > > 
> > > > > > manually reverted in
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/42f3eb1ab815e535223cabf3623b...
> > > > > > 
> > > > > > The trybots passed because your try jobs ran before Peter's CL
landed.
> > 
> > ^ this is how it got through the CQ
> 
> (the CQ will reuse try job results within 24 hours iirc)

oh right, that makes sense. thanks!
> 
> > 
> > > > > 
> > > > > (er, forgot to link Peter's CL:
> > https://codereview.chromium.org/1635153002)
> > > > 
> > > > Opps! Sorry about that, completely forgot your CL also changes
> > > > DocumentTabModelImpl.
> > > 
> > > how did it get through CQ?

[hartmanng, 2016-01-26 22:02:48]

The CQ bit was unchecked by hartmanng@chromium.org

[hartmanng, 2016-01-26 22:28:59]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2016-01-26 22:33:45]

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

[commit-bot: I haz the power, 2016-01-26 22:43:02]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

[commit-bot: I haz the power, 2016-01-26 22:43:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-26 22:45:28]

Description was changed from

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}
==========

to

==========
Measure time spent in SharedPrefs-loading StrictMode violations.

BUG=580262,562189

Committed: https://crrev.com/5db5d9e5815c64ba4bc93fda27115c542df35316
Cr-Commit-Position: refs/heads/master@{#371573}

Committed: https://crrev.com/09bb92eb700b228eb17a176504b4486769aafe55
Cr-Commit-Position: refs/heads/master@{#371623}
==========

[commit-bot: I haz the power, 2016-01-26 22:45:29]

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