[MediaRouter] The minimal change to make everything build on Android.

[MediaRouter] The minimal change to make everything build on Android.

BUG=412331
NOTRY=true
NOTREECHECKS=true

Committed: https://crrev.com/6f828083bd41d61e23a6ebe89288a9b3126ae586
Cr-Commit-Position: refs/heads/master@{#339708}

[whywhat, 2015-07-09 21:12:59]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 21:14:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228863005/1

[commit-bot: I haz the power, 2015-07-09 21:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 21:27:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

(exceeded global retry quota)

[whywhat, 2015-07-09 21:48:05]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 21:49:38]

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

[whywhat, 2015-07-09 21:52:36]

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

[whywhat, 2015-07-09 21:52:37]

I'm splitting my WIP patch you reviewed a few weeks ago into smaller chunks
(incorporating the comments if possible). This is the first minimal change that
allows enable_media_router to build on Android. I added
MediaRouterDialogController and MediaRouterFactory to allow Android specific
implementations in the future patches.

[commit-bot: I haz the power, 2015-07-09 22:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-09 22:09:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[whywhat, 2015-07-13 13:04:39]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-13 13:04:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228863005/120001

[commit-bot: I haz the power, 2015-07-13 13:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 13:06:36]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

(exceeded global retry quota)

[whywhat, 2015-07-13 17:48:53]

Rebased

[whywhat, 2015-07-13 17:49:06]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-13 17:49:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228863005/140001

[commit-bot: I haz the power, 2015-07-13 17:54:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-13 17:54:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

(exceeded global retry quota)

[whywhat, 2015-07-13 18:45:47]

Typo in a chrome/test/BUILD.gn

[imcheng, 2015-07-13 22:04:15]

I wanted to see if we can extract a base abstract class for
MediaRouterDialogController. That way we can share some platform-independent
logic for the two implementations, particularly with the handling of
presentation API logic.

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:24:
~MediaRouterDialogController() = default;
Does this need to be virtual?

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:29: static
MediaRouterDialogController* GetOrCreateForWebContents(
I see that for MediaRouterFactory we return either the desktop or Android
version with #ifdefs. Can we do the same for MRDC so we don't have multiple
definitions of this function across files?

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:35: // Returns
WebContents for the media router dialog if a dialog was created.
s/WebContents for the media router dialog/true ? Similarly for below

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_factory.cc (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_factory.cc:35: return
static_cast<MediaRouterMojoImpl*>(
Is the static_cast still needed?

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_factory.cc:45: // MediaRouter depends
on ProcessManager.
Maybe clarify that this is true for desktop version only?

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/media_router/media_router_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_browsertest.cc:60:
IN_PROC_BROWSER_TEST_F(MediaRouterDialogControllerBrowserTest, ShowDialog) {
Looks like we should rename the test class and file name.

https://codereview.chromium.org/1228863005/diff/160001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/chrome_browser....
chrome/chrome_browser.gypi:590:
'browser/media/android/router/media_router_dialog_controller_android.cc',
Curious why these and many other android files are here instead of
chrome_browser_android_sources below?

[whywhat, 2015-07-15 10:15:08]

Rebase

[whywhat, 2015-07-15 12:41:05]

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:24:
~MediaRouterDialogController() = default;
On 2015/07/13 at 22:04:14, imcheng1 wrote:
> Does this need to be virtual?

Done.

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:29: static
MediaRouterDialogController* GetOrCreateForWebContents(
On 2015/07/13 at 22:04:14, imcheng1 wrote:
> I see that for MediaRouterFactory we return either the desktop or Android
version with #ifdefs. Can we do the same for MRDC so we don't have multiple
definitions of this function across files?

Are you saying you prefer a clear ifdef in one file to conditional linking (by
including one or another cc file in the build)?
I'll look into that.

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:35: // Returns
WebContents for the media router dialog if a dialog was created.
On 2015/07/13 at 22:04:14, imcheng1 wrote:
> s/WebContents for the media router dialog/true ? Similarly for below

Done.

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_factory.cc (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_factory.cc:35: return
static_cast<MediaRouterMojoImpl*>(
On 2015/07/13 at 22:04:14, imcheng1 wrote:
> Is the static_cast still needed?

Don't think so. Removed.

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_factory.cc:45: // MediaRouter depends
on ProcessManager.
On 2015/07/13 at 22:04:14, imcheng1 wrote:
> Maybe clarify that this is true for desktop version only?

Done.

https://codereview.chromium.org/1228863005/diff/160001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/chrome_browser....
chrome/chrome_browser.gypi:590:
'browser/media/android/router/media_router_dialog_controller_android.cc',
On 2015/07/13 at 22:04:15, imcheng1 wrote:
> Curious why these and many other android files are here instead of
chrome_browser_android_sources below?

I don't know for sure. Generally, files are filtered out by the _{platform}
suffix (e.g. _android) or directory (e.g. /android/). I assume these files are
listed together with everything else for simplicity.
chrome_browser_android_sources is probably for a few files that don't have
android in the path for some reason but are intended to be android only. I'd say
you need to follow up with stuartmorgan@chromium.org to get more details :)

[whywhat, 2015-07-15 12:41:46]

Addressed minor comments

[whywhat, 2015-07-15 15:25:48]

Added a stub MediaRouterAndroid for tests

[whywhat, 2015-07-15 16:27:27]

Fixed test_support compile issue on ChromeOS

[imcheng, 2015-07-16 18:28:20]

lgtm % a few comments

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1228863005/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:29: static
MediaRouterDialogController* GetOrCreateForWebContents(
On 2015/07/15 12:41:04, whywhat wrote:
> On 2015/07/13 at 22:04:14, imcheng1 wrote:
> > I see that for MediaRouterFactory we return either the desktop or Android
> version with #ifdefs. Can we do the same for MRDC so we don't have multiple
> definitions of this function across files?
> 
> Are you saying you prefer a clear ifdef in one file to conditional linking (by
> including one or another cc file in the build)?
> I'll look into that.

Yeah. I was thinking we can put the definition in
media_router_dialog_controller.cc.

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:17: class
MediaRouterAndroid: public MediaRouter {
nit: space before :

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:44: explicit
MediaRouterAndroid(content::BrowserContext* context);
Unused argument?

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:40: // posting messages or closing).
Is a KeyedService.
This comment is redundant as the reader can easily infer that from the line
below.

[whywhat, 2015-07-16 19:57:01]

Moved MRDC::GetOrCreateForWebContents to its own .cc file

[whywhat, 2015-07-16 20:17:17]

avayvod@chromium.org changed reviewers:
+ jam@chromium.org

[whywhat, 2015-07-16 20:17:18]

+jam for owners approval

Especially, build files in build/, chrome/test and chrome/browser/ui/
(everything media_router and media/router should be covered by Derek)

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:17: class
MediaRouterAndroid: public MediaRouter {
On 2015/07/16 at 18:28:19, imcheng1 wrote:
> nit: space before :

Done.

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:44: explicit
MediaRouterAndroid(content::BrowserContext* context);
On 2015/07/16 at 18:28:19, imcheng1 wrote:
> Unused argument?

Removed the name.
It seems easier to ignore it here than to conditionally mark it as unused when
creating it for BrowserContext in the factory.

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1228863005/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:40: // posting messages or closing).
Is a KeyedService.
On 2015/07/16 at 18:28:19, imcheng1 wrote:
> This comment is redundant as the reader can easily infer that from the line
below.

Done.

[jam, 2015-07-16 22:36:13]

On 2015/07/16 20:17:18, whywhat wrote:
> +jam for owners approval

see comment in chrome/owners:
# This OWNERS list is a last resort. Please prefer to use more specific OWNERS
# where possible."

so please pick owners from  
chrome/browser/media/OWNERS
chrome/browser/ui/webui/media_router/OWNERS
chrome/test/media_router/OWNERS

which cover most of the interesting bits.

[whywhat, 2015-07-16 22:50:11]

On 2015/07/16 at 22:36:13, jam wrote:
> On 2015/07/16 20:17:18, whywhat wrote:
> > +jam for owners approval
> 
> see comment in chrome/owners:
> # This OWNERS list is a last resort. Please prefer to use more specific OWNERS
> # where possible."
> 
> so please pick owners from  
> chrome/browser/media/OWNERS
> chrome/browser/ui/webui/media_router/OWNERS
> chrome/test/media_router/OWNERS
> 
> which cover most of the interesting bits.

You still qualify for the build/ and chrome/ and chrome/test bits but I guess
you want the specific OWNERS to lgtm first since your lgtm would give me an
ability to land the cl.

+apacible for webui/
+xhwang for chrome/browser/media/android/

chrome/test/media_router is covered by Derek I believe.

[whywhat, 2015-07-16 22:50:37]

avayvod@chromium.org changed reviewers:
+ apacible@chromium.org, xhwang@chromium.org

[xhwang, 2015-07-17 23:51:04]

chrome/browser/media/android/ lgtm

[apacible, 2015-07-20 14:10:32]

chrome/browser/ui/webui lgtm + couple of nits

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:104:
// static
nit: No indentation

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// MediaRouterDialogController implementation.
nit: for new/updated c/b/ui code, we now do this instead:
// SuperClass:

[whywhat, 2015-07-20 14:27:48]

jam: your turn :)

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:104:
// static
On 2015/07/20 at 14:10:32, apacible wrote:
> nit: No indentation

Done.

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// MediaRouterDialogController implementation.
On 2015/07/20 at 14:10:32, apacible wrote:
> nit: for new/updated c/b/ui code, we now do this instead:
> // SuperClass:

I didn't find any examples in the code base yet. Also the point is not to state
that this class is a subclass of MediaRouterDialogController (it's evident from
the base classes list, isn't it?), but that this is the desktop implementation
vs. the Android one.

[apacible, 2015-07-20 14:46:40]

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// MediaRouterDialogController implementation.
On 2015/07/20 14:27:47, whywhat wrote:
> On 2015/07/20 at 14:10:32, apacible wrote:
> > nit: for new/updated c/b/ui code, we now do this instead:
> > // SuperClass:
> 
> I didn't find any examples in the code base yet. Also the point is not to
state
> that this class is a subclass of MediaRouterDialogController (it's evident
from
> the base classes list, isn't it?), but that this is the desktop implementation
> vs. the Android one.

Hm, I got this feedback last week on a CL (which I'll land later today) for the
same impl comment. I can check with my reviewer to see how recent of a change
that was, if you'd like.

[jam, 2015-07-20 16:11:25]

On 2015/07/16 22:50:11, whywhat wrote:
> On 2015/07/16 at 22:36:13, jam wrote:
> > On 2015/07/16 20:17:18, whywhat wrote:
> > > +jam for owners approval
> > 
> > see comment in chrome/owners:
> > # This OWNERS list is a last resort. Please prefer to use more specific
OWNERS
> > # where possible."
> > 
> > so please pick owners from  
> > chrome/browser/media/OWNERS
> > chrome/browser/ui/webui/media_router/OWNERS
> > chrome/test/media_router/OWNERS
> > 
> > which cover most of the interesting bits.
> 
> You still qualify for the build/ and chrome/ and chrome/test bits but I guess
> you want the specific OWNERS to lgtm first since your lgtm would give me an
> ability to land the cl.
lgtm
> 
> +apacible for webui/
> +xhwang for chrome/browser/media/android/
> 
> chrome/test/media_router is covered by Derek I believe.

[whywhat, 2015-07-20 16:52:59]

Thanks all for the reviews!

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// MediaRouterDialogController implementation.
On 2015/07/20 at 14:46:40, apacible wrote:
> On 2015/07/20 14:27:47, whywhat wrote:
> > On 2015/07/20 at 14:10:32, apacible wrote:
> > > nit: for new/updated c/b/ui code, we now do this instead:
> > > // SuperClass:
> > 
> > I didn't find any examples in the code base yet. Also the point is not to
state
> > that this class is a subclass of MediaRouterDialogController (it's evident
from
> > the base classes list, isn't it?), but that this is the desktop
implementation
> > vs. the Android one.
> 
> Hm, I got this feedback last week on a CL (which I'll land later today) for
the same impl comment. I can check with my reviewer to see how recent of a
change that was, if you'd like.

I think I misread what line this referred to - changed.
Yes, if you could refer to any discussion or example, that'd be great!

[whywhat, 2015-07-20 16:53:07]

Fixed a comment in the derived class declaration

[whywhat, 2015-07-20 16:53:34]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-20 16:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, xhwang@chromium.org, apacible@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1228863005/#ps300001
(title: "Fixed a comment in the derived class declaration")

[commit-bot: I haz the power, 2015-07-20 16:53:47]

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

[commit-bot: I haz the power, 2015-07-20 16:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 16:56:02]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[whywhat, 2015-07-20 20:42:04]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-20 20:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
imcheng@chromium.org, jam@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1228863005/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-20 20:43:12]

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

[commit-bot: I haz the power, 2015-07-20 20:46:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 20:46:12]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[whywhat, 2015-07-20 22:10:52]

Rebase with chromium trunk

[whywhat, 2015-07-20 22:14:00]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-20 22:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
imcheng@chromium.org, jam@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1228863005/#ps340001
(title: "Rebase with chromium trunk")

[commit-bot: I haz the power, 2015-07-20 22:15:01]

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

[commit-bot: I haz the power, 2015-07-20 22:28:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 22:28:49]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[whywhat, 2015-07-21 14:11:50]

Fixed the include for MediaRouterDialogControllerImpl

[whywhat, 2015-07-21 14:16:55]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-21 14:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
imcheng@chromium.org, jam@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1228863005/#ps360001
(title: "Fixed the include for MediaRouterDialogControllerImpl")

[commit-bot: I haz the power, 2015-07-21 14:17:09]

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

[commit-bot: I haz the power, 2015-07-21 14:53:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-21 14:53:28]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[whywhat, 2015-07-21 15:07:07]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-21 15:07:26]

On 2015/07/21 at 14:53:28, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The failed test seems to be flaky on trybots.

[commit-bot: I haz the power, 2015-07-21 15:07:46]

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

[commit-bot: I haz the power, 2015-07-21 17:05:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-21 17:05:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[whywhat, 2015-07-21 18:36:14]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2015-07-21 18:36:42]

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

[commit-bot: I haz the power, 2015-07-21 18:44:15]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-07-21 18:45:54]

Patchset 19 (id:??) landed as
https://crrev.com/6f828083bd41d61e23a6ebe89288a9b3126ae586
Cr-Commit-Position: refs/heads/master@{#339708}

[apacible, 2015-07-22 04:00:09]

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// SuperClass:
Oops, some miscommunication for this line -- 

This should be:
// MediaRouterDialogController:

... and SuperClass is just a placeholder. Does that make more sense? :)

I saw you sent out another CL touching this file -- feel free to update there.
Thanks!

[mark a. foltz, 2015-07-22 21:16:39]

I noticed a few things in the build files but nothing urgent.  There are a few
more GN/GYP files that refer to the enable_media_router build flag.  You might
do a code search and make sure there aren't any further updates needed.

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (left):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:49:
media_router::MediaRouterDialogController* GetMediaRouterDialogController();
Why does this API need to be changed to use the implementation type?

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:25: #include
"chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
Was this change intentional?

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:586:
'browser/media/android/router/media_router_android.cc',
Shouldn't these go in an Android-specific source list?

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:1481:
'browser/ui/webui/media_router/media_router_dialog_controller_impl.cc',
These are only included on desktop platforms, correct?

[whywhat, 2015-07-22 22:55:01]

Thanks! I'll take a look at the build files again and will try to follow up to
some comments in the follow-up patch.

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (left):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:49:
media_router::MediaRouterDialogController* GetMediaRouterDialogController();
On 2015/07/22 at 21:16:39, mark a. foltz wrote:
> Why does this API need to be changed to use the implementation type?

Well, I assumed that webui/ classes can use the webui specific Impl class. That
way webui/ specific methods can be added to the Impl class and accessed from
other webui/ classes. These methods (e.g. about dialog's WebContents) won't even
make sense on Android so I don't want to drag them out into the base class.

Also, this made the impact on the webui/ code minimal.

However, it seems to only use Show and Close MediaRouterDialog methods so I
could use the base class. I'll look into it in my follow-up change.

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h
(right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:26:
// SuperClass:
Yup, now it makes much more sense and I changed it in the follow up CL.

On 2015/07/22 at 04:00:08, apacible wrote:
> Oops, some miscommunication for this line -- 
> 
> This should be:
> // MediaRouterDialogController:
> 
> ... and SuperClass is just a placeholder. Does that make more sense? :)
> 
> I saw you sent out another CL touching this file -- feel free to update there.
Thanks!

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:25: #include
"chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
On 2015/07/22 at 21:16:39, mark a. foltz wrote:
> Was this change intentional?

Yes, to avoid breaking the file. It seems to be unused in the file though.

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:586:
'browser/media/android/router/media_router_android.cc',
On 2015/07/22 at 21:16:39, mark a. foltz wrote:
> Shouldn't these go in an Android-specific source list?

There's one historical list for certain files. Most files are simply under the
/android/ dir and with _android. postfix - these are excluded from non-Android
build automatically by gyp/gn.

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1228863005/diff/360001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:1481:
'browser/ui/webui/media_router/media_router_dialog_controller_impl.cc',
On 2015/07/22 at 21:16:39, mark a. foltz wrote:
> These are only included on desktop platforms, correct?

Yes. WebUI doesn't compile on Android.

[mark a. foltz, 2015-07-22 23:05:55]

lgtm

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (left):

https://codereview.chromium.org/1228863005/diff/360001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:49:
media_router::MediaRouterDialogController* GetMediaRouterDialogController();
On 2015/07/22 at 22:55:01, whywhat wrote:
> On 2015/07/22 at 21:16:39, mark a. foltz wrote:
> > Why does this API need to be changed to use the implementation type?
> 
> Well, I assumed that webui/ classes can use the webui specific Impl class.
That way webui/ specific methods can be added to the Impl class and accessed
from other webui/ classes. These methods (e.g. about dialog's WebContents) won't
even make sense on Android so I don't want to drag them out into the base class.
> 
> Also, this made the impact on the webui/ code minimal.
> 
> However, it seems to only use Show and Close MediaRouterDialog methods so I
could use the base class. I'll look into it in my follow-up change.

Okay, if the methods make sense in the MediaRouterDialog class then I would
prefer you move them up and revert this change.  Otherwise it's not a big deal.

In general I prefer APIs that return interface types instead of implementation
types, because exposing implementation types makes unit testing and future
refactoring harder.