Reorganizes Chromecast code to better reflect functional dependencies.

Reorganizes Chromecast code to better reflect functional dependencies.

Motivation:
* chromecast/service/ directory had interplay with chromecast/shell/browser
  code, since the service is all run in the browser process and mildly
  coupled (involves starting WebContents instances, etc,). Made more sense
  for chromecast/service to live in chromecast/shell/browser.
* chromecast/shell/ directory is needless hierarchy, since the entire
  chromecast/ directory represents the Chromecast content-embedder.

Highlights:
* chromecast/shell/{app,browser,common,renderer} --> chromecast/*
* chromecast/shell/browser/resources --> chromecast/app/resources
* chromecast/shell/settings --> chromecast/app/resources
* chromecast/shell/android --> chromecast/browser/android
* chromecast/metrics --> chromecast/browser/metrics
* chromecast/service --> chromecast/browser/service

R=lcwu@chromium.org,gusfernandez@chromium.org,byungchul@chromium.org
TBR=xhwang@chromium.org,asvitkine@chromium.org,thestig@chromium.org,jam@chromium.org,yfriedman@chromium.org
BUG=None

Committed: https://crrev.com/f44ffcbebf37154f23917d058c1bb6fa295887e3
Cr-Commit-Position: refs/heads/master@{#299385}

[gunsch, 2014-10-08 03:04:15]

Couple quick comments:

* The best way to review the structural changes seems to be to just look at
chromecast.gyp

* I'll look for chromecast team LG's first, but then need to expand and grab a
pile of other owners since we completely redid our DEPS

* It'd be nice to get this through quickly, since the merge conflict potential
is extreme.

[lcwu1, 2014-10-08 06:58:36]

On 2014/10/08 03:04:15, gunsch wrote:
> Couple quick comments:
> 
> * The best way to review the structural changes seems to be to just look at
> chromecast.gyp
> 
> * I'll look for chromecast team LG's first, but then need to expand and grab a
> pile of other owners since we completely redid our DEPS
> 
> * It'd be nice to get this through quickly, since the merge conflict potential
> is extreme.

I have sent out an email to chromecast team to request for comments first. I
want to make sure any concerns or objections are addressed before we move
forward with this re-structuring. Let's wait for a day or two for the feedback.

[gunsch, 2014-10-10 00:02:09]

Rebased. PTAL.

[gunsch, 2014-10-11 00:14:36]

24-hour ping

[lcwu1, 2014-10-13 01:26:35]

https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:190: 'app/cast_content_client.h',
Why do you want to place cast_content_client.[cc|h] in app/ instead of in
common/ like what chrome and content_shell do?

[gunsch, 2014-10-13 03:13:17]

https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:190: 'app/cast_content_client.h',
On 2014/10/13 01:26:35, lcwu1 wrote:
> Why do you want to place cast_content_client.[cc|h] in app/ instead of in
> common/ like what chrome and content_shell do?

That was a very conscious decision:
* chromecast/common does not currently have any dependency on content/ and I'd
like to keep it that way
* conceptually, CastContentClient seems to me to fit with the other code in
app/, such as CastMainDelegate, under the notion of "content/ implementation
that's neither browser- or renderer-specific."

[lcwu1, 2014-10-13 06:15:38]

On 2014/10/13 03:13:17, gunsch wrote:
> https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
> 
>
https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gy...
> chromecast/chromecast.gyp:190: 'app/cast_content_client.h',
> On 2014/10/13 01:26:35, lcwu1 wrote:
> > Why do you want to place cast_content_client.[cc|h] in app/ instead of in
> > common/ like what chrome and content_shell do?
> 
> That was a very conscious decision:
> * chromecast/common does not currently have any dependency on content/ and I'd
> like to keep it that way

I think in general a content embedder has (or should have) a directory structure
that matches well to content/public directory. That is, <embedder>/browser
contains code that implements content/public/browser, <embedder>/renderer
contains code that implements content/public/renderer, and <embedder>/common
contains code that implements content/public/common (where content_client.h is
located). So I think it's OK for chromecast/common to depends on
content/public/common. And in fact, all the existing content embedders (chrome,
android_webview, and content_shell) do the same. It's just so happen that we
have some other common files inside chromecast/common other than content/common
implementations. But that's fine because that's also what happens in
chrome/common.

I think consistency should be an important factor to consider here. For people
who are already familiar with chrome/ or content/shell or android_webview, if
they want to look for Chromecast's content_client implementation, the first (or
most natural) location for them to look for would be chromecast/common. So we
probably shouldn't create a surprise here.

> * conceptually, CastContentClient seems to me to fit with the other code in
> app/, such as CastMainDelegate, under the notion of "content/ implementation
> that's neither browser- or renderer-specific."

The difference here is that content_main_delegate.h is located in
content/public/app, so our implementation of that API is therefore in
chromecast/app.

Also there is a description of what goes inside chrome/app/ directory at

http://www.chromium.org/developers/how-tos/getting-around-the-chrome-source-c...

Basically it contains the main browser startup code (so the main function and
the main delegate implementations are there). I am not sure content_client
implementation fits its purpose.

[gunsch, 2014-10-13 15:56:19]

On 2014/10/13 06:15:38, lcwu1 wrote:
> On 2014/10/13 03:13:17, gunsch wrote:
> >
https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gyp
> > File chromecast/chromecast.gyp (right):
> > 
> >
>
https://codereview.chromium.org/638803002/diff/50001/chromecast/chromecast.gy...
> > chromecast/chromecast.gyp:190: 'app/cast_content_client.h',
> > On 2014/10/13 01:26:35, lcwu1 wrote:
> > > Why do you want to place cast_content_client.[cc|h] in app/ instead of in
> > > common/ like what chrome and content_shell do?
> > 
> > That was a very conscious decision:
> > * chromecast/common does not currently have any dependency on content/ and
I'd
> > like to keep it that way
> 
> I think in general a content embedder has (or should have) a directory
structure
> that matches well to content/public directory. That is, <embedder>/browser
> contains code that implements content/public/browser, <embedder>/renderer
> contains code that implements content/public/renderer, and <embedder>/common
> contains code that implements content/public/common (where content_client.h is
> located). So I think it's OK for chromecast/common to depends on
> content/public/common. And in fact, all the existing content embedders
(chrome,
> android_webview, and content_shell) do the same. It's just so happen that we
> have some other common files inside chromecast/common other than
content/common
> implementations. But that's fine because that's also what happens in
> chrome/common.
> 
> I think consistency should be an important factor to consider here. For people
> who are already familiar with chrome/ or content/shell or android_webview, if
> they want to look for Chromecast's content_client implementation, the first
(or
> most natural) location for them to look for would be chromecast/common. So we
> probably shouldn't create a surprise here.
> 
> > * conceptually, CastContentClient seems to me to fit with the other code in
> > app/, such as CastMainDelegate, under the notion of "content/ implementation
> > that's neither browser- or renderer-specific."
> 
> The difference here is that content_main_delegate.h is located in
> content/public/app, so our implementation of that API is therefore in
> chromecast/app.
> 
> Also there is a description of what goes inside chrome/app/ directory at
> 
>
http://www.chromium.org/developers/how-tos/getting-around-the-chrome-source-c...
> 
> Basically it contains the main browser startup code (so the main function and
> the main delegate implementations are there). I am not sure content_client
> implementation fits its purpose.

Hm, I'll buy that. That still leaves us in an uncomfortable place with
chromecast/common, which includes things like command-line flags. (which, for
example, it would be reasonable for chromecast/media to depend on).

How would you feel about having a chromecast/base and moving common code with
low dependencies there? (command-line flags, pref names, etc.) Then I'd feel
more comfortable adding a content/ dependency to chromecast/common and more
closely mirroring the content/public structure.

[gunsch, 2014-10-13 20:04:52]

Discussed offline, made some more changes. PTAL.

[lcwu1, 2014-10-13 20:30:59]

lgtm

[byungchul, 2014-10-13 21:01:02]

lgtm

[gunsch, 2014-10-13 21:24:00]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2014-10-13 21:24:16]

The CQ bit was unchecked by gunsch@chromium.org

[gunsch, 2014-10-13 21:27:44]

On 2014/10/13 21:01:02, byungchul wrote:
> lgtm

TBR'ing a bunch of folks to technically cover OWNERS. This CL touches nothing
outside of the "chromecast/" directory, adds no new dependencies, only
reorganizes our code. This falls squarely within the "widespread or refactoring
changes that are mechanical or global in nature" clause.

[gunsch, 2014-10-13 21:28:22]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-13 21:30:27]

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

[commit-bot: I haz the power, 2014-10-13 21:56:59]

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

[commit-bot: I haz the power, 2014-10-13 21:57:01]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)

[gunsch, 2014-10-13 22:19:49]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-10-13 22:21:24]

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

[commit-bot: I haz the power, 2014-10-13 23:01:59]

Committed patchset #6 (id:980001)

[commit-bot: I haz the power, 2014-10-13 23:02:39]

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