Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146363004:
Turn on strict mode for all userdebug builds. (Closed)
DescriptionTurn on strict mode for all userdebug builds.
BUG=149471
Committed: https://crrev.com/65da93d932a7e5208ac5345ac82240aa9699de7e
Cr-Commit-Position: refs/heads/master@{#333517}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 28 (7 generated)
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/1146363004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1146363004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:258: || "userdebug".equals(Build.TYPE) Ned, Annie: Can we somehow bypass this for telemetry perf tests? Perhaps when the "--enable-gpu-benchmarking" flag is present? Or should we add a more official flag for telemetry runs?
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/1146363004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1146363004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:258: || "userdebug".equals(Build.TYPE) On 2015/06/01 21:34:58, jdduke wrote: > Ned, Annie: Can we somehow bypass this for telemetry perf tests? Perhaps when > the "--enable-gpu-benchmarking" flag is present? Or should we add a more > official flag for telemetry runs? I would push back adding further flags to telemetry. What does this strict mode do and why do we want to bypass this for telemetry per tests?
On 2015/06/01 21:37:55, nednguyen wrote: Could you also do a smoothness.key_mobile_sites_smooth perf comparison as a sanity check? With something like "--pageset-repeat=5"? Thanks.
On 2015/06/02 16:11:10, jdduke wrote: > On 2015/06/01 21:37:55, nednguyen wrote: > > Could you also do a smoothness.key_mobile_sites_smooth perf comparison as a > sanity check? With something like "--pageset-repeat=5"? Thanks. Took a looooong time, but here are the results: http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-06... I don't know why turning on StrictMode improves the stats. :-/ Peter
On 2015/06/02 20:07:34, Peter Wen wrote: > On 2015/06/02 16:11:10, jdduke wrote: > > On 2015/06/01 21:37:55, nednguyen wrote: > > > > Could you also do a smoothness.key_mobile_sites_smooth perf comparison as a > > sanity check? With something like "--pageset-repeat=5"? Thanks. > > Took a looooong time, but here are the results: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-06... > > I don't know why turning on StrictMode improves the stats. :-/ > > Peter Which device was this?
On 2015/06/02 20:13:06, jdduke wrote: > On 2015/06/02 20:07:34, Peter Wen wrote: > > On 2015/06/02 16:11:10, jdduke wrote: > > > On 2015/06/01 21:37:55, nednguyen wrote: > > > > > > Could you also do a smoothness.key_mobile_sites_smooth perf comparison as a > > > sanity check? With something like "--pageset-repeat=5"? Thanks. > > > > Took a looooong time, but here are the results: > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-06... > > > > I don't know why turning on StrictMode improves the stats. :-/ > > > > Peter > > Which device was this? N5 on 5.1
wnwen@chromium.org changed reviewers: + dfalcantara@chromium.org, dtrainor@chromium.org
Hi David and Dan, PTAL, this CL enables strict mode for all userdebug builds. If this impacts perf bots I will roll it back. I believe I've whitelisted the existing places where we violate StrictMode. Hopefully this will motivate everyone to keep new code as StrictMode clean as possible. Thanks, Peter
wnwen@chromium.org changed reviewers: + dfalcantara@chromium.org, dtrainor@chromium.org
Hi David and Dan, PTAL, this CL enables strict mode for all userdebug builds. If this impacts perf bots I will roll it back. I believe I've whitelisted the existing places where we violate StrictMode. Hopefully this will motivate everyone to keep new code as StrictMode clean as possible. Thanks, Peter
On 2015/06/08 19:38:33, Peter Wen wrote: > I believe I've whitelisted the existing places where we violate StrictMode. > Hopefully this will motivate everyone to keep new code as StrictMode clean as > possible. Have you also looked at ChromeShell to see if there are any common violations there? That's the primary driver (currently) for our perf tests.
On 2015/06/08 20:03:33, jdduke wrote: > Have you also looked at ChromeShell to see if there are any common violations > there? That's the primary driver (currently) for our perf tests. Just played around on ChromeShell, no red flashes for now. :)
Would defer to jdduke on this one.
jdduke@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
I'll defer to yusufo@ and tedchoc@ here, my only objection was from a performance perspective =/.
On 2015/06/08 21:24:33, jdduke wrote: > I'll defer to yusufo@ and tedchoc@ here, my only objection was from a > performance perspective =/. lgtm -- let's see how it goes
lgtm
The CQ bit was checked by wnwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146363004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/65da93d932a7e5208ac5345ac82240aa9699de7e Cr-Commit-Position: refs/heads/master@{#333517}
Message was sent while issue was closed.
jonnialva90@gmail.com changed reviewers: + jonnialva90@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
