Re: Add a new ContentRendererClient::HandleNavigation callback in RenderViewImpl::decidePolicyForNaviga… (issue 11293157)

[cr...@chromium.org]

See my comments on the bug about whether we should add this API, but here's
a
comment in case we decide to.


https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_impl.cc#newcode2677
content/renderer/render_view_impl.cc:2677: if
(GetContentClient()->renderer()->HandleNavigation(frame, request, type,
We do not want to do this when request.url() == GURL(kSwappedOutURL).
That's an internal implementation URL that should not be exposed to
embedders or the browser process.

I think adding that to this condition is probably the right thing to do.

https://codereview.chromium.org/11293157/

[mars...@chromium.org]

https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_impl.cc#newcode2677
content/renderer/render_view_impl.cc:2677: if
(GetContentClient()->renderer()->HandleNavigation(frame, request, type,

On 2012/11/08 19:06:12, creis wrote:
> We do not want to do this when request.url() == GURL(kSwappedOutURL).
That's an
> internal implementation URL that should not be exposed to embedders or
the
> browser process.

> I think adding that to this condition is probably the right thing to
do.

Done.

https://codereview.chromium.org/11293157/

[cr...@chromium.org]

I'm ok with this behavior, with a few nits below. I'm going to defer to
John
and Darin about whether we want this in the API, though.


https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h#newcode135
content/public/renderer/content_renderer_client.h:135: // Returns true
if the navigation was handled and should be ignored by WebKit.
nit: was handled by the embedder

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h#newcode136
content/public/renderer/content_renderer_client.h:136: virtual bool
HandleNavigation(WebKit::WebFrame* frame,
John, maybe this should be DidHandleNavigation?

That seems more consistent with similar APIs we have in the Chromium
WebKit API, though I guess there are other "Handle" methods below.

https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_view_impl.cc#newcode2678
content/renderer/render_view_impl.cc:2678: if

(GetContentClient()->renderer()->HandleNavigation(frame, request, type,

nit: Combine these with &&

https://codereview.chromium.org/11293157/

[mars...@chromium.org]

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h#newcode135
content/public/renderer/content_renderer_client.h:135: // Returns true
if the navigation was handled and should be ignored by WebKit.

On 2012/11/08 19:24:51, creis wrote:
> nit: was handled by the embedder

Done.

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h#newcode136
content/public/renderer/content_renderer_client.h:136: virtual bool
HandleNavigation(WebKit::WebFrame* frame,

On 2012/11/08 19:24:51, creis wrote:
> John, maybe this should be DidHandleNavigation?

> That seems more consistent with similar APIs we have in the Chromium
WebKit API,
> though I guess there are other "Handle" methods below.

I'm OK with whatever name is preferred.

https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_view_impl.cc#newcode2678
content/renderer/render_view_impl.cc:2678: if
(GetContentClient()->renderer()->HandleNavigation(frame, request, type,

On 2012/11/08 19:24:51, creis wrote:
> nit: Combine these with &&

Done.

https://codereview.chromium.org/11293157/

[j...@chromium.org]

On 2012/11/08 19:24:51, creis wrote:

> I'm ok with this behavior, with a few nits below. I'm going to defer to
> John
> and Darin about whether we want this in the API, though.

this is fine with me. i.e. the content api is there to support the needs of
embedders. almost always that's just Chrome, but CEF is established enough
that
adding small number of APIs like this is fine.

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/content_renderer_client.h#newcode136
content/public/renderer/content_renderer_client.h:136: virtual bool
HandleNavigation(WebKit::WebFrame* frame,
On 2012/11/08 19:24:51, creis wrote:
> John, maybe this should be DidHandleNavigation?

I think the "Did" prefix would be applicable when the function is just
informing the embedder of something. Since this has a return value and
can change state, the current name is good.

> That seems more consistent with similar APIs we have in the Chromium
WebKit API,
> though I guess there are other "Handle" methods below.

https://codereview.chromium.org/11293157/

[cr...@chromium.org]

LGTM, then.

https://codereview.chromium.org/11293157/

[da...@chromium.org]

https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h#newcode137
content/public/renderer/content_renderer_client.h:137: virtual bool
HandleNavigation(WebKit::WebFrame* frame,
One question: How do Chromium developers know not to delete this
function? "It looks like no one uses this."

https://codereview.chromium.org/11293157/

[j...@chromium.org]

https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h#newcode137
content/public/renderer/content_renderer_client.h:137: virtual bool
HandleNavigation(WebKit::WebFrame* frame,

On 2012/11/09 00:30:24, darin wrote:
> One question: How do Chromium developers know not to delete this
function? "It
> looks like no one uses this."

hmm, good question. how do you handle this for WebKit API? or is every
function on it solely for chromium?

https://codereview.chromium.org/11293157/

[Darin Fisher]

The same issue applies.  Just imagine someone doing a code search for a particular function.  If you don't see a caller, wouldn't you be tempted to delete the code?

Neither WebKit API nor Content API are intended as "stable" interfaces.  They provide modularity for Chromium.

Maybe it is enough to just have a .cc file somewhere that enumerates CEF's dependencies?  Hmm...

-Darin

 

https://codereview.chromium.org/11293157/

[j...@chromium.org]

On 2012/11/09 08:00:54, darin wrote:
> On Thu, Nov 8, 2012 at 4:32 PM, <mailto:j...@chromium.org> wrote:

> >
> > https://codereview.chromium.**org/11293157/diff/8002/**
> >

content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h>
> > File content/public/renderer/**content_renderer_client.h (right):
> >
> > https://codereview.chromium.**org/11293157/diff/8002/**
> >

content/public/renderer/**content_renderer_client.h#**newcode137<https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h#newcode137>
> > content/public/renderer/**content_renderer_client.h:137: virtual bool
> > HandleNavigation(WebKit::**WebFrame* frame,

> > On 2012/11/09 00:30:24, darin wrote:
> >
> >> One question: How do Chromium developers know not to delete this
> >>
> > function? "It
> >
> >> looks like no one uses this."
> >>
> >
> > hmm, good question. how do you handle this for WebKit API? or is every
> > function on it solely for chromium?
> >

> The same issue applies. Just imagine someone doing a code search for a
> particular function. If you don't see a caller, wouldn't you be tempted
> to
> delete the code?

So to be clear: you don't have any WebKit API functions that aren't used by
chromium?

> Neither WebKit API nor Content API are intended as "stable" interfaces.
> They provide modularity for Chromium.

Sure, definitely not treating any of these APIs as stable. I'm sure
Marshall has
to do a bit of work every time he syncs.

> Maybe it is enough to just have a .cc file somewhere that enumerates CEF's
> dependencies? Hmm...

Another option can be to comment the function in the header (i.e. // Used
by
CEF)? That should give a clear hint to anyone who tries to remove it.


> -Darin



> >
> >

https://codereview.chromium.**org/11293157/%3Chttps://codereview.chromium.org/11293157/>
> >



https://codereview.chromium.org/11293157/

[mars...@chromium.org]

On 2012/11/09 17:23:10, John Abd-El-Malek wrote:
> Another option can be to comment the function in the header (i.e. //
> Used by
> CEF)? That should give a clear hint to anyone who tries to remove it.

This option sounds good to me. I've updated the patch set accordingly.

https://codereview.chromium.org/11293157/

[Darin Fisher]

On Fri, Nov 9, 2012 at 9:23 AM, <j...@chromium.org> wrote:

On 2012/11/09 08:00:54, darin wrote:

On Thu, Nov 8, 2012 at 4:32 PM, <mailto:j...@chromium.org> wrote:

>
> https://codereview.chromium.**org/11293157/diff/8002/**
>

content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h>

> File content/public/renderer/**content_renderer_client.h (right):
>
> https://codereview.chromium.**org/11293157/diff/8002/**
>

content/public/renderer/**content_renderer_client.h#**newcode137<https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/content_renderer_client.h#newcode137>

> content/public/renderer/**content_renderer_client.h:137: virtual bool
> HandleNavigation(WebKit::**WebFrame* frame,

> On 2012/11/09 00:30:24, darin wrote:
>
>> One question:  How do Chromium developers know not to delete this
>>
> function?  "It
>
>> looks like no one uses this."
>>
>
> hmm, good question. how do you handle this for WebKit API? or is every
> function on it solely for chromium?
>

The same issue applies.  Just imagine someone doing a code search for a
particular function.  If you don't see a caller, wouldn't you be tempted to
delete the code?

So to be clear: you don't have any WebKit API functions that aren't used by
chromium?

Not that I know of.

Neither WebKit API nor Content API are intended as "stable" interfaces.
  They provide modularity for Chromium.

Sure, definitely not treating any of these APIs as stable. I'm sure Marshall has
to do a bit of work every time he syncs.

Maybe it is enough to just have a .cc file somewhere that enumerates CEF's
dependencies?  Hmm...

Another option can be to comment the function in the header (i.e.  // Used by
CEF)? That should give a clear hint to anyone who tries to remove it.

That seems OK.  I worry a bit about comments like that becoming stale though.  The API could end up being used by more than CEF in the future.  Not a big deal I suppose.

-Darin

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mars...@chromium.org/11293157/5004


https://chromiumcodereview.appspot.com/11293157/

[commi...@chromium.org]

Change committed as 167694

https://chromiumcodereview.appspot.com/11293157/