[Media Router] Add origins to sink query results.

[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.

BUG=594858
Committed: https://crrev.com/c9adbb1e8d955e1f658d92578ab5a51e4dbc8872
Cr-Commit-Position: refs/heads/master@{#381332}

[imcheng, 2016-03-10 18:12:45]

Description was changed from

==========
[Media Router] Add origins to sink query results.


Takes over https://codereview.chromium.org/1693963003/

TODO: description, bug, tests

BUG=
==========

to

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


TODO: bug, tests

BUG=
==========

[imcheng, 2016-03-10 18:13:07]

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

[imcheng, 2016-03-10 18:13:07]

mfoltz@: PTAL. Tests to come.

[mark a. foltz, 2016-03-11 01:20:23]

Looks good overall, a couple of questions about
- Does every media source have an origin?
- Should we handle empty origins/empty lists specially?

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:304: // Called when the Media
Route Manager receives a new list of sinks
s/sinks//

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:192:
origins_list.push_back(origin);
If we receive an invalid origin from the MRPM, can we trust the result at all?

It would be simplest to log and drop the result on the floor rather than figure
out if it's okay to pass along to observers.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:416:
observer->OnSinksUpdated(sinks_query->cached_sink_list,
It seems like an observer could get an empty list without any origins, then an
empty list valid only for some list of origins.  Is that okay?  Or should we
assume an empty list is valid for all origins?

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:486: GURL
origin(origin_string);
GURL("https://google.com/")

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:525:
mojo_origins[0] = origin_string;
or origin.spec()

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.cc:34: if (origins.empty() ||
ContainsValue(origins, origin_))
#include "stl_util.h"

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:28: // with |source|.
Document meaning of |origin|.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:53: // OnSinksReceived will
result in undefined behavior.
This seems like common sense, but maybe it could be enforced by setting a flag
in OnSinksUpdated() and checking it in the dtor?

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:54: virtual void
OnSinksReceived(const std::vector<MediaSink>& sinks) = 0;
Usually virtual methods that are to be implemented by subclasses are declared
protected: or public:

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:177:
GetLastCommittedURLForFrame(render_frame_host_id_).GetOrigin()));
Can GetLastCommittedURLForFrame() ever fail?

What does getOrigin() return for non-http{s} schemes? (file:, javascript:,
chrome: etc.).

We may want a special meaning for "empty origin" in cases where the origin can't
be determined, but on the other hand, I think every RenderFrame must have some
origin.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:533: GURL
MediaRouterUI::GetOrigin(bool for_default_source) const {
I don't think we need a method for this.  Just pass
GURL(chrome::kChromeUIMediaRouterURL).GetOrigin() or
presentation_request_->frame_url().GetOrigin() at the two call sites.

https://codereview.chromium.org/1784533004/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1784533004/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:272:
this.service_.onSinksReceived(sourceUrn, sinks.map(sinkToMojo_),
Add a TODO to make |origins| mandatory in a future milestone.

[imcheng, 2016-03-14 21:30:21]

PTAL

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:304: // Called when the Media
Route Manager receives a new list of sinks
On 2016/03/11 01:20:23, mark a. foltz wrote:
> s/sinks//

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:192:
origins_list.push_back(origin);
On 2016/03/11 01:20:23, mark a. foltz wrote:
> If we receive an invalid origin from the MRPM, can we trust the result at all?
> 
> It would be simplest to log and drop the result on the floor rather than
figure
> out if it's okay to pass along to observers.

Changed it to do an early return if an origin is invalid.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:416:
observer->OnSinksUpdated(sinks_query->cached_sink_list,
On 2016/03/11 01:20:23, mark a. foltz wrote:
> It seems like an observer could get an empty list without any origins, then an
> empty list valid only for some list of origins.  Is that okay?  Or should we
> assume an empty list is valid for all origins?

It should be ok for now. In all cases, the observer will get back an empty list.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:486: GURL
origin(origin_string);
On 2016/03/11 01:20:23, mark a. foltz wrote:
> GURL("https://google.com/")

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:525:
mojo_origins[0] = origin_string;
On 2016/03/11 01:20:23, mark a. foltz wrote:
> or origin.spec()

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.cc:34: if (origins.empty() ||
ContainsValue(origins, origin_))
On 2016/03/11 01:20:23, mark a. foltz wrote:
> #include "stl_util.h"

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:28: // with |source|.
On 2016/03/11 01:20:23, mark a. foltz wrote:
> Document meaning of |origin|.

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:53: // OnSinksReceived will
result in undefined behavior.
On 2016/03/11 01:20:23, mark a. foltz wrote:
> This seems like common sense, but maybe it could be enforced by setting a flag
> in OnSinksUpdated() and checking it in the dtor?

Done.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:54: virtual void
OnSinksReceived(const std::vector<MediaSink>& sinks) = 0;
On 2016/03/11 01:20:23, mark a. foltz wrote:
> Usually virtual methods that are to be implemented by subclasses are declared
> protected: or public:

Done. Moved back to public

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:177:
GetLastCommittedURLForFrame(render_frame_host_id_).GetOrigin()));
On 2016/03/11 01:20:23, mark a. foltz wrote:
> Can GetLastCommittedURLForFrame() ever fail?
> 
> What does getOrigin() return for non-http{s} schemes? (file:, javascript:,
> chrome: etc.).
> 
> We may want a special meaning for "empty origin" in cases where the origin
can't
> be determined, but on the other hand, I think every RenderFrame must have some
> origin.

GetOrigin() will return empty for javascript:. I suspect the same for file: and
chrome: but I will do some testing to confirm. If an observer's origin is empty,
the observer will receive the full sink list only if the result's origins list
is empty.

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1784533004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:533: GURL
MediaRouterUI::GetOrigin(bool for_default_source) const {
On 2016/03/11 01:20:23, mark a. foltz wrote:
> I don't think we need a method for this.  Just pass
> GURL(chrome::kChromeUIMediaRouterURL).GetOrigin() or
> presentation_request_->frame_url().GetOrigin() at the two call sites.

Done.

https://codereview.chromium.org/1784533004/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1784533004/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:272:
this.service_.onSinksReceived(sourceUrn, sinks.map(sinkToMojo_),
On 2016/03/11 01:20:23, mark a. foltz wrote:
> Add a TODO to make |origins| mandatory in a future milestone.

Done.

[mark a. foltz, 2016-03-15 00:36:43]

LGTM % minor comments addressed

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:183: DLOG(ERROR) <<
"Received invalid origin: " << origin
Perhaps LOG(WARNING)

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer_unittest.cc (right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer_unittest.cc:23:
EXPECT_CALL(observer, OnSinksReceived(SequenceEquals(sink_list))).Times(1);
Note that Times(1) is the default cardinality for EXPECT_CALL

https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
(right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:172:
std::vector<GURL>());
Please use a non-empty origin list for one of these test cases.

[mark a. foltz, 2016-03-15 00:37:25]

Also can you please update the BUG= before committing.  Thanks!

[imcheng, 2016-03-15 01:47:10]

Description was changed from

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


TODO: bug, tests

BUG=
==========

to

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


BUG=594858
==========

[imcheng, 2016-03-15 01:52:42]

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:183: DLOG(ERROR) <<
"Received invalid origin: " << origin
On 2016/03/15 00:36:43, mark a. foltz wrote:
> Perhaps LOG(WARNING)

Done.

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer_unittest.cc (right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer_unittest.cc:23:
EXPECT_CALL(observer, OnSinksReceived(SequenceEquals(sink_list))).Times(1);
On 2016/03/15 00:36:43, mark a. foltz wrote:
> Note that Times(1) is the default cardinality for EXPECT_CALL
> 
>
https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...

Done.

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
(right):

https://codereview.chromium.org/1784533004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:172:
std::vector<GURL>());
On 2016/03/15 00:36:43, mark a. foltz wrote:
> Please use a non-empty origin list for one of these test cases.

Done.

[imcheng, 2016-03-15 18:32:36]

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

[imcheng, 2016-03-15 18:32:36]

+oshima for 	chrome/browser/ui/ash/cast_config_delegate_media_router.cc

[oshima, 2016-03-15 18:45:47]

lgtm

[imcheng, 2016-03-15 21:45:09]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-03-15 21:45:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1784533004/#ps160001
(title: "fix test")

[commit-bot: I haz the power, 2016-03-15 21:45:35]

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

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

Description was changed from

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


BUG=594858
==========

to

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


BUG=594858
==========

[commit-bot: I haz the power, 2016-03-15 22:17:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-15 22:18:38]

Description was changed from

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.


BUG=594858
==========

to

==========
[Media Router] Add origins to sink query results.

Takes over https://codereview.chromium.org/1693963003/

Add |origins| to |OnSinksReceived| API from MRPM to MR.
If |origins| is non-empty, then the result of sink query
is only valid for those origins. Otherwise, the result is
valid for all origins, which is the legacy behavior. The
origin checking is done in base MediaSinksObserver class,
which will call into the version of |OnSinksReceived|
overridable by children classes.

BUG=594858

Committed: https://crrev.com/c9adbb1e8d955e1f658d92578ab5a51e4dbc8872
Cr-Commit-Position: refs/heads/master@{#381332}
==========

[commit-bot: I haz the power, 2016-03-15 22:18:39]

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