[Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost is gone

[Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost is gone

In PresentationServiceDelegateImpl::RemoveObserver(), we should also erase PresentationFrame entry from PresentationFrameManager.

BUG=685879
Review-Url: https://codereview.chromium.org/2657613007
Cr-Commit-Position: refs/heads/master@{#447040}
Committed: https://chromium.googlesource.com/chromium/src/+/433b648885b0b665053a43f8cd603dc52ccb44c2

[zhaobin, 2017-01-27 02:57:06]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org

[imcheng, 2017-01-27 20:00:01]

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:122: // Used
by PresentationServiceDelegateImpl to manage
Could you update this comment please?

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:485: const
auto it = presentation_frames_.find(render_frame_host_id);
This can just be presentation_frames_.erase(render_frame_host_id);

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:678:
frame_manager_->AddPresentationFrame(
It seems to me we don't need to explicitly add a PresentationFrame because it
gets created on demand.

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:685:
frame_manager_->RemovePresentationFrame(
I think it would make more sense to erase the PresentationFrame on Reset(). It
would change the lifetime of PresentationFrame slightly but it would align it
with the lifetime of a document, which is more intuitive. WDYT?

[zhaobin, 2017-01-27 23:42:50]

Integration test passed.
Manual test with youtube and general mirroring passed.

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:122: // Used
by PresentationServiceDelegateImpl to manage
On 2017/01/27 20:00:01, imcheng wrote:
> Could you update this comment please?

Done.

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:485: const
auto it = presentation_frames_.find(render_frame_host_id);
On 2017/01/27 20:00:01, imcheng wrote:
> This can just be presentation_frames_.erase(render_frame_host_id);

Code removed.

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:678:
frame_manager_->AddPresentationFrame(
On 2017/01/27 20:00:01, imcheng wrote:
> It seems to me we don't need to explicitly add a PresentationFrame because it
> gets created on demand.

Done.

https://codereview.chromium.org/2657613007/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:685:
frame_manager_->RemovePresentationFrame(
On 2017/01/27 20:00:01, imcheng wrote:
> I think it would make more sense to erase the PresentationFrame on Reset(). It
> would change the lifetime of PresentationFrame slightly but it would align it
> with the lifetime of a document, which is more intuitive. WDYT?

Done.

[imcheng, 2017-01-28 03:57:23]

lgtm thanks!

[zhaobin, 2017-01-30 18:31:16]

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

[commit-bot: I haz the power, 2017-01-30 18:31:40]

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, 2017-01-30 19:34:34]

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

[commit-bot: I haz the power, 2017-01-30 19:34:34]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-01-30 19:50:21]

The CQ bit was checked by zhaobin@chromium.org

[commit-bot: I haz the power, 2017-01-30 19:51:00]

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-30 19:55:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485805821932650,
"parent_rev": "61e45cfa8c57280973848e75d31eae78c27a3e62", "commit_rev":
"433b648885b0b665053a43f8cd603dc52ccb44c2"}

[commit-bot: I haz the power, 2017-01-30 19:56:18]

Description was changed from

==========
[Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost
is gone

In PresentationServiceDelegateImpl::RemoveObserver(), we should also erase
PresentationFrame entry from PresentationFrameManager.

BUG=685879
==========

to

==========
[Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost
is gone

In PresentationServiceDelegateImpl::RemoveObserver(), we should also erase
PresentationFrame entry from PresentationFrameManager.

BUG=685879

Review-Url: https://codereview.chromium.org/2657613007
Cr-Commit-Position: refs/heads/master@{#447040}
Committed:
https://chromium.googlesource.com/chromium/src/+/433b648885b0b665053a43f8cd60...
==========

[commit-bot: I haz the power, 2017-01-30 19:56:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/433b648885b0b665053a43f8cd60...