Pass origin to StartObservingMediaSinks.

Pass origin to StartObservingMediaSinks.

The DIAL provider needs to whitelist requests based on the origin of the
current page.  To make this happen, the Media Router interface needs to
be updated to pass the origin on the StartObservingMediaSinks call.

BUG=

[matt.boetger, 2016-02-12 23:34:50]

Patchset #1 (id:1) has been deleted

[matt.boetger, 2016-02-12 23:34:58]

Patchset #1 (id:20001) has been deleted

[matt.boetger, 2016-02-12 23:35:05]

Patchset #1 (id:40001) has been deleted

[matt.boetger, 2016-02-12 23:35:15]

Patchset #1 (id:60001) has been deleted

[matt.boetger, 2016-02-12 23:37:59]

Description was changed from

==========
Pass origin to StartObservingMediaSinks.

The DIAL provider needs to whitelist requests based on the origin of the
current page.  To make this happen, the Media Router interface needs to
be updated to pass the origin on the StartObservingMediaSinks call.

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

to

==========
Pass origin to StartObservingMediaSinks.

The DIAL provider needs to whitelist requests based on the origin of the
current page.  To make this happen, the Media Router interface needs to
be updated to pass the origin on the StartObservingMediaSinks call.

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

[matt.boetger, 2016-02-12 23:38:00]

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

[imcheng, 2016-02-16 20:54:18]

A couple of high level comments. We need to make sure sink list results are
passed back to the correct observers. We don't want to erroneously return a
(cached) non-empty list to origins that are not whitelisted.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:260: OnSinksReceived(string
media_source, array<MediaSink> sinks);
do we need an origin param here?

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:333:
base::ScopedPtrHashMap<MediaSource::Id, scoped_ptr<MediaSinksQuery>>
since queries with same source and different origin could return different
results, we should make origin part of the map key as well.

[mark a. foltz, 2016-02-17 21:50:51]

A few minor comments.

Re: Caching

It would be better to combine cache results across origins, otherwise, the cache
becomes less effective.  Can the sink observer return a "not authorized" result
which is not cached?

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:178: // |origin| can be used to
validate the media sink query to prevent
Maybe just say "to optionally verify that the origin is authorized to issue the
query."

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:418:
DCHECK(origin.is_valid());
You might want to DCHECK or LOG(WARNING) that the GURL doesn't have a path, it
means some truncation didn't happen upstream.

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:433: 
Extra newline

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_source_helper.cc:29: GURL OriginForDesktop() {
If we need an origin for tab and desktop casting, it would probably be the media
router WebUI, chrome://media-router

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/url_...

This is what we pass to CreateRoute()

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc:33:
GURL("http://example.com/presentation.html")));
Nit: This is not an origin; origin is scheme, host, and optional port.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:99: GURL
origin("http://fooUrl");
Nit: Technically this is a valid URL but can we use URLs with a TLD instead?

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:113: GURL
another_origin("http://barUrl");
Ditto

[imcheng (use chromium acct), 2016-02-18 00:33:16]

On 2016/02/17 21:50:51, mark a. foltz wrote:
> A few minor comments.
> 
> Re: Caching
> 
> It would be better to combine cache results across origins, otherwise, the
cache
> becomes less effective.  Can the sink observer return a "not authorized"
result
> which is not cached?
> 

To do that we would have to introduce DIAL-specific logic to MR. Since MR does
not know which sink queries can have results shared across origins and which
cannot.

>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/media_router.mojom (right):
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router.mojom:178: // |origin| can be used to
> validate the media sink query to prevent
> Maybe just say "to optionally verify that the origin is authorized to issue
the
> query."
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/media_router_mojo_impl.cc (right):
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router_mojo_impl.cc:418:
> DCHECK(origin.is_valid());
> You might want to DCHECK or LOG(WARNING) that the GURL doesn't have a path, it
> means some truncation didn't happen upstream.
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router_mojo_impl.cc:433: 
> Extra newline
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/media_source_helper.cc (right):
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_source_helper.cc:29: GURL OriginForDesktop()
{
> If we need an origin for tab and desktop casting, it would probably be the
media
> router WebUI, chrome://media-router
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/url_...
> 
> This is what we pass to CreateRoute()
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc:33:
> GURL("http://example.com/presentation.html")));
> Nit: This is not an origin; origin is scheme, host, and optional port.
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:99: GURL
> origin("http://fooUrl");
> Nit: Technically this is a valid URL but can we use URLs with a TLD instead?
> 
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:113:
GURL
> another_origin("http://barUrl");
> Ditto

