Adds QueryResultManager support class for Media Router Dialog WebUI

QueryResultManager allows the Media Router dialog to query for sets of media sinks compatible with specific media sources.  Each media source is in a correspondence to a cast mode (a specific action/button the user can take to
initiate casting).

NOTE: The mock_media_router.{h,cc} files will be put later into a proper test_support target for the media_router.

BUG=464216
Committed: https://crrev.com/34f8e9ed426fe7538f61215e234abdf7f238c001
Cr-Commit-Position: refs/heads/master@{#330014}

[mark a. foltz, 2015-05-07 20:35:10]

mfoltz@chromium.org changed reviewers:
+ bauerb@chromium.org, imcheng@chromium.org, sky@chromium.org

[mark a. foltz, 2015-05-07 20:35:11]

bauerb: Please review query_result_manager.{h,cc} and unit test
imcheng@: Please review everything
sky@: For BUILD.gn, but you can wait until I land a cleanup of the media_router
gyp/GN files.

[sky, 2015-05-07 23:14:30]

https://codereview.chromium.org/1131913003/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:217: "../browser/media/router/mock_media_router.cc",
Why do these need to be here and not closer to where you're using it?

[mark a. foltz, 2015-05-08 00:13:16]

https://codereview.chromium.org/1131913003/diff/60001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:217: "../browser/media/router/mock_media_router.cc",
On 2015/05/07 23:14:30, sky wrote:
> Why do these need to be here and not closer to where you're using it?

I have another patch which will move these to
chrome/browser/media/router/BUILD.gn.  Once that lands I'll update this issue,
your review might no longer be necessary :)

https://codereview.chromium.org/1136623003/

