Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11293157:
Add a new ContentRendererClient::HandleNavigation callback in RenderViewImpl::decidePolicyForNaviga… (Closed)
|
Created:
8 years, 1 month ago by Marshall Modified:
8 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd a new ContentRendererClient::HandleNavigation callback in RenderViewImpl::decidePolicyForNavigation that allows the client to intercept/ignore any navigation request.
BUG=159923
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167694
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 7
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 15 (0 generated)
Please review. jam=FYI
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_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_... 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/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11293157/diff/1/content/renderer/render_view_... 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.
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/co... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/co... 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/co... 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_vi... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2678: if (GetContentClient()->renderer()->HandleNavigation(frame, request, type, nit: Combine these with &&
https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/co... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/co... 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/co... 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_vi... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11293157/diff/6001/content/renderer/render_vi... 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.
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/co... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/11293157/diff/6001/content/public/renderer/co... 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.
LGTM, then.
https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/co... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/co... 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/diff/8002/content/public/renderer/co... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/11293157/diff/8002/content/public/renderer/co... 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?
On Thu, Nov 8, 2012 at 4:32 PM, <jam@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? 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/<https://codereview.chromium.org/1... >
On 2012/11/09 08:00:54, darin wrote: > On Thu, Nov 8, 2012 at 4:32 PM, <mailto:jam@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...> > >
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.
On Fri, Nov 9, 2012 at 9:23 AM, <jam@chromium.org> wrote: > On 2012/11/09 08:00:54, darin wrote: > >> On Thu, Nov 8, 2012 at 4:32 PM, <mailto:jam@chromium.org> wrote: >> > > > >> > https://codereview.chromium.****org/11293157/diff/8002/** >> > >> > > content/public/renderer/****content_renderer_client.h<http** > s://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<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 > > -Darin >> > > > > > >> > >> > > https://codereview.chromium.****org/11293157/%3Chttps://codere** > view.chromium.org/11293157/ <http://codereview.chromium.org/11293157/>> > >> > >> > > > > https://codereview.chromium.**org/11293157/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marshall@chromium.org/11293157/5004
Change committed as 167694 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