[imcheng (use chromium acct), 2016-02-18 00:40:33]

On 2016/02/18 00:33:16, imcheng wrote:
> On 2016/02/17 21:50:51, mark a. foltz wrote:
> > A few minor comments.
> > 
> > Re: Caching
> > 
> > It would be better to combine cache results across origins, otherwise, the
> cache
> > becomes less effective.  Can the sink observer return a "not authorized"
> result
> > which is not cached?
> > 
> 
> To do that we would have to introduce DIAL-specific logic to MR. Since MR does
> not know which sink queries can have results shared across origins and which
> cannot.
> 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > File chrome/browser/media/router/media_router.mojom (right):
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router.mojom:178: // |origin| can be used
to
> > validate the media sink query to prevent
> > Maybe just say "to optionally verify that the origin is authorized to issue
> the
> > query."
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > File chrome/browser/media/router/media_router_mojo_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router_mojo_impl.cc:418:
> > DCHECK(origin.is_valid());
> > You might want to DCHECK or LOG(WARNING) that the GURL doesn't have a path,
it
> > means some truncation didn't happen upstream.
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router_mojo_impl.cc:433: 
> > Extra newline
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > File chrome/browser/media/router/media_source_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_source_helper.cc:29: GURL
OriginForDesktop()
> {
> > If we need an origin for tab and desktop casting, it would probably be the
> media
> > router WebUI, chrome://media-router
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/url_...
> > 
> > This is what we pass to CreateRoute()
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> > File
chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
> >
chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc:33:
> > GURL("http://example.com/presentation.html")));
> > Nit: This is not an origin; origin is scheme, host, and optional port.
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:99:
GURL
> > origin("http://fooUrl");
> > Nit: Technically this is a valid URL but can we use URLs with a TLD instead?
> > 
> >
>
https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:113:
> GURL
> > another_origin("http://barUrl");
> > Ditto

BTW, I do agree that keying observers by origin makes the caching less
effective. So it would be ideal if we can come up with a solution that avoids
doing that. Perhaps we can do something like modifying the sink query result to
mark it as not cached, or valid only for certain origins?

[matt.boetger, 2016-02-18 00:50:33]

I agree with Derek.  I don't think there is a way of doing this without passing
the origin back via the onSinksReceived call.  I addressed the other comments
with the most recent patch, but I'm waiting on a consensus before moving forward
with the cache changes.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:178: // |origin| can be used to
validate the media sink query to prevent
On 2016/02/17 21:50:51, mark a. foltz wrote:
> Maybe just say "to optionally verify that the origin is authorized to issue
the
> query."

Done.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:418:
DCHECK(origin.is_valid());
On 2016/02/17 21:50:51, mark a. foltz wrote:
> You might want to DCHECK or LOG(WARNING) that the GURL doesn't have a path, it
> means some truncation didn't happen upstream.

Done.

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:433: 
On 2016/02/17 21:50:51, mark a. foltz wrote:
> Extra newline

Done.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_source_helper.cc:29: GURL OriginForDesktop() {
On 2016/02/17 21:50:51, mark a. foltz wrote:
> If we need an origin for tab and desktop casting, it would probably be the
media
> router WebUI, chrome://media-router
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/url_...
> 
> This is what we pass to CreateRoute()

Done.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_media_sinks_observer_unittest.cc:33:
GURL("http://example.com/presentation.html")));
On 2016/02/17 21:50:51, mark a. foltz wrote:
> Nit: This is not an origin; origin is scheme, host, and optional port.

Done.

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:99: GURL
origin("http://fooUrl");
On 2016/02/17 21:50:51, mark a. foltz wrote:
> Nit: Technically this is a valid URL but can we use URLs with a TLD instead?

Done.

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:113: GURL
another_origin("http://barUrl");
On 2016/02/17 21:50:51, mark a. foltz wrote:
> Ditto

