Support listening for available screens for multiple presentation urls in the same frame

Support listening for available screens for multiple
Presentation URLs on a per frame basis.

This is done by associating PresentationServiceImpl
with the RenderFrameHost. When Presentation API
requests are passed to PresentationServiceImpl, it
will delegate to the embedder's Media Router along
with the frame id via the PresentationServiceDelegate
interface.

Each PresentationServiceImpl also maintains a hash
map of Presentation URLs currently registered to
"context" objects. These objects wait for both result
and renderer (i.e. callback) to both be ready before
sending the result back to renderer /
PresentationDispatcher.

Furthermore, PresentationServiceImpl listens for
changes to the RenderFrameHost. If an out of page
navigation occurred or if RenderFrameHost is
being destroyed, all current Presentation URLs
are unregistered.

Also added unit tests for PresentationSerivceImpl.

BUG=412331
Committed: https://crrev.com/ec17a20355dbc3b05e2133c0112c332167b63ce7
Cr-Commit-Position: refs/heads/master@{#315442}

[imcheng, 2015-02-09 07:29:04]

imcheng@chromium.org changed reviewers:
+ avayvod@chromium.org, avi@chromium.org, imcheng@chromium.org

[imcheng, 2015-02-09 07:29:05]

Avi: please review render_frame_host_impl.cc.
Anton: please review everything. All of the changes here are from the patch that
landed in the private repo.

Thanks!

[whywhat, 2015-02-09 10:17:08]

lgtm with some nits

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:28:
DCHECK(render_frame_host_);
Move the DCHECKs before the VLOG?

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:66: VLOG(2) <<
"PresentationServiceImpl::GetNextScreenAvailability";
s/Next//

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:73: new
ScreenAvailabilityContext(presentation_url.get()));
Note that PresentationDispatcher can pass a null mojo string here (the frame
hasn't specified any DPU). For now it only passes a null string. Perhaps change
PresentationDispatcher to pass an empty but not-null string?

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:16: #include
"content/public/browser/presentation_service_delegate.h"
should we put these classes into a presentation/ subfolder?

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:124: //
ScreenAvailabilityListener implementation.
s/ScreenAvailabilityListener/PresentationScreenAvailabilityListener?

[whywhat, 2015-02-09 12:56:55]

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl_unittest.cc:53:
EXPECT_CALL(mock_delegate_, AddObserver(_)).Times(1);
Also I'm a supporter of having most/all EXPECT statements in the test cases even
if it means some code duplication. E.g if GetScreenAvailability test case fails
in one of the GetScreenAvailabilityAndWait calls, you wouldn't know which of the
two from the logs since the EXPECT macro will print the same line number for
each.

[Avi (use Gerrit), 2015-02-09 14:19:39]

LGTM

[imcheng (use chromium acct), 2015-02-09 22:31:20]

New patchsets have been uploaded after l-g-t-m from
avayvod@chromium.org,avi@chromium.org

[imcheng, 2015-02-09 22:33:33]

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:28:
DCHECK(render_frame_host_);
On 2015/02/09 10:17:07, whywhat wrote:
> Move the DCHECKs before the VLOG?

Done.

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:66: VLOG(2) <<
"PresentationServiceImpl::GetNextScreenAvailability";
On 2015/02/09 10:17:07, whywhat wrote:
> s/Next//

Done.

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:73: new
ScreenAvailabilityContext(presentation_url.get()));
On 2015/02/09 10:17:07, whywhat wrote:
> Note that PresentationDispatcher can pass a null mojo string here (the frame
> hasn't specified any DPU). For now it only passes a null string. Perhaps
change
> PresentationDispatcher to pass an empty but not-null string?

It's fine to pass in a null mojo::String to PSImpl. It will just check for null
and fall back to default Presentation URL here. We should add the presentation
URL parameter to updateAvailableChangeWatched and complete the rest of the
Presentation API implementation in Blink.

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:16: #include
"content/public/browser/presentation_service_delegate.h"
On 2015/02/09 10:17:07, whywhat wrote:
> should we put these classes into a presentation/ subfolder?

It looks like there are no subholders in content/public/browser besides android.
It looks any changes made in content/public/browser must be reviewed by one of
the content/ OWNERs. Maybe Avi can confirm? Though it will be more organized if
we have a subfolder.

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:124: //
ScreenAvailabilityListener implementation.
On 2015/02/09 10:17:07, whywhat wrote:
> s/ScreenAvailabilityListener/PresentationScreenAvailabilityListener?

Done.

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/906113002/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl_unittest.cc:53:
EXPECT_CALL(mock_delegate_, AddObserver(_)).Times(1);
On 2015/02/09 12:56:55, whywhat wrote:
> Also I'm a supporter of having most/all EXPECT statements in the test cases
even
> if it means some code duplication. E.g if GetScreenAvailability test case
fails
> in one of the GetScreenAvailabilityAndWait calls, you wouldn't know which of
the
> two from the logs since the EXPECT macro will print the same line number for
> each.

So I tried doing what you suggested. The quirkyness of base::RunLoop makes it
really hard (e.g. it has to be initialized after
RenderViewHostImplTestHarness::SetUp() otherwise you get a weird stack trace,
can only be run once per RunLoop but you also need to save the quit closure for
later callback) and makes the code really bloated. It's also less readable
because now the EXPECT_CALLs are located where it makes less sense. 
I think it's not really worth the effort. If we need to differentiate between
the two calls, we can log the arguments passed in. I've done that here.

[imcheng, 2015-02-09 22:45:51]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2015-02-09 22:47:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/906113002/20001

[commit-bot: I haz the power, 2015-02-10 00:02:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-10 00:03:28]

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

[jiayl, 2015-02-10 00:19:11]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/908073002/ by jiayl@chromium.org.

The reason for reverting is: Bot failure:
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Linux%20Builder%20....