Gate console.log output in Google Now

Gate console.log output in Google Now

To reduce the chattiness of Google Now, localStorage.debug_mode for Google Now
and showConsoleLogOutput for unit tests will control when console.log actually
outputs to the console.

BUG=338239
TEST=By default, there will be no console.log output. When
localStorage.debug_mode is true in Google Now or showConsoleLogOutput is set
to true in unit tests, console.log output will occur.

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

[Vadim Sh., 2014-01-28 20:47:52]

On 2014/01/28 20:40:18, robliao wrote:

I am not the reviewer you're looking for. Removing myself from the list...

[robliao, 2014-01-28 20:49:24]

Vadim autocomplete fix.

[vadimt, 2014-01-28 21:07:12]

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:10: var
showConsoleLogOutput = false;
showConsoleLogOutput seems to be a constant set to false.
Probably, if we want unit tests to always show console output, it should be
true? However, if it's a constant that no one changes, do we need it at all?

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:39: return
!!localStorage.debug_mode && (localStorage.debug_mode === 'true');
Why do we need the first check (!!localStorage.debug_mode)?
Also: every use of localStorage causes a blocking roundtrip to the browser. It's
better to use it only once, not from every log call.

[skare_, 2014-01-28 21:25:51]

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:36: * @return {boolean} Debug
mode is enabled.
tiny nit: Debug mode is enabled -> Whether debug mode... or True if debug
mode...

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:58: var originalConsoleLog =
console.log;
any chance this and the other replacement will be run in the same file? should
behave ok, just checking

[robliao, 2014-01-28 21:56:02]

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:10: var
showConsoleLogOutput = false;
One of the main complaints is that the unit tests are too chatty. The request
here is to turn them off.
On 2014/01/28 21:07:12, vadimt wrote:
> showConsoleLogOutput seems to be a constant set to false.
> Probably, if we want unit tests to always show console output, it should be
> true? However, if it's a constant that no one changes, do we need it at all?

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:36: * @return {boolean} Debug
mode is enabled.
On 2014/01/28 21:25:52, Travis Skare wrote:
> tiny nit: Debug mode is enabled -> Whether debug mode... or True if debug
> mode...

Done.

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:39: return
!!localStorage.debug_mode && (localStorage.debug_mode === 'true');
I guess we don't need to check it based off of this structure.

The goal of this function is to have a one-stop-shop for getting the debug mode.
It turns out, localStorage is cached in the renderer, so lookups happen in the
same process and appear to be quick!

Check out
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/d...

and
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/d...
On 2014/01/28 21:07:12, vadimt wrote:
> Why do we need the first check (!!localStorage.debug_mode)?
> Also: every use of localStorage causes a blocking roundtrip to the browser.
It's
> better to use it only once, not from every log call.

https://codereview.chromium.org/145353010/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/utility.js:58: var originalConsoleLog =
console.log;
There aren't any includes in JS world, just Chrome loading all of the JS files
together. As long as utilty.js is specified once, we should be okay here.
On 2014/01/28 21:25:52, Travis Skare wrote:
> any chance this and the other replacement will be run in the same file? should
> behave ok, just checking

[vadimt, 2014-01-28 22:19:07]

lgtm

[rgustafson, 2014-01-28 22:37:12]

lgtm

[commit-bot: I haz the power, 2014-01-28 23:50:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/145353010/30001

[commit-bot: I haz the power, 2014-01-29 00:50:48]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-29 00:54:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/145353010/30001

[commit-bot: I haz the power, 2014-01-29 02:21:54]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-29 02:31:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/145353010/30001

[commit-bot: I haz the power, 2014-01-29 02:52:15]

Change committed as 247606