Done.

[mark a. foltz, 2016-02-19 00:34:53]

I think there's a consensus between Derek and I that we need to distinguish the
"no sinks" vs. "origin not allowed to get sinks" cases.  Next steps would be to
propose an API change to do that.

[matt.boetger, 2016-02-24 23:53:11]

On 2016/02/19 00:34:53, mark a. foltz wrote:
> I think there's a consensus between Derek and I that we need to distinguish
the
> "no sinks" vs. "origin not allowed to get sinks" cases.  Next steps would be
to
> propose an API change to do that.

I am waiting on the proposed API change before I move forward with this patch or
the SDK CL.

[mark a. foltz, 2016-02-25 17:06:42]

On 2016/02/24 at 23:53:11, boetger wrote:
> On 2016/02/19 00:34:53, mark a. foltz wrote:
> > I think there's a consensus between Derek and I that we need to distinguish
the
> > "no sinks" vs. "origin not allowed to get sinks" cases.  Next steps would be
to
> > propose an API change to do that.
> 
> I am waiting on the proposed API change before I move forward with this patch
or the SDK CL.

Are you waiting on a proposal from us?  Or do you have something to suggest?

[chromium-reviews, 2016-02-25 17:57:56]

As the owners of the API, I assumed you would want to propose the changes.
As I see it, this "consensus" doesn't address the issue of caching, but
perhaps I'm missing something. Derek has suggested in the past the origin
will need to be reported along with sinks as the media router will need to
know what sinks to show for what origin now. I've never gotten a definitive
answer if that's how you'd like to proceed.


On Thu, Feb 25, 2016, 9:06 AM  <mfoltz@chromium.org> wrote:

> On 2016/02/24 at 23:53:11, boetger wrote:
> > On 2016/02/19 00:34:53, mark a. foltz wrote:
> > > I think there's a consensus between Derek and I that we need to
> distinguish
> the
> > > "no sinks" vs. "origin not allowed to get sinks" cases. Next steps
> would be
> to
> > > propose an API change to do that.
> >
> > I am waiting on the proposed API change before I move forward with this
> patch
> or the SDK CL.
>
> Are you waiting on a proposal from us? Or do you have something to suggest?
>
> https://codereview.chromium.org/1693963003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[imcheng, 2016-02-25 19:18:36]

Okay, I will take a stab at this. So continuing from the idea in #10 my proposal
is the following:

- add an origin field to MediaSinksObserver.
- no changes required to StartObservingMediaSinks mojo API.
- OnSinksReceived mojo API is modified to return an optional origin. The origin
is cached along with the sink list in MediaSinksQuery, and the origin isa also
passed back to MediaSinksObserver. The cache will remain keyed by MediaSource.
- When MediaSinksObserver receives results, it first checks the origin provided
in the result against the observer's origin. If provided origin is non-empty and
does not match observer's origin, then return empty list. Otherwise proceed as
normal. This can be something implemented in the base MediaSinksObserver class.
- On the MRP side, DIAL provider needs to look up the whitelisted origin for a
given sinks query and return it along with the sink list.

Let me know what you guys think.

On 2016/02/25 17:57:56, chromium-reviews wrote:
> As the owners of the API, I assumed you would want to propose the changes.
> As I see it, this "consensus" doesn't address the issue of caching, but
> perhaps I'm missing something. Derek has suggested in the past the origin
> will need to be reported along with sinks as the media router will need to
> know what sinks to show for what origin now. I've never gotten a definitive
> answer if that's how you'd like to proceed.
> 
> 
> On Thu, Feb 25, 2016, 9:06 AM  <mailto:mfoltz@chromium.org> wrote:
> 
> > On 2016/02/24 at 23:53:11, boetger wrote:
> > > On 2016/02/19 00:34:53, mark a. foltz wrote:
> > > > I think there's a consensus between Derek and I that we need to
> > distinguish
> > the
> > > > "no sinks" vs. "origin not allowed to get sinks" cases. Next steps
> > would be
> > to
> > > > propose an API change to do that.
> > >
> > > I am waiting on the proposed API change before I move forward with this
> > patch
> > or the SDK CL.
> >
> > Are you waiting on a proposal from us? Or do you have something to suggest?
> >
> > https://codereview.chromium.org/1693963003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[matt.boetger, 2016-02-25 19:31:52]

