Wire up OOM exception to the sad tab page.

Wire up OOM exception to the sad tab page.

BUG=630472
Committed: https://crrev.com/fa40c27241523f6bf6e542657ea64bd1ff9f146c
Cr-Commit-Position: refs/heads/master@{#407667}

[Will Harris, 2016-07-22 15:54:34]

Description was changed from

==========
Wire up OOM exception to the sad tab page.

BUG=630472
==========

to

==========
Wire up OOM exception to the sad tab page.

BUG=630472
==========

[Will Harris, 2016-07-22 15:54:34]

wfh@chromium.org changed reviewers:
+ mark@chromium.org, msw@chromium.org

[Will Harris, 2016-07-22 15:56:10]

wfh@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Will Harris, 2016-07-22 15:56:45]

PTAL

mark -> base/*
msw -> chrome/browser/ui/*
sdefresne -> strings

[Mark Mentovai, 2016-07-22 18:30:44]

LGTM in base following rejiggering

[msw, 2016-07-22 22:48:10]

lgtm with nits

https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
File chrome/browser/ui/sad_tab_types.h (right):

https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
chrome/browser/ui/sad_tab_types.h:17: SAD_TAB_OOM,         // Tab ran out of
memory.
nit: use two spaces (or align all comments)

https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
File components/new_or_sad_tab_strings.grdp (right):

https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
components/new_or_sad_tab_strings.grdp:17: Google Chrome ran out of memory while
trying to display this webpage.
optional nit: use IDS_SHORT_PRODUCT_NAME?

[Will Harris, 2016-07-22 23:23:56]

Thanks for reviews - sdefresne -> PTAL at the string change. Thanks!

https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
File chrome/browser/ui/sad_tab_types.h (right):

https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
chrome/browser/ui/sad_tab_types.h:17: SAD_TAB_OOM,         // Tab ran out of
memory.
On 2016/07/22 22:48:10, msw wrote:
> nit: use two spaces (or align all comments)

Done.

https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
File components/new_or_sad_tab_strings.grdp (right):

https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
components/new_or_sad_tab_strings.grdp:17: Google Chrome ran out of memory while
trying to display this webpage.
On 2016/07/22 22:48:10, msw wrote:
> optional nit: use IDS_SHORT_PRODUCT_NAME?

I found all the other .grdp were doing it this way, so I copied them... I'll let
sdefresne decide what is best here...

[sdefresne, 2016-07-25 17:22:21]

On 2016/07/22 23:23:56, Will Harris wrote:
> Thanks for reviews - sdefresne -> PTAL at the string change. Thanks!
> 
>
https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
> File chrome/browser/ui/sad_tab_types.h (right):
> 
>
https://codereview.chromium.org/2172013002/diff/20001/chrome/browser/ui/sad_t...
> chrome/browser/ui/sad_tab_types.h:17: SAD_TAB_OOM,         // Tab ran out of
> memory.
> On 2016/07/22 22:48:10, msw wrote:
> > nit: use two spaces (or align all comments)
> 
> Done.
> 
>
https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
> File components/new_or_sad_tab_strings.grdp (right):
> 
>
https://codereview.chromium.org/2172013002/diff/20001/components/new_or_sad_t...
> components/new_or_sad_tab_strings.grdp:17: Google Chrome ran out of memory
while
> trying to display this webpage.
> On 2016/07/22 22:48:10, msw wrote:
> > optional nit: use IDS_SHORT_PRODUCT_NAME?
> 
> I found all the other .grdp were doing it this way, so I copied them... I'll
let
> sdefresne decide what is best here...

We do not want to use IDS_SHORT_PRODUCT_NAME as it can cause issues with
localisation (see https://bugs.chromium.org/p/chromium/issues/detail?id=111425).

lgtm

[Will Harris, 2016-07-25 20:19:39]

Patchset #5 (id:80001) has been deleted

[Will Harris, 2016-07-25 20:20:26]

wfh@chromium.org changed reviewers:
+ asvitkine@chromium.org, reillyg@chromium.org

[Will Harris, 2016-07-25 20:21:18]

PTAL as I added missing histograms and case statements.

reillyg -> extensions/
asvitkine -> histograms.xml

[Alexei Svitkine (slow), 2016-07-25 20:39:00]

https://codereview.chromium.org/2172013002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2172013002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:118: UMA_HISTOGRAM_CUSTOM_COUNTS(
This is UMA_HISTOGRAM_COUNTS_1000().

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

https://codereview.chromium.org/2172013002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59179: +<histogram
name="Tabs.SadTab.OomCreated">
Add units="sad tabs" here and below. (Or something similar).

[Will Harris, 2016-07-25 20:47:53]

PTAL

https://codereview.chromium.org/2172013002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2172013002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:118: UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2016/07/25 20:39:00, Alexei Svitkine (slow) wrote:
> This is UMA_HISTOGRAM_COUNTS_1000().

Done.

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

https://codereview.chromium.org/2172013002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:59179: +<histogram
name="Tabs.SadTab.OomCreated">
On 2016/07/25 20:39:00, Alexei Svitkine (slow) wrote:
> Add units="sad tabs" here and below. (Or something similar).

Done. Used "tabs" as that's used elsewhere.

[Alexei Svitkine (slow), 2016-07-25 22:15:44]

lgtm % comment

https://codereview.chromium.org/2172013002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2172013002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:254:
UMA_HISTOGRAM_COUNTS_1000("Tabs.SadTab.CrashDisplayed", ++crashed);
Nit: I'd also put the increment outside the macro.

While I know it's OK in this case, macros are dangerous because the same arg can
be evaluated multiple times when expanded, so when I see a ++ in a macro, I
always wonder if it's correct. Now, for these ones I know it's fine, but other
readers might not. So best to remove doubt and put it in the line above. Same
for the one below.

[Will Harris, 2016-07-25 23:15:06]

wfh@chromium.org changed reviewers:
+ lazyboy@chromium.org
- reillyg@chromium.org

[Will Harris, 2016-07-25 23:15:32]

reillyg is ooo so -> lazyboy@ can you take a look at the minor change in
extensions?

https://codereview.chromium.org/2172013002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2172013002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:254:
UMA_HISTOGRAM_COUNTS_1000("Tabs.SadTab.CrashDisplayed", ++crashed);
On 2016/07/25 22:15:44, Alexei Svitkine (slow) wrote:
> Nit: I'd also put the increment outside the macro.
> 
> While I know it's OK in this case, macros are dangerous because the same arg
can
> be evaluated multiple times when expanded, so when I see a ++ in a macro, I
> always wonder if it's correct. Now, for these ones I know it's fine, but other
> readers might not. So best to remove doubt and put it in the line above. Same
> for the one below.

thanks for pointing this out, I also much prefer incrementing outside the macro.
Done.

[lazyboy, 2016-07-25 23:19:07]

web_view_guest.cc LGTM

[Will Harris, 2016-07-26 00:15:25]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-07-26 00:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
msw@chromium.org, sdefresne@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2172013002/#ps140001
(title: "increment outside macro")

[commit-bot: I haz the power, 2016-07-26 00:16:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Description was changed from

==========
Wire up OOM exception to the sad tab page.

BUG=630472
==========

to

==========
Wire up OOM exception to the sad tab page.

BUG=630472
==========

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

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-26 01:16:12]

Description was changed from

==========
Wire up OOM exception to the sad tab page.

BUG=630472
==========

to

==========
Wire up OOM exception to the sad tab page.

BUG=630472

Committed: https://crrev.com/fa40c27241523f6bf6e542657ea64bd1ff9f146c
Cr-Commit-Position: refs/heads/master@{#407667}
==========

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

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