Tab capture: Tracking continues after renderer crash.

Tab capture: Tracking continues after renderer crash.

Fixes a bug where tab capture would freeze whenever the renderer
crashes, and would not resume capturing after navigating to a new page.
There were a number of causes, all of which amounted to:

1. Having WebContentsTracker observe an extra event, RenderFrameCreated.
2. Having WebContentsTracker report on changes to RenderWidgetHostView
   instead of just RenderWidgetHost.
3. Correcting an incorrect assumption: Tracking should not halt just
   because a RenderWidgetHost lacks a view. Intsead, it is the
   destruction of the WebContents that should cause tracking to halt.

BUG=677347
Committed: https://crrev.com/418dd6327b1241f4b667c54051ab2894c64c07a9
Cr-Commit-Position: refs/heads/master@{#441221}

[miu, 2016-12-29 04:59:44]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 04:59:54]

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

[miu, 2016-12-29 05:00:06]

miu@chromium.org changed reviewers:
+ xjz@chromium.org

[miu, 2016-12-29 05:00:07]

xjz: PTAL.

[commit-bot: I haz the power, 2016-12-29 05:48:34]

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

[commit-bot: I haz the power, 2016-12-29 05:48:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[miu, 2016-12-29 21:37:54]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:38:09]

Dry run: 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-12-29 22:27:56]

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

[commit-bot: I haz the power, 2016-12-29 22:27:57]

Dry run: This issue passed the CQ dry run.

[xjz, 2017-01-03 19:16:41]

lgtm

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_tracker.cc (right):

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.cc:172:
OnPossibleTargetChange(true);
ooc: Will this cause an error report because RenewFrameSubscription() will be
called with |is_still_tracking| equal to false?

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_tracker.h (right):

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:62: bool
is_still_tracking() const { return !!web_contents(); }
nit: DCHECK_CURRENTLY_ON(BrowserThread::UI);?

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:82: // RenderWidgetHost has
not changed.
nit: s/RenderWidgetHost/RenderWidgetHostView

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:100: // main frame may have
changed..
nit: s/../.

[miu, 2017-01-03 21:01:28]

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_tracker.cc (right):

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.cc:172:
OnPossibleTargetChange(true);
On 2017/01/03 19:16:40, xjz wrote:
> ooc: Will this cause an error report because RenewFrameSubscription() will be
> called with |is_still_tracking| equal to false?

Yes. WAI. And, it should because the thing being captured has gone away while
video capture is still running. It's analogous to pulling the cord on your USB
webcam during video capture.

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_tracker.h (right):

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:62: bool
is_still_tracking() const { return !!web_contents(); }
On 2017/01/03 19:16:40, xjz wrote:
> nit: DCHECK_CURRENTLY_ON(BrowserThread::UI);?

We generally don't add these for inline accessors.

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:82: // RenderWidgetHost has
not changed.
On 2017/01/03 19:16:40, xjz wrote:
> nit: s/RenderWidgetHost/RenderWidgetHostView

Done. Good catch.

https://codereview.chromium.org/2602923002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_tracker.h:100: // main frame may have
changed..
On 2017/01/03 19:16:40, xjz wrote:
> nit: s/../.

Done.

[miu, 2017-01-03 21:01:34]

The CQ bit was checked by miu@chromium.org

[miu, 2017-01-03 21:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/2602923002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-01-03 21:02:15]

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, 2017-01-03 22:16:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483477294200060,
"parent_rev": "2ace45e1596cde7975e0c3bef909d1050367aa9d", "commit_rev":
"2f25f575065ea7fa290a5d5aea8a22f19787879b"}

[commit-bot: I haz the power, 2017-01-03 22:17:14]

Description was changed from

==========
Tab capture: Tracking continues after renderer crash.

Fixes a bug where tab capture would freeze whenever the renderer
crashes, and would not resume capturing after navigating to a new page.
There were a number of causes, all of which amounted to:

1. Having WebContentsTracker observe an extra event, RenderFrameCreated.
2. Having WebContentsTracker report on changes to RenderWidgetHostView
   instead of just RenderWidgetHost.
3. Correcting an incorrect assumption: Tracking should not halt just
   because a RenderWidgetHost lacks a view. Intsead, it is the
   destruction of the WebContents that should cause tracking to halt.

BUG=677347
==========

to

==========
Tab capture: Tracking continues after renderer crash.

Fixes a bug where tab capture would freeze whenever the renderer
crashes, and would not resume capturing after navigating to a new page.
There were a number of causes, all of which amounted to:

1. Having WebContentsTracker observe an extra event, RenderFrameCreated.
2. Having WebContentsTracker report on changes to RenderWidgetHostView
   instead of just RenderWidgetHost.
3. Correcting an incorrect assumption: Tracking should not halt just
   because a RenderWidgetHost lacks a view. Intsead, it is the
   destruction of the WebContents that should cause tracking to halt.

BUG=677347

Review-Url: https://codereview.chromium.org/2602923002
==========

[commit-bot: I haz the power, 2017-01-03 22:17:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2017-01-03 22:19:46]

Description was changed from

==========
Tab capture: Tracking continues after renderer crash.

Fixes a bug where tab capture would freeze whenever the renderer
crashes, and would not resume capturing after navigating to a new page.
There were a number of causes, all of which amounted to:

1. Having WebContentsTracker observe an extra event, RenderFrameCreated.
2. Having WebContentsTracker report on changes to RenderWidgetHostView
   instead of just RenderWidgetHost.
3. Correcting an incorrect assumption: Tracking should not halt just
   because a RenderWidgetHost lacks a view. Intsead, it is the
   destruction of the WebContents that should cause tracking to halt.

BUG=677347

Review-Url: https://codereview.chromium.org/2602923002
==========

to

==========
Tab capture: Tracking continues after renderer crash.

Fixes a bug where tab capture would freeze whenever the renderer
crashes, and would not resume capturing after navigating to a new page.
There were a number of causes, all of which amounted to:

1. Having WebContentsTracker observe an extra event, RenderFrameCreated.
2. Having WebContentsTracker report on changes to RenderWidgetHostView
   instead of just RenderWidgetHost.
3. Correcting an incorrect assumption: Tracking should not halt just
   because a RenderWidgetHost lacks a view. Intsead, it is the
   destruction of the WebContents that should cause tracking to halt.

BUG=677347

Committed: https://crrev.com/418dd6327b1241f4b667c54051ab2894c64c07a9
Cr-Commit-Position: refs/heads/master@{#441221}
==========

[commit-bot: I haz the power, 2017-01-03 22:19:48]

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