To be clear, this would mean the app availability queries will run all the time,
but the filtering will happen in the media router.  Is that sufficient to
address the original concerns about fingerprinting, mfoltz@?

On 2016/02/25 19:18:36, imcheng1 wrote:
> Okay, I will take a stab at this. So continuing from the idea in #10 my
proposal
> is the following:
> 
> - add an origin field to MediaSinksObserver.
> - no changes required to StartObservingMediaSinks mojo API.
> - OnSinksReceived mojo API is modified to return an optional origin. The
origin
> is cached along with the sink list in MediaSinksQuery, and the origin isa also
> passed back to MediaSinksObserver. The cache will remain keyed by MediaSource.
> - When MediaSinksObserver receives results, it first checks the origin
provided
> in the result against the observer's origin. If provided origin is non-empty
and
> does not match observer's origin, then return empty list. Otherwise proceed as
> normal. This can be something implemented in the base MediaSinksObserver
class.
> - On the MRP side, DIAL provider needs to look up the whitelisted origin for a
> given sinks query and return it along with the sink list.
> 
> Let me know what you guys think.
> 
> On 2016/02/25 17:57:56, chromium-reviews wrote:
> > As the owners of the API, I assumed you would want to propose the changes.
> > As I see it, this "consensus" doesn't address the issue of caching, but
> > perhaps I'm missing something. Derek has suggested in the past the origin
> > will need to be reported along with sinks as the media router will need to
> > know what sinks to show for what origin now. I've never gotten a definitive
> > answer if that's how you'd like to proceed.
> > 
> > 
> > On Thu, Feb 25, 2016, 9:06 AM  <mailto:mfoltz@chromium.org> wrote:
> > 
> > > On 2016/02/24 at 23:53:11, boetger wrote:
> > > > On 2016/02/19 00:34:53, mark a. foltz wrote:
> > > > > I think there's a consensus between Derek and I that we need to
> > > distinguish
> > > the
> > > > > "no sinks" vs. "origin not allowed to get sinks" cases. Next steps
> > > would be
> > > to
> > > > > propose an API change to do that.
> > > >
> > > > I am waiting on the proposed API change before I move forward with this
> > > patch
> > > or the SDK CL.
> > >
> > > Are you waiting on a proposal from us? Or do you have something to
suggest?
> > >
> > > https://codereview.chromium.org/1693963003/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[mark a. foltz, 2016-02-25 19:56:39]

That seems reasonable, but a little complicated.  Now that I have seen the
whitelist, in the common case, there will only be one origin asking for a given
media source.  So I would also be okay extending MediaSource with an optional
origin and including it as part of the cache key.

[imcheng, 2016-02-25 20:28:02]

On 2016/02/25 19:56:39, mark a. foltz wrote:
> That seems reasonable, but a little complicated.  Now that I have seen the
> whitelist, in the common case, there will only be one origin asking for a
given
> media source.  So I would also be okay extending MediaSource with an optional
> origin and including it as part of the cache key.

mfoltz@: sgtm. Let's change the cache to also key by origin then.

[matt.boetger, 2016-02-25 22:58:26]

On 2016/02/25 20:28:02, imcheng1 wrote:
> On 2016/02/25 19:56:39, mark a. foltz wrote:
> > That seems reasonable, but a little complicated.  Now that I have seen the
> > whitelist, in the common case, there will only be one origin asking for a
> given
> > media source.  So I would also be okay extending MediaSource with an
optional
> > origin and including it as part of the cache key.
> 
> mfoltz@: sgtm. Let's change the cache to also key by origin then.

So we are going with Derek's suggestions as well as changing the case to key by
the origin?

If I follow Derek's logic, we'll need to send multiple OnSinksReceived calls - 1
for each origin in the whitelist - EVERY time we send onSinksReceived.  We can
reduce this if the origin is sent in the StartObservingMediaSinks call (like we
are doing now).  That way we know know what origin's are of interest.

[matt.boetger, 2016-02-25 22:58:29]

