MD Downloads: reset list tracker when renderer crashes

MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234
Committed: https://crrev.com/3ace0dbc408422826a06f2e96943f62b77376145
Cr-Commit-Position: refs/heads/master@{#392476}

[Dan Beam, 2016-05-09 16:55:15]

Description was changed from

==========
MD Downloads: reset list tracker when renderer crashes

R=asanka@chromium.org
BUG=610234
==========

to

==========
MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234
==========

[Dan Beam, 2016-05-09 16:55:16]

dbeam@chromium.org changed reviewers:
+ tommycli@chromium.org
- asanka@chromium.org

[Dan Beam, 2016-05-09 16:58:10]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-05-09 16:59:52]

/cc asanka@ and creis@ as FYI

creis@: yet another reason to make WebUIImpl a WebContentsObserver.  previous
issues[1].  tommycli@ might look into this.

[1]
https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u...

[tommycli, 2016-05-09 17:54:35]

https://codereview.chromium.org/1960223002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (right):

https://codereview.chromium.org/1960223002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:145: if (status
!= base::TERMINATION_STATUS_NORMAL_TERMINATION &&
Does using WebContents::IsCrashed() work here instead?

Also... maybe we should just always DisallowJavascript. I can't think of any
case where the RenderProcess is Gone (even for non-crash reasons) that we would
not want to disallow JS.

[Dan Beam, 2016-05-09 18:29:32]

https://codereview.chromium.org/1960223002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (right):

https://codereview.chromium.org/1960223002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:145: if (status
!= base::TERMINATION_STATUS_NORMAL_TERMINATION &&
On 2016/05/09 17:54:35, tommycli wrote:
> Does using WebContents::IsCrashed() work here instead?
> 
> Also... maybe we should just always DisallowJavascript. I can't think of any
> case where the RenderProcess is Gone (even for non-crash reasons) that we
would
> not want to disallow JS.

Done.

[tommycli, 2016-05-09 18:58:58]

LGTM

[Dan Beam, 2016-05-09 19:47:18]

dbeam@chromium.org changed reviewers:
+ creis@chromium.org

[Dan Beam, 2016-05-09 19:47:18]

+creis@ for content/public

[Charlie Reis, 2016-05-09 20:14:41]

On 2016/05/09 16:59:52, Dan Beam wrote:
> /cc asanka@ and creis@ as FYI
> 
> creis@: yet another reason to make WebUIImpl a WebContentsObserver.  previous
> issues[1].  tommycli@ might look into this.
> 
> [1]
>
https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u...

Can you file a bug for making WebUIImpl a WebContentsObserver (with this
context) and mention the bug number in your TODO?  We'll probably forget
otherwise, and the codesearch links in [1] are a bit stale by now.  We should
add some tests for this case when we do move it to WebUIImpl.

Otherwise this CL LGTM.

[Dan Beam, 2016-05-09 22:56:12]

On 2016/05/09 20:14:41, Charlie Reis wrote:
> On 2016/05/09 16:59:52, Dan Beam wrote:
> > /cc asanka@ and creis@ as FYI
> > 
> > creis@: yet another reason to make WebUIImpl a WebContentsObserver. 
previous
> > issues[1].  tommycli@ might look into this.
> > 
> > [1]
> >
>
https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u...
> 
> Can you file a bug for making WebUIImpl a WebContentsObserver (with this
> context) and mention the bug number in your TODO?  We'll probably forget
> otherwise, and the codesearch links in [1] are a bit stale by now.  We should
> add some tests for this case when we do move it to WebUIImpl.

Done. (and will do on tests)

> 
> Otherwise this CL LGTM.

[Dan Beam, 2016-05-09 22:56:17]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-05-09 22:56:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1960223002/#ps60001
(title: "asdf")

[commit-bot: I haz the power, 2016-05-09 22:57:01]

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

[commit-bot: I haz the power, 2016-05-09 23:42:31]

Description was changed from

==========
MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234
==========

to

==========
MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234
==========

[commit-bot: I haz the power, 2016-05-09 23:42:32]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-09 23:44:34]

Description was changed from

==========
MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234
==========

to

==========
MD Downloads: reset list tracker when renderer crashes

R=tommycli@chromium.org
BUG=610234

Committed: https://crrev.com/3ace0dbc408422826a06f2e96943f62b77376145
Cr-Commit-Position: refs/heads/master@{#392476}
==========

[commit-bot: I haz the power, 2016-05-09 23:44:36]

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