[Bernhard Bauer, 2015-05-08 08:34:34]

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.cc (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:36: if
(source.Empty()) {
If you have a DCHECK for the inverse condition right before, you are saying that
the condition can not happen, which means you shouldn't have code that checks
for it (see also
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:49:
CHECK(result.second);
Unless there is a specific reason (like avoiding security issues due to memory
corruption), prefer DCHECKs for internal consistency checks.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:80:
base::hash_set<MediaSinkId> result_sink_ids;
You could call the constructor with result.begin() and result.end(), which would
have the same effect.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:92:
all_sinks_.erase(it++);
Whoa... is deleting elements from a hash set while iterating over it guaranteed
to work? Couldn't it happen that the hash table is resized or something?

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:102: all_sinks_it =
all_sinks_.insert(
insert() will actually already do pretty much what you are doing here: It only
inserts the new element if an element with the key doesn't already exist, so at
the cost of a potentially unnecessary stack allocation (which I think is cheaper
regardless) you could get rid of the find() above.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:105:
all_sinks_it->second.sink = sink;
Is this necessary if you have just inserted a new element?

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:58: // Not
thread-safe.
Nit: Thread safety (or lack thereof) comes in so many different flavors, it
might be more useful to specifically note the threading constraints. In this
case, I think the class should always be used on the same thread?

Alternatively, you could inherit from base::NonThreadSafe, which will give you a
protected CalledOnValidThread() method and basically express the same thing.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:61: class Observer {
Optional: Instead of a single-method Observer interface, you could also use
base::CallbackList<>.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:106: class
CastModeMediaSinksObserver : public MediaSinksObserver {
Can you define this class in the .cc file and just forward-declare it here?

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:149:
std::map<MediaCastMode, linked_ptr<CastModeMediaSinksObserver>>
Can you avoid using linked_ptr? Its use is somwhat discouraged. For maps with
owwning semantics, you can use base::ScopedPtrHashMap.

[mark a. foltz, 2015-05-12 06:23:15]

PTAL.

Note: I will wait for our gyp/GN changes to land and rebase before committing,
which will make the GYP/GN changes simpler.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.cc (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:36: if
(source.Empty()) {
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> If you have a DCHECK for the inverse condition right before, you are saying
that
> the condition can not happen, which means you shouldn't have code that checks
> for it (see also
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).

Yeah, this is a bug but not a terminal condition.  Removed DCHECK.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:49:
CHECK(result.second);
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Unless there is a specific reason (like avoiding security issues due to memory
> corruption), prefer DCHECKs for internal consistency checks.

Done.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:80:
base::hash_set<MediaSinkId> result_sink_ids;
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> You could call the constructor with result.begin() and result.end(), which
would
> have the same effect.

We are inserting sink.id() not just the sinks.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:92:
all_sinks_.erase(it++);
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Whoa... is deleting elements from a hash set while iterating over it
guaranteed
> to work? Couldn't it happen that the hash table is resized or something?

This is deleting from the all_sinks_ std::map, not the hash_set.  From what I
can tell erasure invalidates iterators that point to the erased element, so the
postincrement behavior keeps the iterator valid.

I did convert the hash_set to an unordered_set as hash_set is obsolete.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:102: all_sinks_it =
all_sinks_.insert(
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> insert() will actually already do pretty much what you are doing here: It only
> inserts the new element if an element with the key doesn't already exist, so
at
> the cost of a potentially unnecessary stack allocation (which I think is
cheaper
> regardless) you could get rid of the find() above.

Done.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:105:
all_sinks_it->second.sink = sink;
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Is this necessary if you have just inserted a new element?

Done.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:58: // Not
thread-safe.
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Nit: Thread safety (or lack thereof) comes in so many different flavors, it
> might be more useful to specifically note the threading constraints. In this
> case, I think the class should always be used on the same thread?

That's correct. I clarified the comment.

> Alternatively, you could inherit from base::NonThreadSafe, which will give you
a
> protected CalledOnValidThread() method and basically express the same thing.

The class uses a base::ThreadChecker to verify thread usage, this is the pattern
we have been using in media router related classes.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:61: class Observer {
On 2015/05/08 08:34:34, Bernhard Bauer wrote:
> Optional: Instead of a single-method Observer interface, you could also use
> base::CallbackList<>.

Thanks I was not aware of that.  It looks like CallbackList is good for classes
that have several different subscriber implementations and want very loose
coupling between the two.  In this case we have one implementation and the
Observer interface makes the collaboration obvious via public inheritance.

My preference is to keep the current pattern; if you feel strongly I would
appreciate reasons why CallbackList is preferable.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:106: class
CastModeMediaSinksObserver : public MediaSinksObserver {
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Can you define this class in the .cc file and just forward-declare it here?

Done.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:149:
std::map<MediaCastMode, linked_ptr<CastModeMediaSinksObserver>>
On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> Can you avoid using linked_ptr? Its use is somwhat discouraged. For maps with
> owwning semantics, you can use base::ScopedPtrHashMap.

Is there a "scoped" replacement for std::map?  I did not see any landed in
crbug.com/137767 or in base/memory.

[Bernhard Bauer, 2015-05-12 09:34:34]

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.cc (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:80:
base::hash_set<MediaSinkId> result_sink_ids;
On 2015/05/12 06:23:14, mark a. foltz wrote:
> On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> > You could call the constructor with result.begin() and result.end(), which
> would
> > have the same effect.
> 
> We are inserting sink.id() not just the sinks.

Oh, right.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.cc:92:
all_sinks_.erase(it++);
On 2015/05/12 06:23:14, mark a. foltz wrote:
> On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> > Whoa... is deleting elements from a hash set while iterating over it
> guaranteed
> > to work? Couldn't it happen that the hash table is resized or something?
> 
> This is deleting from the all_sinks_ std::map, not the hash_set.  From what I
> can tell erasure invalidates iterators that point to the erased element, so
the
> postincrement behavior keeps the iterator valid.

Oh, you're right, you iterate over the map. That's totally fine, yeah.

> I did convert the hash_set to an unordered_set as hash_set is obsolete.

Oh, does this work on all platforms? I thought we didn't support C++11 *library*
features everywhere yet.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:61: class Observer {
On 2015/05/12 06:23:15, mark a. foltz wrote:
> On 2015/05/08 08:34:34, Bernhard Bauer wrote:
> > Optional: Instead of a single-method Observer interface, you could also use
> > base::CallbackList<>.
> 
> Thanks I was not aware of that.  It looks like CallbackList is good for
classes
> that have several different subscriber implementations and want very loose
> coupling between the two.  In this case we have one implementation and the
> Observer interface makes the collaboration obvious via public inheritance.
> 
> My preference is to keep the current pattern; if you feel strongly I would
> appreciate reasons why CallbackList is preferable.

I definitely don't feel strongly. Personally, I just prefer Callback over a
single-method interface, as they are basically isomorphic, and CallbackList lets
you do registration and unregistration with a single method, so in total
Callback + CallbackList is less code.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:149:
std::map<MediaCastMode, linked_ptr<CastModeMediaSinksObserver>>
On 2015/05/12 06:23:15, mark a. foltz wrote:
> On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> > Can you avoid using linked_ptr? Its use is somwhat discouraged. For maps
with
> > owwning semantics, you can use base::ScopedPtrHashMap.
> 
> Is there a "scoped" replacement for std::map?  I did not see any landed in
> crbug.com/137767 or in base/memory.

No, there's only one for hash_map so far (for whatever reason).

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

https://codereview.chromium.org/1131913003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.cc:145:
result.first->second.sink = sink;
Thinking about this a bit more, even if we have not inserted a new entry, this
is basically guaranteed by the invariant, right? Should we just DCHECK it?

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

https://codereview.chromium.org/1131913003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:100: friend class
CastModeMediaSinksObserver;
You could still keep this class inside of QueryResultManager, just
forward-declare it (that way you avoid any accidental naming collisions, as
unlikely as they are).

[mark a. foltz, 2015-05-12 19:05:26]

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/query_result_manager.h (right):

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:61: class Observer {
On 2015/05/12 09:34:34, Bernhard Bauer wrote:
> On 2015/05/12 06:23:15, mark a. foltz wrote:
> > On 2015/05/08 08:34:34, Bernhard Bauer wrote:
> > > Optional: Instead of a single-method Observer interface, you could also
use
> > > base::CallbackList<>.
> > 
> > Thanks I was not aware of that.  It looks like CallbackList is good for
> classes
> > that have several different subscriber implementations and want very loose
> > coupling between the two.  In this case we have one implementation and the
> > Observer interface makes the collaboration obvious via public inheritance.
> > 
> > My preference is to keep the current pattern; if you feel strongly I would
> > appreciate reasons why CallbackList is preferable.
> 
> I definitely don't feel strongly. Personally, I just prefer Callback over a
> single-method interface, as they are basically isomorphic, and CallbackList
lets
> you do registration and unregistration with a single method, so in total
> Callback + CallbackList is less code.

Acknowledged.

https://codereview.chromium.org/1131913003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/query_result_manager.h:149:
std::map<MediaCastMode, linked_ptr<CastModeMediaSinksObserver>>
On 2015/05/12 09:34:34, Bernhard Bauer wrote:
> On 2015/05/12 06:23:15, mark a. foltz wrote:
> > On 2015/05/08 08:34:33, Bernhard Bauer wrote:
> > > Can you avoid using linked_ptr? Its use is somwhat discouraged. For maps
> with
> > > owwning semantics, you can use base::ScopedPtrHashMap.
> > 
> > Is there a "scoped" replacement for std::map?  I did not see any landed in
> > crbug.com/137767 or in base/memory.
> 
> No, there's only one for hash_map so far (for whatever reason).

The ScopedPtrHashMap is really a different API from std::map and would require
extra code to use (like defining hash functions).  I'm happy to make the change
when it makes the code simpler to do so.  Added a TODO.

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

https://codereview.chromium.org/1131913003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.cc:145:
result.first->second.sink = sink;
On 2015/05/12 09:34:34, Bernhard Bauer wrote:
> Thinking about this a bit more, even if we have not inserted a new entry, this
> is basically guaranteed by the invariant, right? Should we just DCHECK it?

If there's an existing sink in all_sinks_ it's not guaranteed to be identical to
|sink|.  For example the user might have renamed their Chromecast between calls
to UpdateWithSinksQueryResult().  It's a bit of a corner case but I'd rather
ensure we have the latest version.

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

https://codereview.chromium.org/1131913003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.h:100: friend class
CastModeMediaSinksObserver;
On 2015/05/12 09:34:34, Bernhard Bauer wrote:
> You could still keep this class inside of QueryResultManager, just
> forward-declare it (that way you avoid any accidental naming collisions, as
> unlikely as they are).

Done.

[Bernhard Bauer, 2015-05-13 08:04:25]

LGTM!

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

https://codereview.chromium.org/1131913003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/query_result_manager.cc:145:
result.first->second.sink = sink;
On 2015/05/12 19:05:26, mark a. foltz wrote:
> On 2015/05/12 09:34:34, Bernhard Bauer wrote:
> > Thinking about this a bit more, even if we have not inserted a new entry,
this
> > is basically guaranteed by the invariant, right? Should we just DCHECK it?
> 
> If there's an existing sink in all_sinks_ it's not guaranteed to be identical
to
> |sink|.  For example the user might have renamed their Chromecast between
calls
> to UpdateWithSinksQueryResult().  It's a bit of a corner case but I'd rather
> ensure we have the latest version.

Okay, makes sense.

[mark a. foltz, 2015-05-15 00:17:26]

sky@:  I fixed chrome_tests_unit.gypi with a proper target for the mock files
needed to land this.  PTAL

[sky, 2015-05-15 00:58:08]

LGTM

[mark a. foltz, 2015-05-15 01:25:38]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-05-15 01:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1131913003/#ps160001
(title: "Remove unnecessary #include")

[commit-bot: I haz the power, 2015-05-15 01:27:40]

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

[commit-bot: I haz the power, 2015-05-15 01:33:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-15 01:34:07]

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