On 2016/02/25 20:28:02, imcheng1 wrote:
> On 2016/02/25 19:56:39, mark a. foltz wrote:
> > That seems reasonable, but a little complicated.  Now that I have seen the
> > whitelist, in the common case, there will only be one origin asking for a
> given
> > media source.  So I would also be okay extending MediaSource with an
optional
> > origin and including it as part of the cache key.
> 
> mfoltz@: sgtm. Let's change the cache to also key by origin then.

So we are going with Derek's suggestions as well as changing the case to key by
the origin?

If I follow Derek's logic, we'll need to send multiple OnSinksReceived calls - 1
for each origin in the whitelist - EVERY time we send onSinksReceived.  We can
reduce this if the origin is sent in the StartObservingMediaSinks call (like we
are doing now).  That way we know know what origin's are of interest.

[imcheng (use chromium acct), 2016-02-25 23:10:54]

On 2016/02/25 22:58:29, matt.boetger wrote:
> On 2016/02/25 20:28:02, imcheng1 wrote:
> > On 2016/02/25 19:56:39, mark a. foltz wrote:
> > > That seems reasonable, but a little complicated.  Now that I have seen the
> > > whitelist, in the common case, there will only be one origin asking for a
> > given
> > > media source.  So I would also be okay extending MediaSource with an
> optional
> > > origin and including it as part of the cache key.
> > 
> > mfoltz@: sgtm. Let's change the cache to also key by origin then.
> 
> So we are going with Derek's suggestions as well as changing the case to key
by
> the origin?
> 
> If I follow Derek's logic, we'll need to send multiple OnSinksReceived calls -
1
> for each origin in the whitelist - EVERY time we send onSinksReceived.  We can
> reduce this if the origin is sent in the StartObservingMediaSinks call (like
we
> are doing now).  That way we know know what origin's are of interest.

If we cache results by origin then there is no need to implement the changes I
described. You will only need to extend the cache to also key by origin.

So the flow will be like this:
scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink query
(source1, whitelistedOrigin) -> MR extension sends back (source1,
whitelistedOrigin, sink list) -> observer gets sink list
scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends sink
query (source1, nonWhitelistedOrigin) -> MR extension sends back (source1,
nonWhitelistedOrigin, []) -> observer gets []




FWIW, my proposal is actually the following. And with this you actually do not
need to cache result by origin.
scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink query
(source1) -> MR extension sends back (source1, sink list, origin for which the
result is valid) -> observer base class gets (sink list, origin for which the
result is valid), finds the origins match, and returns the full list to
observer.
scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends sink
query (source1) -> MR extension sends back (source1, sink list, origin for which
the result is valid) -> observer base class gets (sink list, origin for which
the result is valid), finds the origins do NOT match, and returns [] to
observer.

[matt.boetger, 2016-02-26 01:59:24]

On 2016/02/25 23:10:54, imcheng wrote:
> On 2016/02/25 22:58:29, matt.boetger wrote:
> > On 2016/02/25 20:28:02, imcheng1 wrote:
> > > On 2016/02/25 19:56:39, mark a. foltz wrote:
> > > > That seems reasonable, but a little complicated.  Now that I have seen
the
> > > > whitelist, in the common case, there will only be one origin asking for
a
> > > given
> > > > media source.  So I would also be okay extending MediaSource with an
> > optional
> > > > origin and including it as part of the cache key.
> > > 
> > > mfoltz@: sgtm. Let's change the cache to also key by origin then.
> > 
> > So we are going with Derek's suggestions as well as changing the case to key
> by
> > the origin?
> > 
> > If I follow Derek's logic, we'll need to send multiple OnSinksReceived calls
-
> 1
> > for each origin in the whitelist - EVERY time we send onSinksReceived.  We
can
> > reduce this if the origin is sent in the StartObservingMediaSinks call (like
> we
> > are doing now).  That way we know know what origin's are of interest.
> 
> If we cache results by origin then there is no need to implement the changes I
> described. You will only need to extend the cache to also key by origin.
> 
> So the flow will be like this:
> scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink
query
> (source1, whitelistedOrigin) -> MR extension sends back (source1,
> whitelistedOrigin, sink list) -> observer gets sink list
> scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends sink
> query (source1, nonWhitelistedOrigin) -> MR extension sends back (source1,
> nonWhitelistedOrigin, []) -> observer gets []
> 
> 
> 
> 
> FWIW, my proposal is actually the following. And with this you actually do not
> need to cache result by origin.
> scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink
query
> (source1) -> MR extension sends back (source1, sink list, origin for which the
> result is valid) -> observer base class gets (sink list, origin for which the
> result is valid), finds the origins match, and returns the full list to
> observer.
> scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends sink
> query (source1) -> MR extension sends back (source1, sink list, origin for
which
> the result is valid) -> observer base class gets (sink list, origin for which
> the result is valid), finds the origins do NOT match, and returns [] to
> observer.

