Added histograms to https://codereview.chromium.org/207553008/.

Added histograms to https://codereview.chromium.org/207553008/.

Also includes some nits from jar@'s review of that CL.

BUG=329620
R=jar@chromium.org
R=mmenke@chromium.org

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

[Randy Smith (Not in Mondays), 2014-04-22 20:41:50]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-04-22 20:43:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/248123002/1

[commit-bot: I haz the power, 2014-04-22 20:43:11]

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

[commit-bot: I haz the power, 2014-04-22 20:43:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Randy Smith (Not in Mondays), 2014-04-22 20:44:03]

On 2014/04/22 20:43:12, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Arggh, sorry, meant for that to be try bots.  I think I'm a little spacy :-J.

[jar (doing other things), 2014-04-23 01:58:51]

LGTM (you can talk or leave the comment below).

https://codereview.chromium.org/248123002/diff/20001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper_core.h (right):

https://codereview.chromium.org/248123002/diff/20001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_core.h:216: // This value is set
(|navigation_from_button_ != NO_BUTTON|) only
nit: I actually like the new prose much better... but would prefer to omit the
pseudo code (as I waste too much time reading the double negative... and don't
get more out of it that the prose).

This value is set only when a navigation has been initiated from the error page.

YMMV... not worth arguing.  The prose helped me a lot. thx!

[mmenke, 2014-04-23 14:29:08]

LGTM

[Randy Smith (Not in Mondays), 2014-04-23 18:36:58]

Thanks!

NIco, willing to stamp neterror.js?  It's just a removal of curly braces
following a comment of Jim's.

https://codereview.chromium.org/248123002/diff/20001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper_core.h (right):

https://codereview.chromium.org/248123002/diff/20001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_core.h:216: // This value is set
(|navigation_from_button_ != NO_BUTTON|) only
On 2014/04/23 01:58:51, jar wrote:
> nit: I actually like the new prose much better... but would prefer to omit the
> pseudo code (as I waste too much time reading the double negative... and don't
> get more out of it that the prose).
> 
> This value is set only when a navigation has been initiated from the error
page.
> 
> YMMV... not worth arguing.  The prose helped me a lot. thx!
> 
Yeah, I wasn't sure that everyone would realize that "NO_BUTTON" is the same as
not set.  But It's clear enough, and I agree with you the text is better without
the pseudo-code.  Done.

[Nico, 2014-04-23 19:15:29]

lgtm

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_page_controller.cc (right):

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_page_controller.cc:80: isolate)
This seems worse than the lhs. The lhs is also what clang-format produces. I'd
revert the change to this file.

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:81: if (window.errorPageController)
This change is inconsistent with loadStaleButtonClick a few lines above, and the
other functions in this file. I'd revert the change to this file.

[Randy Smith (Not in Mondays), 2014-04-23 19:31:54]

On 2014/04/23 19:15:29, Nico wrote:
> lgtm
> 
>
https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
> File chrome/renderer/net/net_error_page_controller.cc (right):
> 
>
https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
> chrome/renderer/net/net_error_page_controller.cc:80: isolate)
> This seems worse than the lhs. The lhs is also what clang-format produces. I'd
> revert the change to this file.
> 
>
https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
> File chrome/renderer/resources/neterror.js (right):
> 
>
https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
> chrome/renderer/resources/neterror.js:81: if (window.errorPageController)
> This change is inconsistent with loadStaleButtonClick a few lines above, and
the
> other functions in this file. I'd revert the change to this file.

Both changes were made in response to another reviewers requests.  Jim, WDYT? 
I'm neutral.

[Nico, 2014-04-23 19:41:42]

On Wed, Apr 23, 2014 at 12:31 PM, <rdsmith@chromium.org> wrote:

> On 2014/04/23 19:15:29, Nico wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.org/248123002/diff/40001/
> chrome/renderer/net/net_error_page_controller.cc
>
>> File chrome/renderer/net/net_error_page_controller.cc (right):
>>
>
>
> https://codereview.chromium.org/248123002/diff/40001/
> chrome/renderer/net/net_error_page_controller.cc#newcode80
>
>> chrome/renderer/net/net_error_page_controller.cc:80: isolate)
>> This seems worse than the lhs. The lhs is also what clang-format
>> produces. I'd
>> revert the change to this file.
>>
>
>
> https://codereview.chromium.org/248123002/diff/40001/
> chrome/renderer/resources/neterror.js
>
>> File chrome/renderer/resources/neterror.js (right):
>>
>
>
> https://codereview.chromium.org/248123002/diff/40001/
> chrome/renderer/resources/neterror.js#newcode81
>
>> chrome/renderer/resources/neterror.js:81: if (window.errorPageController)
>> This change is inconsistent with loadStaleButtonClick a few lines above,
>> and
>>
> the
>
>> other functions in this file. I'd revert the change to this file.
>>
>
> Both changes were made in response to another reviewers requests.  Jim,
> WDYT?
> I'm neutral.
>

(As a general guideline: Since there's a cost to formatting-only changes
(makes blame harder to read), I'd only do them when they're clearly better,
such as the constructor list reindent that's also part of this change.)


>
>
> https://codereview.chromium.org/248123002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Randy Smith (Not in Mondays), 2014-04-28 16:35:59]

In the absence of commentary from Jim, and in the interests of getting the
histograms landed, I've incorporated Nico's comments.

Thanks, all!

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_page_controller.cc (right):

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_page_controller.cc:80: isolate)
On 2014/04/23 19:15:30, Nico wrote:
> This seems worse than the lhs. The lhs is also what clang-format produces. I'd
> revert the change to this file.

Done (via git cl format, which combined two lines below as well).

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:81: if (window.errorPageController)
On 2014/04/23 19:15:30, Nico wrote:
> This change is inconsistent with loadStaleButtonClick a few lines above, and
the
> other functions in this file. I'd revert the change to this file.

Done.

[Randy Smith (Not in Mondays), 2014-04-28 16:36:07]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/248123002/60001

[commit-bot: I haz the power, 2014-04-28 16:54:49]

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

[commit-bot: I haz the power, 2014-04-28 16:54:51]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Randy Smith (Not in Mondays), 2014-04-28 17:02:50]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:03:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/248123002/60001

[jar (doing other things), 2014-04-28 17:55:08]

histograms.xml LGTM

You can decide which style to consistently use, per comments below.  Thanks!

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/248123002/diff/40001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:81: if (window.errorPageController)
On 2014/04/23 19:15:30, Nico wrote:
> This change is inconsistent with loadStaleButtonClick a few lines above, and
the
> other functions in this file. I'd revert the change to this file.

FWIW: The change is consistent with existing lines 27, 43, and 50.  

line 75 appears to be the only example using the curlies on a one-line
statement... and should IMO probably not have the curlies.

[commit-bot: I haz the power, 2014-04-29 00:14:26]

Change committed as 266711