[Android] Add support for a hung renderer dialog

[Android] Add support for a hung renderer dialog

Implement the hung renderer dialog in terms of an infobar. Reduce the
default hang timeout on Android from 30 seconds to 5 seconds.

BUG=148136
Committed: https://crrev.com/b9cdfa9500bec95d05f19d840801610a79c77ce5
Cr-Commit-Position: refs/heads/master@{#347229}

[gone, 2015-08-14 21:05:42]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2015-08-14 21:05:43]

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2810: public
void onInfoBarDismissed(InfoBar infoBar) {}
You probably also want to clear the mRendererUnresponsiveInfoBar field here (by
setting it to null).  This happens when the user just hits the X on it instead
of acting.  You might want to double check if this is called when the user
navigates somewhere else because that might also dismiss the infobar.

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:2250: Kill
Probably want to use less forceful language.  Run this by UX, but maybe
initially:

"The page has become unresponsive.  Would you like to reload the page?"
"YES"  /  "MAYBE LATER"

[gone, 2015-08-14 21:07:28]

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2798:
getInfoBarContainer().addInfoBar(mRendererUnresponsiveInfoBar);
You probably want to make a native-side InfoBarDelegate.  We've been (sort of)
trying to get rid of Java-only infobars.

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:2250: Kill
On 2015/08/14 21:05:43, dfalcantara wrote:
> Probably want to use less forceful language.  Run this by UX, but maybe
> initially:
> 
> "The page has become unresponsive.  Would you like to reload the page?"
> "YES"  /  "MAYBE LATER"

MAYBE LATER is kind of a joke but not really.  Desktop chrome asks if you want
to wait, right?

[jdduke (slow), 2015-08-14 21:22:19]

Probably should have added a (WIP) to the title but early feedback appreciated
:)

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2798:
getInfoBarContainer().addInfoBar(mRendererUnresponsiveInfoBar);
On 2015/08/14 21:07:28, dfalcantara wrote:
> You probably want to make a native-side InfoBarDelegate.  We've been (sort of)
> trying to get rid of Java-only infobars.

I see, and just host the infobar in the native Tab counterpart or something like
that?

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2810: public
void onInfoBarDismissed(InfoBar infoBar) {}
On 2015/08/14 21:05:43, dfalcantara wrote:
> You probably also want to clear the mRendererUnresponsiveInfoBar field here
(by
> setting it to null).  This happens when the user just hits the X on it instead
> of acting.  You might want to double check if this is called when the user
> navigates somewhere else because that might also dismiss the infobar.

Done.

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:2250: Kill
On 2015/08/14 21:07:28, dfalcantara wrote:
> On 2015/08/14 21:05:43, dfalcantara wrote:
> > Probably want to use less forceful language.  Run this by UX, but maybe
> > initially:
> > 
> > "The page has become unresponsive.  Would you like to reload the page?"
> > "YES"  /  "MAYBE LATER"
> 
> MAYBE LATER is kind of a joke but not really.  Desktop chrome asks if you want
> to wait, right?

Chrome asks to wait or kill, I basically stole the language from them (though
the desktop dialog can show multiple frozen tabs so it's plural).

I definitely agree it'd be nice to have a softer tone here, chatting with rolfe@
next week about this.

[gone, 2015-08-14 21:31:08]

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1299513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2798:
getInfoBarContainer().addInfoBar(mRendererUnresponsiveInfoBar);
On 2015/08/14 21:22:19, jdduke wrote:
> On 2015/08/14 21:07:28, dfalcantara wrote:
> > You probably want to make a native-side InfoBarDelegate.  We've been (sort
of)
> > trying to get rid of Java-only infobars.
> 
> I see, and just host the infobar in the native Tab counterpart or something
like
> that?

Something like that... This sounds like it'll be a plain old ConfirmInfoBar kind
of thing, which can extend from the ConfirmInfoBarDelegate.  If you had to you
could hold onto one from the TabAndroid so you can dismiss it if the renderer
starts responding, but some other places don't seem to care about holding onto
them (see chrome_download_delegate.cc)

[jdduke (slow), 2015-08-18 17:39:30]

OK, switched to native InfoBars. After we get this into a reasonable state I'll
add some UMA tracking.