Ok, I've started on this.  If we need it in by tomorrow I will work through the
night.  However, I want to be clarify that the following APIs WILL need to be
updated according to the scenarios described by Derek.  The statement "You will
only need to extend the cache to also key by origin." is simply not not true.

StartObservingMediaSinks will need origin added to the parameter list
StopObservingMediaSinks will need origin added to the parameter list
OnSinksReceived will need origin added to the parameter list

Internally, the sinks_query cache will be updated to key off a MediaSource
(which now has an origin associated with it).

[mark a. foltz, 2016-02-26 06:12:00]

On 2016/02/26 at 01:59:24, boetger wrote:
> On 2016/02/25 23:10:54, imcheng wrote:
> > On 2016/02/25 22:58:29, matt.boetger wrote:
> > > On 2016/02/25 20:28:02, imcheng1 wrote:
> > > > On 2016/02/25 19:56:39, mark a. foltz wrote:
> > > > > That seems reasonable, but a little complicated.  Now that I have seen
the
> > > > > whitelist, in the common case, there will only be one origin asking
for a
> > > > given
> > > > > media source.  So I would also be okay extending MediaSource with an
> > > optional
> > > > > origin and including it as part of the cache key.
> > > > 
> > > > mfoltz@: sgtm. Let's change the cache to also key by origin then.
> > > 
> > > So we are going with Derek's suggestions as well as changing the case to
key
> > by
> > > the origin?
> > > 
> > > If I follow Derek's logic, we'll need to send multiple OnSinksReceived
calls -
> > 1
> > > for each origin in the whitelist - EVERY time we send onSinksReceived.  We
can
> > > reduce this if the origin is sent in the StartObservingMediaSinks call
(like
> > we
> > > are doing now).  That way we know know what origin's are of interest.
> > 
> > If we cache results by origin then there is no need to implement the changes
I
> > described. You will only need to extend the cache to also key by origin.
> > 
> > So the flow will be like this:
> > scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink
query
> > (source1, whitelistedOrigin) -> MR extension sends back (source1,
> > whitelistedOrigin, sink list) -> observer gets sink list
> > scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends
sink
> > query (source1, nonWhitelistedOrigin) -> MR extension sends back (source1,
> > nonWhitelistedOrigin, []) -> observer gets []
> > 
> > 
> > 
> > 
> > FWIW, my proposal is actually the following. And with this you actually do
not
> > need to cache result by origin.
> > scenario 1: observer (source1, whitelistedOrigin) created -> MR sends sink
query
> > (source1) -> MR extension sends back (source1, sink list, origin for which
the
> > result is valid) -> observer base class gets (sink list, origin for which
the
> > result is valid), finds the origins match, and returns the full list to
> > observer.
> > scenario 2: observer (source1, nonWhitelistedOrigin) created -> MR sends
sink
> > query (source1) -> MR extension sends back (source1, sink list, origin for
which
> > the result is valid) -> observer base class gets (sink list, origin for
which
> > the result is valid), finds the origins do NOT match, and returns [] to
> > observer.
> 
> Ok, I've started on this.  If we need it in by tomorrow I will work through
the night.  However, I want to be clarify that the following APIs WILL need to
be updated according to the scenarios described by Derek.  The statement "You
will only need to extend the cache to also key by origin." is simply not not
true.
> 
> StartObservingMediaSinks will need origin added to the parameter list
> StopObservingMediaSinks will need origin added to the parameter list
> OnSinksReceived will need origin added to the parameter list
> 
> Internally, the sinks_query cache will be updated to key off a MediaSource
(which now has an origin associated with it).

