Tighten up content/public DEPS rules using new checkdeps features.

Tighten up content/public DEPS rules using new checkdeps features.

Also, remove use of RenderViewHostImpl from WebContentsObserver as it is not needed.

BUG=98716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151617

[Jói, 2012-08-14 15:35:50]

Stumbled onto this while looking for a way to call Send() and routing_id() with
just a WebContents, and it seemed easy enough to get rid of the dependency on
RenderViewHostImpl.

Cheers,
Jói

[jam, 2012-08-14 15:44:09]

I'm not sure I understand the motivation? It's fine for the cc files in
content/public to depend on the implementation, i.e. in the helper functions for
the observer files. I think having protected member functions in public
interfaces for what is really internal usage of content is something we should
avoid (it's helpful in other places too, but it's a slipper slope, and I'm
pretty sure WebKit API doesn't do this).

[Jói, 2012-08-14 15:52:46]

Meh, no special motivation, I just noticed it and thought it would be
cleaner this way.  If you ever wanted a separate implementation of the
Content API, doing it this way would be better.  I realize that's
probably not a practical concern :)

In the WebContentsObserver (which is what I started looking at) there
is no reason whatsoever to cast to RenderViewHostImpl.  If you prefer,
I can update the patch to just remove the cast from
WebContentsObserver.  If you like I can also update the DEPS file so
that it allows only .cc files to include render_view_host_impl.h (or
all of the files currently in the list, if they are only included in
.cc files).

Cheers,
Jói


On Tue, Aug 14, 2012 at 3:44 PM,  <jam@chromium.org> wrote:
> I'm not sure I understand the motivation? It's fine for the cc files in
> content/public to depend on the implementation, i.e. in the helper functions
> for
> the observer files. I think having protected member functions in public
> interfaces for what is really internal usage of content is something we
> should
> avoid (it's helpful in other places too, but it's a slipper slope, and I'm
> pretty sure WebKit API doesn't do this).
>
> http://codereview.chromium.org/10823315/

[jam, 2012-08-14 16:03:42]

On 2012/08/14 15:52:46, Jói wrote:
> Meh, no special motivation, I just noticed it and thought it would be
> cleaner this way.  If you ever wanted a separate implementation of the
> Content API, doing it this way would be better.  I realize that's
> probably not a practical concern :)
> 
> In the WebContentsObserver (which is what I started looking at) there
> is no reason whatsoever to cast to RenderViewHostImpl.  If you prefer,
> I can update the patch to just remove the cast from
> WebContentsObserver.

sure, that sounds great

>  If you like I can also update the DEPS file so
> that it allows only .cc files to include render_view_host_impl.h (or
> all of the files currently in the list, if they are only included in
> .cc files).

YES! That would be great. This is the reason i had listed the internal content
includes one-by-one, to help make it harder to introduce new includes in
headers, but of course it wasn't perfect since someone can leak these headers in
other headers. Enforcing  this would be awesome.
> 
> Cheers,
> Jói
> 
> 
> On Tue, Aug 14, 2012 at 3:44 PM,  <mailto:jam@chromium.org> wrote:
> > I'm not sure I understand the motivation? It's fine for the cc files in
> > content/public to depend on the implementation, i.e. in the helper functions
> > for
> > the observer files. I think having protected member functions in public
> > interfaces for what is really internal usage of content is something we
> > should
> > avoid (it's helpful in other places too, but it's a slipper slope, and I'm
> > pretty sure WebKit API doesn't do this).
> >
> > http://codereview.chromium.org/10823315/

[Jói, 2012-08-14 16:27:02]

Please take another look.  I updated the patch description to match what it's
now doing.

NB, the docstring at the start of tools/checkdeps/checkdeps.py is up to date
w.r.t. the new features you can use, if you wish to use them in other parts of
content.

Cheers,
Jói

[jam, 2012-08-14 16:40:19]

lgtm

http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS
File content/public/test/DEPS (left):

http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS#ol...
content/public/test/DEPS:6: "+content/test/content_browser_test.h",
can you ignore this file, since I'm taking care of it in another cl?

[Jói, 2012-08-14 16:48:45]

http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS
File content/public/test/DEPS (left):

http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS#ol...
content/public/test/DEPS:6: "+content/test/content_browser_test.h",
On 2012/08/14 16:40:19, John Abd-El-Malek wrote:
> can you ignore this file, since I'm taking care of it in another cl?

Do you mean remove the line?

[jam, 2012-08-14 16:50:37]

On Tue, Aug 14, 2012 at 9:48 AM, <joi@chromium.org> wrote:

>
> http://codereview.chromium.**org/10823315/diff/4001/**
>
content/public/test/DEPS<http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS>
> File content/public/test/DEPS (left):
>
> http://codereview.chromium.**org/10823315/diff/4001/**
>
content/public/test/DEPS#**oldcode6<http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS#oldcode6>
> content/public/test/DEPS:6: "+content/test/content_**browser_test.h",
> On 2012/08/14 16:40:19, John Abd-El-Malek wrote:
>
>> can you ignore this file, since I'm taking care of it in another cl?
>>
>
> Do you mean remove the line?
>

yep

>
>
http://codereview.chromium.**org/10823315/<http://codereview.chromium.org/108...
>

[Jói, 2012-08-14 16:54:22]

Sure thing, will remove before I commit it.

In the interest of landing this quickly, could you give me a rubber
stamp on http://codereview.chromium.org/10830308/ (I sent it to
jamesr@ previously)?  It's ahead of this change in my git pipeline.

Cheers,
Jói


On Tue, Aug 14, 2012 at 4:50 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Aug 14, 2012 at 9:48 AM, <joi@chromium.org> wrote:
>>
>>
>> http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS
>> File content/public/test/DEPS (left):
>>
>>
>>
http://codereview.chromium.org/10823315/diff/4001/content/public/test/DEPS#ol...
>> content/public/test/DEPS:6: "+content/test/content_browser_test.h",
>> On 2012/08/14 16:40:19, John Abd-El-Malek wrote:
>>>
>>> can you ignore this file, since I'm taking care of it in another cl?
>>
>>
>> Do you mean remove the line?
>
>
> yep
>>
>>
>> http://codereview.chromium.org/10823315/
>
>

[commit-bot: I haz the power, 2012-08-14 21:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10823315/10001

[commit-bot: I haz the power, 2012-08-14 21:51:18]

Failed to apply patch for content/public/test/DEPS:
While running patch -p1 --forward --force;
patching file content/public/test/DEPS
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file content/public/test/DEPS.rej

[Jói, 2012-08-14 21:52:52]

Whut? I merged to head! OK let's go one more time.

On Tue, Aug 14, 2012 at 9:51 PM,  <commit-bot@chromium.org> wrote:
> Failed to apply patch for content/public/test/DEPS:
> While running patch -p1 --forward --force;
> patching file content/public/test/DEPS
> Hunk #1 FAILED at 4.
> 1 out of 1 hunk FAILED -- saving rejects to file
> content/public/test/DEPS.rej
>
>
> https://chromiumcodereview.appspot.com/10823315/

[commit-bot: I haz the power, 2012-08-14 21:54:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10823315/5007

[commit-bot: I haz the power, 2012-08-15 00:31:33]

Change committed as 151617