https://codereview.chromium.org/1299513002/diff/40001/chrome/browser/android/...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/40001/chrome/browser/android/...
chrome/browser/android/hung_renderer_infobar_delegate.cc:18: :
web_contents_(web_contents) {
There's not much code here, I thought of just embedding this class directly in
tab_android.cc...

[gone, 2015-08-18 18:21:36]

https://codereview.chromium.org/1299513002/diff/40001/chrome/browser/android/...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/40001/chrome/browser/android/...
chrome/browser/android/hung_renderer_infobar_delegate.cc:18: :
web_contents_(web_contents) {
On 2015/08/18 17:39:29, jdduke wrote:
> There's not much code here, I thought of just embedding this class directly in
> tab_android.cc...

Eh, the less things we shove into TabAndroid the better.  Tab and its native
counterpart are like black holes.

[gone, 2015-08-18 20:45:18]

Ggiven how short the manual test case is, it seems that adding a InfoBarTest
would be relatively easy (something like testInfoBarForGeolocation, but for
hangs).  When the design settles down, can you add a few for making sure the
infobar appears and that either hitting "kill", hitting "wait", or  doing
nothing all work as expected?

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:829: if (!hung_renderer_infobar_)
instead of hanging onto the infobar*, would it make sense to iterate through the
InfobarService/InfobarManager using infobar_count() and infobar_at() to get
access to the infobar?  it'd avoid having to observe the InfobarService and
would prevent us from ending up with the pointers out of sync.

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.h:151: // Overriden from
infobars::InfoBarManager::Observer:
nit: while you're here, could you make these observer comments consistent?

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.h:256: // Transient InfoBar handle allowing
dismissal of the hung renderer infobar
nit: grammar is weird

[jdduke (slow), 2015-08-21 20:32:47]

Patchset #5 (id:80001) has been deleted

[jdduke (slow), 2015-08-21 21:18:31]

Added a test.

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:829: if (!hung_renderer_infobar_)
On 2015/08/18 20:45:17, dfalcantara wrote:
> instead of hanging onto the infobar*, would it make sense to iterate through
the
> InfobarService/InfobarManager using infobar_count() and infobar_at() to get
> access to the infobar?  it'd avoid having to observe the InfobarService and
> would prevent us from ending up with the pointers out of sync.

How would we know we've found it while iterating if we don't store a handle to
it? We'd need some kind of ID or type identifier, right?

I suppose we could create a new InfoBarDelegate type and add some overloading to
the existing casting methods on the delegate, then just try casting all the
delegates (AsHungRendererInfoBarDelegate())  until we get a valid result, but
I'm not sure if that is any better...

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.h:151: // Overriden from
infobars::InfoBarManager::Observer:
On 2015/08/18 20:45:17, dfalcantara wrote:
> nit: while you're here, could you make these observer comments consistent?

Done.

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.h:151: // Overriden from
infobars::InfoBarManager::Observer:
On 2015/08/18 20:45:17, dfalcantara wrote:
> nit: while you're here, could you make these observer comments consistent?

Done.

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.h:256: // Transient InfoBar handle allowing
dismissal of the hung renderer infobar
On 2015/08/18 20:45:17, dfalcantara wrote:
> nit: grammar is weird

Done.

[gone, 2015-08-21 21:39:30]

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1299513002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:829: if (!hung_renderer_infobar_)
On 2015/08/21 21:18:30, jdduke wrote:
> On 2015/08/18 20:45:17, dfalcantara wrote:
> > instead of hanging onto the infobar*, would it make sense to iterate through
> the
> > InfobarService/InfobarManager using infobar_count() and infobar_at() to get
> > access to the infobar?  it'd avoid having to observe the InfobarService and
> > would prevent us from ending up with the pointers out of sync.
> 
> How would we know we've found it while iterating if we don't store a handle to
> it? We'd need some kind of ID or type identifier, right?
> 
> I suppose we could create a new InfoBarDelegate type and add some overloading
to
> the existing casting methods on the delegate, then just try casting all the
> delegates (AsHungRendererInfoBarDelegate())  until we get a valid result, but
> I'm not sure if that is any better...

I think it'd be a little better safety-wise to add the
AsHungRendererInfoBarDelegate() call.  The number of infobars that are usually
visible is one or less, and we should be guaranteed a fresh pointer.  Could
always go for a second opinion, though.  Ping newt@?

[jdduke (slow), 2015-09-01 17:25:10]

Dan could you take another look? I've gone ahead with the approach you suggested
(looking up the InfoBar instead of caching it). I also added some UMA logging in
the delegate to record the action taken. I'll add more owners once you're happy.

[gone, 2015-09-01 18:49:34]

Yeah, lgtm.  Thanks!

https://chromiumcodereview.appspot.com/1299513002/diff/160001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://chromiumcodereview.appspot.com/1299513002/diff/160001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:196:
* Verifies the unresponsive renderer notification creates an InfoBar.
update the comment?

https://chromiumcodereview.appspot.com/1299513002/diff/160001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:236:
@Override
Other places in this file indent the @Overrides by 8.  Not sure what the style
guide says to do, but you may as well make it consistent with everything else in
here.

https://chromiumcodereview.appspot.com/1299513002/diff/160001/chrome/browser/...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/1299513002/diff/160001/chrome/browser/...
chrome/browser/android/hung_renderer_infobar_delegate.cc:79:
base::UserMetricsAction("MobileHungRendererInfoBarKill"));
Any reason you're doubling up on the count and histogram?  Seems like the
histogram should have all the information you'd get from a count.

[jdduke (slow), 2015-09-01 21:49:19]

jdduke@chromium.org changed reviewers:
+ asvitkine@chromium.org, pkasting@chromium.org, sievers@chromium.org

[jdduke (slow), 2015-09-01 21:49:20]

+pkasting for components/infobars
+asvitkine for tools/metrics
+sievers for content/common

https://codereview.chromium.org/1299513002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java
(right):

https://codereview.chromium.org/1299513002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:196:
* Verifies the unresponsive renderer notification creates an InfoBar.
On 2015/09/01 18:49:34, dfalcantara wrote:
> update the comment?

Done.

https://codereview.chromium.org/1299513002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:236:
@Override
On 2015/09/01 18:49:34, dfalcantara wrote:
> Other places in this file indent the @Overrides by 8.  Not sure what the style
> guide says to do, but you may as well make it consistent with everything else
in
> here.

Hmm, that seems to be when you put the "new Foo() {" on the next line. I see
both cases here between runOnUiThread and pollForCriteria. I tried changing it
to your suggestion but "git cl format" puts it back this way =/.

https://codereview.chromium.org/1299513002/diff/160001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/160001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:79:
base::UserMetricsAction("MobileHungRendererInfoBarKill"));
On 2015/09/01 18:49:34, dfalcantara wrote:
> Any reason you're doubling up on the count and histogram?  Seems like the
> histogram should have all the information you'd get from a count.