There's no need to rush it.   If we decide it is a must have for M50 we will
help get the patch merged after it lands.

[matt.boetger, 2016-03-02 22:28:44]

Patchset #4 (id:140001) has been deleted

[matt.boetger, 2016-03-03 05:40:52]

Patchset #4 (id:160001) has been deleted

[matt.boetger, 2016-03-03 05:41:03]

Patchset #3 (id:120001) has been deleted

[matt.boetger, 2016-03-03 05:42:22]

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

https://codereview.chromium.org/1693963003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:333:
base::ScopedPtrHashMap<MediaSource::Id, scoped_ptr<MediaSinksQuery>>
On 2016/02/16 20:54:18, imcheng wrote:
> since queries with same source and different origin could return different
> results, we should make origin part of the map key as well.

Done.

[mark a. foltz, 2016-03-03 22:58:37]

Please update the BUG= in the patch description when you get a chance.

Overall looks good, mostly minor comments about constants used in tests. I will
take a final look after Derek's approval.

It would be good to clarify whether the origin is a mandatory parameter in the
MR and check for  an empty/invalid origin.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:218: // |origin| can be used to
optionally verify that the origin is
I would say:
|origin| is the origin of the client that is requesting media sinks.
It may be used to verify that |origin| is authorized to receive sinks for
|media_source|.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:222: // Stops querying sinks for
|media_source|.
And:
|origin| is the origin of the client that started the query.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:488: 
Set an origin on media_source2?

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:548:
base::RunLoop run_loop3;
I want to clean up these test cases not to manage multiple run loops, it creates
code that is hard to maintain.

