[Media Router Tests] Add tests for MediaRouterFactory.

[Media Router Tests] Add tests for MediaRouterFactory.

Committed: https://crrev.com/4f04fcb749ed895a6d5baa72a0ff164fa27589e6
Cr-Commit-Position: refs/heads/master@{#389721}

[imcheng, 2016-04-12 09:50:07]

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

[imcheng, 2016-04-12 09:50:08]

PTAL, thanks.

[mark a. foltz, 2016-04-12 17:21:23]

Thanks!

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_factory.h (right):

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:34:
FRIEND_TEST_ALL_PREFIXES(MediaRouterFactoryTest,
Can be removed if the static fn below is also removed.

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:41: static
MediaRouterFactory* GetMediaRouterFactory();
Is this necessary or can you just new up a MediaRouterFactory in your test?

[imcheng, 2016-04-12 17:56:33]

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_factory.h (right):

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:34:
FRIEND_TEST_ALL_PREFIXES(MediaRouterFactoryTest,
On 2016/04/12 17:21:23, mark a. foltz wrote:
> Can be removed if the static fn below is also removed.

Acknowledged.

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:41: static
MediaRouterFactory* GetMediaRouterFactory();
On 2016/04/12 17:21:23, mark a. foltz wrote:
> Is this necessary or can you just new up a MediaRouterFactory in your test?

It's possible to new a MediaRouterFactory, but I will not be able to use it to
create a MediaRouter instance with the current set of public APIs. The only way
is to use the public static method which uses the singleton instance of the
factory.

[mark a. foltz, 2016-04-12 20:40:03]

On 2016/04/12 at 17:56:33, imcheng wrote:
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> File chrome/browser/media/router/media_router_factory.h (right):
> 
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router_factory.h:34:
FRIEND_TEST_ALL_PREFIXES(MediaRouterFactoryTest,
> On 2016/04/12 17:21:23, mark a. foltz wrote:
> > Can be removed if the static fn below is also removed.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router_factory.h:41: static
MediaRouterFactory* GetMediaRouterFactory();
> On 2016/04/12 17:21:23, mark a. foltz wrote:
> > Is this necessary or can you just new up a MediaRouterFactory in your test?
> 
> It's possible to new a MediaRouterFactory, but I will not be able to use it to
create a MediaRouter instance with the current set of public APIs. The only way
is to use the public static method which uses the singleton instance of the
factory.

A design I've seen in the extensions system (e.g., ExtensionPrefsFactory) is
have a factory getInstance() that returns the singleton instance, then
GetApiForBrowserContext() becomes an instance method. A static convenience
method MediaRouter::Get(BrowserContext) handles the common case of using the
singleton factory and means you don't have to expose the MediaRouterFactory API
outside of the MR.

WDYT?

[imcheng, 2016-04-14 21:48:08]

On 2016/04/12 20:40:03, mark a. foltz wrote:
> On 2016/04/12 at 17:56:33, imcheng wrote:
> >
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> > File chrome/browser/media/router/media_router_factory.h (right):
> > 
> >
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router_factory.h:34:
> FRIEND_TEST_ALL_PREFIXES(MediaRouterFactoryTest,
> > On 2016/04/12 17:21:23, mark a. foltz wrote:
> > > Can be removed if the static fn below is also removed.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router_factory.h:41: static
> MediaRouterFactory* GetMediaRouterFactory();
> > On 2016/04/12 17:21:23, mark a. foltz wrote:
> > > Is this necessary or can you just new up a MediaRouterFactory in your
test?
> > 
> > It's possible to new a MediaRouterFactory, but I will not be able to use it
to
> create a MediaRouter instance with the current set of public APIs. The only
way
> is to use the public static method which uses the singleton instance of the
> factory.
> 
> A design I've seen in the extensions system (e.g., ExtensionPrefsFactory) is
> have a factory getInstance() that returns the singleton instance, then
> GetApiForBrowserContext() becomes an instance method. A static convenience
> method MediaRouter::Get(BrowserContext) handles the common case of using the
> singleton factory and means you don't have to expose the MediaRouterFactory
API
> outside of the MR.
> 
> WDYT?

Hmm, I tried changing the API as you suggested (adding a non-static method for
MediaRouterFactory and creating a MediaRouterFactory instance in test), but it
ends up being more complicated. I still need the FRIEND_TEST_ALL_PREFIXES for
the non-static method but now also "friend class MediaRouterFactory;" due to the
private destructor (since the factory is supposed to outlive the profile).  So I
am inclined to stick with the API in PS2.

[mark a. foltz, 2016-04-22 21:17:55]

lgtm with one requested change

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_factory.h (right):

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:41: static
MediaRouterFactory* GetMediaRouterFactory();
On 2016/04/12 at 17:56:33, imcheng wrote:
> On 2016/04/12 17:21:23, mark a. foltz wrote:
> > Is this necessary or can you just new up a MediaRouterFactory in your test?
> 
> It's possible to new a MediaRouterFactory, but I will not be able to use it to
create a MediaRouter instance with the current set of public APIs. The only way
is to use the public static method which uses the singleton instance of the
factory.

OK.  Please name it GetMediaRouterFactoryForTest() to make it clear it's not the
correct API for usage.

[imcheng, 2016-04-26 07:02:38]

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_factory.h (right):

https://codereview.chromium.org/1877273002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory.h:41: static
MediaRouterFactory* GetMediaRouterFactory();
On 2016/04/22 21:17:55, mark a. foltz wrote:
> On 2016/04/12 at 17:56:33, imcheng wrote:
> > On 2016/04/12 17:21:23, mark a. foltz wrote:
> > > Is this necessary or can you just new up a MediaRouterFactory in your
test?
> > 
> > It's possible to new a MediaRouterFactory, but I will not be able to use it
to
> create a MediaRouter instance with the current set of public APIs. The only
way
> is to use the public static method which uses the singleton instance of the
> factory.
> 
> OK.  Please name it GetMediaRouterFactoryForTest() to make it clear it's not
the
> correct API for usage.

Done.

[imcheng, 2016-04-26 07:02:44]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-04-26 07:02:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1877273002/#ps60001
(title: "git cl try")

[commit-bot: I haz the power, 2016-04-26 07:02:56]

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

[commit-bot: I haz the power, 2016-04-26 07:43:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-26 07:45:11]

Description was changed from

==========
[Media Router Tests] Add tests for MediaRouterFactory.
==========

to

==========
[Media Router Tests] Add tests for MediaRouterFactory.

Committed: https://crrev.com/4f04fcb749ed895a6d5baa72a0ff164fa27589e6
Cr-Commit-Position: refs/heads/master@{#389721}
==========

[commit-bot: I haz the power, 2016-04-26 07:45:12]

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

[Marc Treib, 2016-04-26 07:55:31]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1916973003/ by treib@chromium.org.

The reason for reverting is: Breaks the build:
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder/builds....

[dcheng, 2016-04-26 08:26:11]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-04-26 08:26:13]

https://codereview.chromium.org/1877273002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_factory_unittest.cc (right):

https://codereview.chromium.org/1877273002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_factory_unittest.cc:17:
scoped_ptr<KeyedService> CreateMockMediaRouter(
Please use std::unique_ptr and base::WrapUnique. scoped_ptr is being removed
very shortly (probably tomorrow)