Yeah, I thought it might be nice to have this included in all the other fun user
metrics so you can compare frequency of killing to say, refreshing. But it's not
really necessary. I'll remove it.

[Peter Kasting, 2015-09-01 22:24:05]

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:24: LogAction(REMOVE);
This is just browser shutdown, right?  Is logging this case really necessary? 
If so, something like "BROWSER_SHUTDOWN" would be a better name than "REMOVE".

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:68: if
(action_logged_for_uma_)
The only reason we should get multiple actions logged is because of the call in
the destructor, right?  So if that goes away, this could go away, and if it
doesn't, it seems like this conditional could move to guard that call, making it
clearer what the source of dupe logging calls would be.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.h (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:14: // This class is
responsible for customizing a hung renderer infobar.
Nit: "customizing"?  That sounds as if there's already some concept of a hung
renderer infobar and you're tweaking it.  How about something like:

An infobar shown when a tab's renderer is hung.  <Further description of how
this looks and how user interacts with it>

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:15: // If the "kill"
dialog is clicked, the renderer of the corresponding
Nit: Did you mean "button" instead of "dialog"?

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:16: // WebContents will
be shutdown.
Nit: shut down (shutdown is a noun)

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:18: public:
Almost none of this stuff should be public.  You should instead have a public
static Create() function that can create the infobar and delegate and add it to
an infobar service.  Look at how all other infobars in the codebase work.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:25: // InfoBarDelegate:
You don't directly subclass InfoBarDelegate, so these should be in the
ConfirmInfoBarDelegate section.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:36: // Called when the
renderer regains responsiveness before the dialog is
Nit: when -> if?

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:42: enum Action { WAIT =
0, KILL, DISMISS, RESPONSIVE, REMOVE, ACTION_COUNT };
Nit: RESPONSIVE is not an action.  Perhaps you meant "Event" rather than
"Action" and wanted to name your events something like:

WAIT_CLICKED
KILL_CLICKED
INFOBAR_DISMISSED
RENDERER_BECAME_RESPONSIVE

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:109: if (!infobar || !infobar->delegate())
These cannot be null.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:112: if
(!infobar->delegate()->AsHungRendererInfoBarDelegate())
Nit: Simpler:

    if (infobar->delegate()->AsHungRendererInfoBarDelegate())
      return infobar;

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:864: if (!infobar_service)
When will this be null?  Either you shouldn't be checking it or it should have a
comment giving specific examples of when it's null.

[jdduke (slow), 2015-09-02 00:03:36]

Patchset #10 (id:200001) has been deleted

[jdduke (slow), 2015-09-02 00:04:01]

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:24: LogAction(REMOVE);
On 2015/09/01 22:24:05, Peter Kasting wrote:
> This is just browser shutdown, right?  Is logging this case really necessary? 
> If so, something like "BROWSER_SHUTDOWN" would be a better name than "REMOVE".

You tell me =/. Is there anything else that could result in an infobar being
shutdown? I'll just call it INFOBAR_SHUTDOWN for now.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:68: if
(action_logged_for_uma_)
On 2015/09/01 22:24:05, Peter Kasting wrote:
> The only reason we should get multiple actions logged is because of the call
in
> the destructor, right?  So if that goes away, this could go away, and if it
> doesn't, it seems like this conditional could move to guard that call, making
it
> clearer what the source of dupe logging calls would be.

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.h (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:14: // This class is
responsible for customizing a hung renderer infobar.
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: "customizing"?  That sounds as if there's already some concept of a hung
> renderer infobar and you're tweaking it.  How about something like:
> 
> An infobar shown when a tab's renderer is hung.  <Further description of how
> this looks and how user interacts with it>

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:15: // If the "kill"
dialog is clicked, the renderer of the corresponding
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: Did you mean "button" instead of "dialog"?

Yeah, rephrased in the new comment.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:16: // WebContents will
be shutdown.
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: shut down (shutdown is a noun)

Yeah, also removed in the new phrasing.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:18: public:
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Almost none of this stuff should be public.  You should instead have a public
> static Create() function that can create the infobar and delegate and add it
to
> an infobar service.  Look at how all other infobars in the codebase work.

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:25: // InfoBarDelegate:
On 2015/09/01 22:24:05, Peter Kasting wrote:
> You don't directly subclass InfoBarDelegate, so these should be in the
> ConfirmInfoBarDelegate section.

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:36: // Called when the
renderer regains responsiveness before the dialog is
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: when -> if?

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.h:42: enum Action { WAIT =
0, KILL, DISMISS, RESPONSIVE, REMOVE, ACTION_COUNT };
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: RESPONSIVE is not an action.  Perhaps you meant "Event" rather than
> "Action" and wanted to name your events something like:
> 
> WAIT_CLICKED
> KILL_CLICKED
> INFOBAR_DISMISSED
> RENDERER_BECAME_RESPONSIVE

I thought of using RESULT, as we'd really like just one per infobar. Event
works.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:109: if (!infobar || !infobar->delegate())
On 2015/09/01 22:24:05, Peter Kasting wrote:
> These cannot be null.

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:112: if
(!infobar->delegate()->AsHungRendererInfoBarDelegate())
On 2015/09/01 22:24:05, Peter Kasting wrote:
> Nit: Simpler:
> 
>     if (infobar->delegate()->AsHungRendererInfoBarDelegate())
>       return infobar;

Done.

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:864: if (!infobar_service)
On 2015/09/01 22:24:05, Peter Kasting wrote:
> When will this be null?  Either you shouldn't be checking it or it should have
a
> comment giving specific examples of when it's null.

I don't think it can be, I looked at some other code that has the check but I
think the lifetimes are pretty clear here.

[Alexei Svitkine (slow), 2015-09-02 15:26:10]

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

https://codereview.chromium.org/1299513002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:14466: +<histogram
name="HungRenderer.MobileInfoBar.Event"
Nit: This is adding a new top-level prefix. Unless you're planning to add many
histograms with this prefix, I suggest finding an existing prefix to nest this
under.

e.g. "Renderer.Hung.MobileInfoBar.UserEvent".

[jdduke (slow), 2015-09-02 16:56:07]

jdduke@chromium.org changed reviewers:
+ oshima@chromium.org

[jdduke (slow), 2015-09-02 16:56:07]

+oshima for chrome/app/theme

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

https://codereview.chromium.org/1299513002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:14466: +<histogram
name="HungRenderer.MobileInfoBar.Event"
On 2015/09/02 15:26:10, Alexei Svitkine wrote:
> Nit: This is adding a new top-level prefix. Unless you're planning to add many
> histograms with this prefix, I suggest finding an existing prefix to nest this
> under.
> 
> e.g. "Renderer.Hung.MobileInfoBar.UserEvent".

Done.

[Alexei Svitkine (slow), 2015-09-02 16:59:33]

lgtm

[oshima, 2015-09-02 17:11:27]

c/a/theme lgtm

[Peter Kasting, 2015-09-02 21:52:46]

LGTM

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/180001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:24: LogAction(REMOVE);
On 2015/09/02 00:04:01, jdduke wrote:
> On 2015/09/01 22:24:05, Peter Kasting wrote:
> > This is just browser shutdown, right?  Is logging this case really
necessary? 
> > If so, something like "BROWSER_SHUTDOWN" would be a better name than
"REMOVE".
> 
> You tell me =/. Is there anything else that could result in an infobar being
> shutdown? I'll just call it INFOBAR_SHUTDOWN for now.

I wasn't thinking clearly.  This is tab closure, not browser shutdown (though
browser shutdown implies first closing all the tabs).  Maybe name this
TAB_CLOSED.

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:24: return;
The only caller of this method has already assumed the infobar service pointer
is valid, so you don't need this conditional.

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:26: new
HungRendererInfoBarDelegate(web_contents));
Nit: You can avoid the Pass() call and the temp here by inlining this into the
next statement, which is what most Create() implementations do.

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:37: :
web_contents_(web_contents), terminal_event_logged_for_uma_(false) {
Nit: It seems like the only thing we save |web_contents_| for is to get the
render process host later.  That suggests that we should pass in the
RenderProcessHost* directly here (to minimize the dependencies of this object),
unless it's not guaranteed to be valid yet.

[jdduke (slow), 2015-09-02 23:00:35]

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
File chrome/browser/android/hung_renderer_infobar_delegate.cc (right):

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:24: return;
On 2015/09/02 21:52:45, Peter Kasting wrote:
> The only caller of this method has already assumed the infobar service pointer
> is valid, so you don't need this conditional.

Done.

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:26: new
HungRendererInfoBarDelegate(web_contents));
On 2015/09/02 21:52:45, Peter Kasting wrote:
> Nit: You can avoid the Pass() call and the temp here by inlining this into the
> next statement, which is what most Create() implementations do.

Done.

https://codereview.chromium.org/1299513002/diff/240001/chrome/browser/android...
chrome/browser/android/hung_renderer_infobar_delegate.cc:37: :
web_contents_(web_contents), terminal_event_logged_for_uma_(false) {
On 2015/09/02 21:52:45, Peter Kasting wrote:
> Nit: It seems like the only thing we save |web_contents_| for is to get the
> render process host later.  That suggests that we should pass in the
> RenderProcessHost* directly here (to minimize the dependencies of this
object),
> unless it's not guaranteed to be valid yet.

Done.

[no sievers, 2015-09-03 18:10:01]

content lgtm

[jdduke (slow), 2015-09-03 18:41:10]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-09-03 18:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, oshima@chromium.org, asvitkine@chromium.org,
pkasting@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1299513002/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-03 18:41:20]

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

[commit-bot: I haz the power, 2015-09-03 20:27:01]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-09-03 20:27:44]

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