Can you try:
- Creating one run loop here
- Removing the code that calls Quit()
- Call RunUntilIdle at the end of the test to run all pending tasks

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_source.h (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:30: std::string GetOrigin() const;
origin() for simple getters

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:33: void SetOrigin(std::string
origin);
set_origin() for simple setters

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_source_helper_unittest.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source_helper_unittest.cc:18:
IsMirroringMediaSource(MediaSourceForPresentationUrl("http://url",
Nit: Please use valid looking URLs and origins, like

"http://example.com/presentation.html", "http://example.com"

It makes the test easier to understand.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_media_sinks_observer.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/presentation_media_sinks_observer.cc:31: if
(!incomingSource.Equals(source()))
Derek: Should this be a DCHECK()?

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:88:
std::string presentation_url1("http://foo.fakeUrl");
Similar comment about URLs/origins in tests.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.cc:34: if
(!incomingSource.Equals(source())) {
Similar comment about DCHECK.  It seems like the observer is owned by the frame
which should not change origins?

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:42: //      
MediaSourceForPresentationUrl("http://google.com"),
Make the presentation URL different from the origin, for clarity.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:45: //      
MediaSourceForTab(123), GURL(""));
When would a sinks query not pass an origin?

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
(right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:98:
"http://fooUrl.com"));
url != origin here and below

https://codereview.chromium.org/1693963003/diff/180001/chrome/test/media_rout...
File chrome/test/media_router/test_media_sinks_observer.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/test/media_rout...
chrome/test/media_router/test_media_sinks_observer.cc:25: if
(!incomingSource.Equals(source())) {
DCHECK?

https://codereview.chromium.org/1693963003/diff/180001/extensions/renderer/re...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1693963003/diff/180001/extensions/renderer/re...
extensions/renderer/resources/media_router_bindings.js:272:
this.service_.onSinksReceived(sourceUrn, origin, sinks.map(sinkToMojo_));
How will this handle an extension that does not use the new API?

Rather than sticking an optional argument on the end, I would use a type check
on the second argument to determine which API is in use.

[matt.boetger, 2016-03-04 00:18:52]

Patchset #4 (id:200001) has been deleted

[matt.boetger, 2016-03-04 00:22:11]

I'm PTO Friday - Monday.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:218: // |origin| can be used to
optionally verify that the origin is
On 2016/03/03 22:58:36, mark a. foltz wrote:
> I would say:
> |origin| is the origin of the client that is requesting media sinks.
> It may be used to verify that |origin| is authorized to receive sinks for
> |media_source|.
>  

This is the second time you have suggested a change to this.  I will change it
this time, but any future change of your mind should require another CL to
prevent this from continuing ad nauseam.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router.mojom:222: // Stops querying sinks for
|media_source|.
On 2016/03/03 22:58:36, mark a. foltz wrote:
> And:
> |origin| is the origin of the client that started the query.

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:488: 
On 2016/03/03 22:58:36, mark a. foltz wrote:
> Set an origin on media_source2?

Wanted to test not setting an origin for this one.  Should default to the empty
string tested below.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:548:
base::RunLoop run_loop3;
On 2016/03/03 22:58:36, mark a. foltz wrote:
> I want to clean up these test cases not to manage multiple run loops, it
creates
> code that is hard to maintain.
> 
> Can you try:
> - Creating one run loop here
> - Removing the code that calls Quit()
> - Call RunUntilIdle at the end of the test to run all pending tasks
> 

Perhaps that clean up could be in a different CL since there are multiple
methods that use multiple loops in this class and that cleanup has nothing to do
with this code change.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_source.h (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:30: std::string GetOrigin() const;
On 2016/03/03 22:58:36, mark a. foltz wrote:
> origin() for simple getters

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:33: void SetOrigin(std::string
origin);
On 2016/03/03 22:58:36, mark a. foltz wrote:
> set_origin() for simple setters

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/media_source_helper_unittest.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/media_source_helper_unittest.cc:18:
IsMirroringMediaSource(MediaSourceForPresentationUrl("http://url",
On 2016/03/03 22:58:36, mark a. foltz wrote:
> Nit: Please use valid looking URLs and origins, like
> 
> "http://example.com/presentation.html", "http://example.com"
> 
> It makes the test easier to understand.

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_media_sinks_observer.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/presentation_media_sinks_observer.cc:31: if
(!incomingSource.Equals(source()))
On 2016/03/03 22:58:36, mark a. foltz wrote:
> Derek: Should this be a DCHECK()?

This method WILL be called with other sources unless we don't want to use the
FOR_EACH_OBSERVER macros.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:88:
std::string presentation_url1("http://foo.fakeUrl");
On 2016/03/03 22:58:36, mark a. foltz wrote:
> Similar comment about URLs/origins in tests.

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.cc:34: if
(!incomingSource.Equals(source())) {
On 2016/03/03 22:58:37, mark a. foltz wrote:
> Similar comment about DCHECK.  It seems like the observer is owned by the
frame
> which should not change origins?

Similar answer.  These will be called.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:42: //      
MediaSourceForPresentationUrl("http://google.com"),
On 2016/03/03 22:58:37, mark a. foltz wrote:
> Make the presentation URL different from the origin, for clarity.

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:45: //      
MediaSourceForTab(123), GURL(""));
On 2016/03/03 22:58:37, mark a. foltz wrote:
> When would a sinks query not pass an origin?

For tabs.

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc
(right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager_unittest.cc:98:
"http://fooUrl.com"));
On 2016/03/03 22:58:37, mark a. foltz wrote:
> url != origin here and below

Done.

https://codereview.chromium.org/1693963003/diff/180001/chrome/test/media_rout...
File chrome/test/media_router/test_media_sinks_observer.cc (right):

https://codereview.chromium.org/1693963003/diff/180001/chrome/test/media_rout...
chrome/test/media_router/test_media_sinks_observer.cc:25: if
(!incomingSource.Equals(source())) {
On 2016/03/03 22:58:37, mark a. foltz wrote:
> DCHECK?

Same answer.

https://codereview.chromium.org/1693963003/diff/180001/extensions/renderer/re...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1693963003/diff/180001/extensions/renderer/re...
extensions/renderer/resources/media_router_bindings.js:272:
this.service_.onSinksReceived(sourceUrn, origin, sinks.map(sinkToMojo_));
On 2016/03/03 22:58:37, mark a. foltz wrote:
> How will this handle an extension that does not use the new API?
> 
> Rather than sticking an optional argument on the end, I would use a type check
> on the second argument to determine which API is in use.

Done.