Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27551003:
The content api for shouldoverrideurlloading (Closed)
|
Created:
7 years, 2 months ago by sgurun-gerrit only Modified:
7 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Marshall Greenblatt Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThe content api for shouldoverrideurlloading
Modify the content API to implement android_webview's
shouldoverrideurlloading.
BUG=308257
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239073
Patch Set 1 #Patch Set 2 : try to fix old chunk mismatch error #
Total comments: 2
Patch Set 3 : conditionalized the handlenavigation API for android #
Total comments: 2
Patch Set 4 : addressed nit #Patch Set 5 : rebased #
Messages
Total messages: 27 (0 generated)
PTAL, thanks. The android_webview cl that is going to use this API change is mostly done, adding some tests now. It is at https://codereview.chromium.org/24228003/
Can you reupload? All of the diffs say "error: old chunk mismatch".
On 2013/10/16 21:48:58, Jói wrote: > Can you reupload? All of the diffs say "error: old chunk mismatch". just uploaded again. It seems to be ok now.
+magreenblatt@gmail.com who maintains CEF, to see if this will continue to support the use case it was added for. Cheers, Jói
On 2013/10/16 21:54:07, Jói wrote: > mailto:+magreenblatt@gmail.com who maintains CEF, to see if this will continue to > support the use case it was added for. We no longer use HandleNavigation in CEF (we now use ResourceThrottle instead), so it can be modified or removed as desired. jam@ expressed a desire to remove it, so you might want to check with him as well.
Thanks Marshall, removing you and adding jam@
ah, I wasn't aware that Marshall isn't using it anymore. What is webview using this for? Can the use case switch to ResourceThrottle as extensions and CEF? see https://codereview.chromium.org/23847004 as an example
On 2013/10/16 23:28:11, jam wrote: > ah, I wasn't aware that Marshall isn't using it anymore. > > What is webview using this for? Can the use case switch to ResourceThrottle as > extensions and CEF? see https://codereview.chromium.org/23847004 as an example Hi Jam, Our current implementation is based on ResourceThrottle, please see https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... where we use an InterceptNavigationDelegate to implement it. Unfortunately our compatibility tests revealed that a large number of applications failed due to that (see b/10715395 for details). The main problem is that XHRs are cancelled when a URL is navigated to, even though the navigation really is cancelled by the application (via the delegate). Please see b/10550955 header #12 where we first observed the problem and then discovered so many apps fail because of that. In the long term we may want to modify frameloader in Blink to prevent the cancellation of XHRs. Thanks,
sorry -- rereading my response, I see I did not explain what we use it for that well. Shortly, we use it for implementing interceptnavigationdelegate functionality where navigations can be overridden by the application. On 2013/10/17 00:13:22, sgurun wrote: > On 2013/10/16 23:28:11, jam wrote: > > ah, I wasn't aware that Marshall isn't using it anymore. > > > > What is webview using this for? Can the use case switch to ResourceThrottle as > > extensions and CEF? see https://codereview.chromium.org/23847004 as an example > > Hi Jam, > > Our current implementation is based on ResourceThrottle, please see > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... > where we use an InterceptNavigationDelegate to implement it. Unfortunately our > compatibility tests revealed that a large number of applications failed due to > that (see b/10715395 for details). > > The main problem is that XHRs are cancelled when a URL is navigated to, even > though the navigation really is cancelled by the application (via the delegate). > Please see b/10550955 header #12 where we first observed the problem and then > discovered so many apps fail because of that. In the long term we may want to > modify frameloader in Blink to prevent the cancellation of XHRs. > > Thanks,
On 2013/10/17 00:26:13, sgurun wrote: > sorry -- rereading my response, I see I did not explain what we use it for that > well. Shortly, we use it for implementing interceptnavigationdelegate > functionality where navigations can be overridden by the application. I don't follow what you mean. Perhaps it would help to see the android_webview changes that use this. Isn't that code in src/android_webview? Were you planning on adding the usage in a followup? If so can you add it here? Adding Darin as he's more familiar with this code, and he's the one who helped others use ResourceThrottle instead of this method.
On 2013/10/17 19:46:21, jam wrote: > On 2013/10/17 00:26:13, sgurun wrote: > > sorry -- rereading my response, I see I did not explain what we use it for > that > > well. Shortly, we use it for implementing interceptnavigationdelegate > > functionality where navigations can be overridden by the application. > > I don't follow what you mean. Perhaps it would help to see the android_webview > changes that use this. Isn't that code in src/android_webview? Were you planning > on adding the usage in a followup? If so can you add it here? > > Adding Darin as he's more familiar with this code, and he's the one who helped > others use ResourceThrottle instead of this method. Oh sure, actually I have the other CL pointed out in the first message (I am still addressing some code reviews (nothing structural, mostly comments, tests) there: https://codereview.chromium.org/24228003/
https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/c... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/c... content/public/renderer/content_renderer_client.h:173: virtual bool HandleNavigation(RenderView* view, Our intent is to delete this method. It is better to use the browser-side ResourceThrottle hook. Note also, that the OOPIF infrastructure is moving way more of the navigation system to the browser process. You are much better off sitting on top of the ResourceThrottle infrastructure. What does that not provide that you need?
On 23 October 2013 20:59, <darin@chromium.org> wrote: > > https://codereview.chromium.**org/27551003/diff/10001/** > content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> > File content/public/renderer/**content_renderer_client.h (right): > > https://codereview.chromium.**org/27551003/diff/10001/** > content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> > content/public/renderer/**content_renderer_client.h:173: virtual bool > HandleNavigation(RenderView* view, > Our intent is to delete this method. It is better to use the > browser-side ResourceThrottle hook. Note also, that the OOPIF > infrastructure is moving way more of the navigation system to the > browser process. You are much better off sitting on top of the > ResourceThrottle infrastructure. What does that not provide that you > need? > > The major blocker hit with the current ResourceThrottle approach (as described in the bug) is that by the time the resource request is sent and the throttle triggered, any pending XHRs (e.g. hanging GET) on the page have already been canceled. Many existing applications depend on being able to abort a navigation (by returning true in shouldOverrideURLLoading) and for this to not have any observable side effect on the state of the page. Using the HandleNavigation() hook here is the closest equivalent to the point where both legacy webview and iOS UIWebView make their corresponding shouldOverrideURLLoading calls, so we know that all else being equal apps that are designed to work with either/both those will work with this too. [It's common for apps to use shouldOverrideURLLoading as a arbitrary back-channel from JS to native, exactly because it is the common denominator between these platforms. See b/10535003 for the most scary examples] It's quite likely that as well as canceling XHR, the blink FrameLoader makes other state changes in navigation prior to triggering the resource throttles, so even if/when we solve XHR we'll need to plan carefully in rolling out changes in this so that we can detect other unknown ways this breaks apps, and deal with it. The ideal strategy we're looking for is to get the proven stable solution landed, build comprehensive tests to cover these numerous cases and subtle interactions, and then work to improve the implementation as needed (with reassurance of the test coverage in place around it) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you back up and describe what you are actually trying to accomplish? I feel like I don't understand your first sentence even. -Darin On Thu, Oct 24, 2013 at 3:01 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 23 October 2013 20:59, <darin@chromium.org> wrote: > >> >> https://codereview.chromium.**org/27551003/diff/10001/** >> content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> >> File content/public/renderer/**content_renderer_client.h (right): >> >> https://codereview.chromium.**org/27551003/diff/10001/** >> content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> >> content/public/renderer/**content_renderer_client.h:173: virtual bool >> HandleNavigation(RenderView* view, >> Our intent is to delete this method. It is better to use the >> browser-side ResourceThrottle hook. Note also, that the OOPIF >> infrastructure is moving way more of the navigation system to the >> browser process. You are much better off sitting on top of the >> ResourceThrottle infrastructure. What does that not provide that you >> need? >> >> > > The major blocker hit with the current ResourceThrottle approach (as > described in the bug) is that by the time the resource request is sent and > the throttle triggered, any pending XHRs (e.g. hanging GET) on the page > have already been canceled. > Many existing applications depend on being able to abort a navigation (by > returning true in shouldOverrideURLLoading) and for this to not have any > observable side effect on the state of the page. > > Using the HandleNavigation() hook here is the closest equivalent to the > point where both legacy webview and iOS UIWebView make their corresponding > shouldOverrideURLLoading calls, so we know that all else being equal apps > that are designed to work with either/both those will work with this too. > [It's common for apps to use shouldOverrideURLLoading as a arbitrary > back-channel from JS to native, exactly because it is the common > denominator between these platforms. See b/10535003 for the most scary > examples] > > It's quite likely that as well as canceling XHR, the blink FrameLoader > makes other state changes in navigation prior to triggering the resource > throttles, so even if/when we solve XHR we'll need to plan carefully in > rolling out changes in this so that we can detect other unknown ways this > breaks apps, and deal with it. > > The ideal strategy we're looking for is to get the proven stable solution > landed, build comprehensive tests to cover these numerous cases and subtle > interactions, and then work to improve the implementation as needed (with > reassurance of the test coverage in place around it) > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 24 October 2013 16:59, Darin Fisher <darin@chromium.org> wrote: > Can you back up and describe what you are actually trying to accomplish? I > feel like I don't understand your first sentence even. > > Make a backward compatible implementation of WebViewClient.shouldOverrideUrlLoading() http://developer.android.com/reference/android/webkit/WebViewClient.html#shou... > -Darin > > > On Thu, Oct 24, 2013 at 3:01 PM, Jonathan Dixon <joth@chromium.org> wrote: > >> >> >> >> On 23 October 2013 20:59, <darin@chromium.org> wrote: >> >>> >>> https://codereview.chromium.**org/27551003/diff/10001/** >>> content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> >>> File content/public/renderer/**content_renderer_client.h (right): >>> >>> https://codereview.chromium.**org/27551003/diff/10001/** >>> content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> >>> content/public/renderer/**content_renderer_client.h:173: virtual bool >>> HandleNavigation(RenderView* view, >>> Our intent is to delete this method. It is better to use the >>> browser-side ResourceThrottle hook. Note also, that the OOPIF >>> infrastructure is moving way more of the navigation system to the >>> browser process. You are much better off sitting on top of the >>> ResourceThrottle infrastructure. What does that not provide that you >>> need? >>> >>> >> >> The major blocker hit with the current ResourceThrottle approach (as >> described in the bug) is that by the time the resource request is sent and >> the throttle triggered, any pending XHRs (e.g. hanging GET) on the page >> have already been canceled. >> Many existing applications depend on being able to abort a navigation (by >> returning true in shouldOverrideURLLoading) and for this to not have any >> observable side effect on the state of the page. >> >> Using the HandleNavigation() hook here is the closest equivalent to the >> point where both legacy webview and iOS UIWebView make their corresponding >> shouldOverrideURLLoading calls, so we know that all else being equal apps >> that are designed to work with either/both those will work with this too. >> [It's common for apps to use shouldOverrideURLLoading as a arbitrary >> back-channel from JS to native, exactly because it is the common >> denominator between these platforms. See b/10535003 for the most scary >> examples] >> >> It's quite likely that as well as canceling XHR, the blink FrameLoader >> makes other state changes in navigation prior to triggering the resource >> throttles, so even if/when we solve XHR we'll need to plan carefully in >> rolling out changes in this so that we can detect other unknown ways this >> breaks apps, and deal with it. >> >> The ideal strategy we're looking for is to get the proven stable solution >> landed, build comprehensive tests to cover these numerous cases and subtle >> interactions, and then work to improve the implementation as needed (with >> reassurance of the test coverage in place around it) >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK. Is this called for any URL or only for navigations? Can you please explain in greater detail the XHR issue you were alluding to? I get the sense that you were trying to describe some kind of synchronization issue, but I don't follow all of the details. -Darin On Thu, Oct 24, 2013 at 5:32 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 24 October 2013 16:59, Darin Fisher <darin@chromium.org> wrote: > >> Can you back up and describe what you are actually trying to accomplish? >> I feel like I don't understand your first sentence even. >> >> > Make a backward compatible implementation of > WebViewClient.shouldOverrideUrlLoading() > > http://developer.android.com/reference/android/webkit/WebViewClient.html#shou... > > > > >> -Darin >> >> >> On Thu, Oct 24, 2013 at 3:01 PM, Jonathan Dixon <joth@chromium.org>wrote: >> >>> >>> >>> >>> On 23 October 2013 20:59, <darin@chromium.org> wrote: >>> >>>> >>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>> content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> >>>> File content/public/renderer/**content_renderer_client.h (right): >>>> >>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>> content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> >>>> content/public/renderer/**content_renderer_client.h:173: virtual bool >>>> HandleNavigation(RenderView* view, >>>> Our intent is to delete this method. It is better to use the >>>> browser-side ResourceThrottle hook. Note also, that the OOPIF >>>> infrastructure is moving way more of the navigation system to the >>>> browser process. You are much better off sitting on top of the >>>> ResourceThrottle infrastructure. What does that not provide that you >>>> need? >>>> >>>> >>> >>> The major blocker hit with the current ResourceThrottle approach (as >>> described in the bug) is that by the time the resource request is sent and >>> the throttle triggered, any pending XHRs (e.g. hanging GET) on the page >>> have already been canceled. >>> Many existing applications depend on being able to abort a navigation >>> (by returning true in shouldOverrideURLLoading) and for this to not have >>> any observable side effect on the state of the page. >>> >>> Using the HandleNavigation() hook here is the closest equivalent to the >>> point where both legacy webview and iOS UIWebView make their corresponding >>> shouldOverrideURLLoading calls, so we know that all else being equal apps >>> that are designed to work with either/both those will work with this too. >>> [It's common for apps to use shouldOverrideURLLoading as a arbitrary >>> back-channel from JS to native, exactly because it is the common >>> denominator between these platforms. See b/10535003 for the most scary >>> examples] >>> >>> It's quite likely that as well as canceling XHR, the blink FrameLoader >>> makes other state changes in navigation prior to triggering the resource >>> throttles, so even if/when we solve XHR we'll need to plan carefully in >>> rolling out changes in this so that we can detect other unknown ways this >>> breaks apps, and deal with it. >>> >>> The ideal strategy we're looking for is to get the proven stable >>> solution landed, build comprehensive tests to cover these numerous cases >>> and subtle interactions, and then work to improve the implementation as >>> needed (with reassurance of the test coverage in place around it) >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 24 October 2013 20:47, Darin Fisher <darin@chromium.org> wrote: > OK. Is this called for any URL or only for navigations? > > Only for navigations of the main frame, initiated from the content of the page (i.e. clicking <a href> or JS assigning to window.location) > Can you please explain in greater detail the XHR issue you were alluding > to? I get the sense that you were trying to describe some kind of > synchronization issue, but I don't follow all of the details. > > Imagine an IM client implemented inside a webview. 1/ It has a hanging XHR to a server to poll for incoming messages. 2/ The user clicks a button (implemented as a styled <a herf> link), which the app has chosen to handle entirely native side. 3/ The app overrides shouldOverrideUrlLoading, does its handling, and returns true (to abort the navigation) Problem: the user will receive no more incoming IM messages, as the hanging XHR got canceled inside Blink when the link was clicked (step 2). > -Darin > > > On Thu, Oct 24, 2013 at 5:32 PM, Jonathan Dixon <joth@chromium.org> wrote: > >> >> >> >> On 24 October 2013 16:59, Darin Fisher <darin@chromium.org> wrote: >> >>> Can you back up and describe what you are actually trying to accomplish? >>> I feel like I don't understand your first sentence even. >>> >>> >> Make a backward compatible implementation of >> WebViewClient.shouldOverrideUrlLoading() >> >> http://developer.android.com/reference/android/webkit/WebViewClient.html#shou... >> >> >> >> >>> -Darin >>> >>> >>> On Thu, Oct 24, 2013 at 3:01 PM, Jonathan Dixon <joth@chromium.org>wrote: >>> >>>> >>>> >>>> >>>> On 23 October 2013 20:59, <darin@chromium.org> wrote: >>>> >>>>> >>>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>>> content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> >>>>> File content/public/renderer/**content_renderer_client.h (right): >>>>> >>>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>>> content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> >>>>> content/public/renderer/**content_renderer_client.h:173: virtual bool >>>>> HandleNavigation(RenderView* view, >>>>> Our intent is to delete this method. It is better to use the >>>>> browser-side ResourceThrottle hook. Note also, that the OOPIF >>>>> infrastructure is moving way more of the navigation system to the >>>>> browser process. You are much better off sitting on top of the >>>>> ResourceThrottle infrastructure. What does that not provide that you >>>>> need? >>>>> >>>>> >>>> >>>> The major blocker hit with the current ResourceThrottle approach (as >>>> described in the bug) is that by the time the resource request is sent and >>>> the throttle triggered, any pending XHRs (e.g. hanging GET) on the page >>>> have already been canceled. >>>> Many existing applications depend on being able to abort a navigation >>>> (by returning true in shouldOverrideURLLoading) and for this to not have >>>> any observable side effect on the state of the page. >>>> >>>> Using the HandleNavigation() hook here is the closest equivalent to >>>> the point where both legacy webview and iOS UIWebView make their >>>> corresponding shouldOverrideURLLoading calls, so we know that all else >>>> being equal apps that are designed to work with either/both those will work >>>> with this too. [It's common for apps to use shouldOverrideURLLoading as a >>>> arbitrary back-channel from JS to native, exactly because it is the common >>>> denominator between these platforms. See b/10535003 for the most scary >>>> examples] >>>> >>>> It's quite likely that as well as canceling XHR, the blink FrameLoader >>>> makes other state changes in navigation prior to triggering the resource >>>> throttles, so even if/when we solve XHR we'll need to plan carefully in >>>> rolling out changes in this so that we can detect other unknown ways this >>>> breaks apps, and deal with it. >>>> >>>> The ideal strategy we're looking for is to get the proven stable >>>> solution landed, build comprehensive tests to cover these numerous cases >>>> and subtle interactions, and then work to improve the implementation as >>>> needed (with reassurance of the test coverage in place around it) >>>> >>>> >>> >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Oct 24, 2013 at 9:40 PM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 24 October 2013 20:47, Darin Fisher <darin@chromium.org> wrote: > >> OK. Is this called for any URL or only for navigations? >> >> > Only for navigations of the main frame, initiated from the content of the > page (i.e. clicking <a href> or JS assigning to window.location) > > OK. Note: Navigation always begins asynchronously (via a 0-delay WebCore::Timer). That means JavaScript in the page shouldn't be able to observe the difference between the navigation being aborted in decidePolicyForNavigation or in ResourceThrottle::willStartRequest. > > >> Can you please explain in greater detail the XHR issue you were alluding >> to? I get the sense that you were trying to describe some kind of >> synchronization issue, but I don't follow all of the details. >> >> > Imagine an IM client implemented inside a webview. > 1/ It has a hanging XHR to a server to poll for incoming messages. > 2/ The user clicks a button (implemented as a styled <a herf> link), which > the app has chosen to handle entirely native side. > 3/ The app overrides shouldOverrideUrlLoading, does its handling, and > returns true (to abort the navigation) > > Problem: the user will receive no more incoming IM messages, as the > hanging XHR got canceled inside Blink when the link was clicked (step 2). > Ah, I see. What you are observing is that when the FrameLoader begins a navigation, it stops all other loading, so as to give the greatest priority to the new navigation. If that navigation turns out to be aborted (e.g., handled as a download or by an external program), then even though the page remains intact, the aborted navigation is not without side-effects. I'm surprised these apps do not already handle errors on their hanging GETs and then just reconnect. Don't they have to deal with network connectivity issues anyways? At any rate, it seems an alternative in this day and age to stopping all loading upon starting a new navigation is to just defer all existing loaders instead until the outcome of the new navigation is known. That would have the same effect but without the side-effect of interrupting existing loaders. This seems like it would be a good improvement for the web platform in general. -Darin > > > > >> -Darin >> >> >> On Thu, Oct 24, 2013 at 5:32 PM, Jonathan Dixon <joth@chromium.org>wrote: >> >>> >>> >>> >>> On 24 October 2013 16:59, Darin Fisher <darin@chromium.org> wrote: >>> >>>> Can you back up and describe what you are actually trying to >>>> accomplish? I feel like I don't understand your first sentence even. >>>> >>>> >>> Make a backward compatible implementation of >>> WebViewClient.shouldOverrideUrlLoading() >>> >>> http://developer.android.com/reference/android/webkit/WebViewClient.html#shou... >>> >>> >>> >>> >>>> -Darin >>>> >>>> >>>> On Thu, Oct 24, 2013 at 3:01 PM, Jonathan Dixon <joth@chromium.org>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On 23 October 2013 20:59, <darin@chromium.org> wrote: >>>>> >>>>>> >>>>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>>>> content/public/renderer/**content_renderer_client.h<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h> >>>>>> File content/public/renderer/**content_renderer_client.h (right): >>>>>> >>>>>> https://codereview.chromium.**org/27551003/diff/10001/** >>>>>> content/public/renderer/**content_renderer_client.h#**newcode173<https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/content_renderer_client.h#newcode173> >>>>>> content/public/renderer/**content_renderer_client.h:173: virtual bool >>>>>> HandleNavigation(RenderView* view, >>>>>> Our intent is to delete this method. It is better to use the >>>>>> browser-side ResourceThrottle hook. Note also, that the OOPIF >>>>>> infrastructure is moving way more of the navigation system to the >>>>>> browser process. You are much better off sitting on top of the >>>>>> ResourceThrottle infrastructure. What does that not provide that you >>>>>> need? >>>>>> >>>>>> >>>>> >>>>> The major blocker hit with the current ResourceThrottle approach (as >>>>> described in the bug) is that by the time the resource request is sent and >>>>> the throttle triggered, any pending XHRs (e.g. hanging GET) on the page >>>>> have already been canceled. >>>>> Many existing applications depend on being able to abort a navigation >>>>> (by returning true in shouldOverrideURLLoading) and for this to not have >>>>> any observable side effect on the state of the page. >>>>> >>>>> Using the HandleNavigation() hook here is the closest equivalent to >>>>> the point where both legacy webview and iOS UIWebView make their >>>>> corresponding shouldOverrideURLLoading calls, so we know that all else >>>>> being equal apps that are designed to work with either/both those will work >>>>> with this too. [It's common for apps to use shouldOverrideURLLoading as a >>>>> arbitrary back-channel from JS to native, exactly because it is the common >>>>> denominator between these platforms. See b/10535003 for the most scary >>>>> examples] >>>>> >>>>> It's quite likely that as well as canceling XHR, the blink FrameLoader >>>>> makes other state changes in navigation prior to triggering the resource >>>>> throttles, so even if/when we solve XHR we'll need to plan carefully in >>>>> rolling out changes in this so that we can detect other unknown ways this >>>>> breaks apps, and deal with it. >>>>> >>>>> The ideal strategy we're looking for is to get the proven stable >>>>> solution landed, build comprehensive tests to cover these numerous cases >>>>> and subtle interactions, and then work to improve the implementation as >>>>> needed (with reassurance of the test coverage in place around it) >>>>> >>>>> >>>> >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
bubbling this one back up as it's now the only blocked issue we have for getting webview unforked. As I mentioned offline, getting this complete by Dec 17 would be awesome. Thanks for the help with this Darin. https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/c... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/27551003/diff/10001/content/public/renderer/c... content/public/renderer/content_renderer_client.h:173: virtual bool HandleNavigation(RenderView* view, On 2013/10/24 03:59:34, darin wrote: > Our intent is to delete this method. It is better to use the browser-side > ResourceThrottle hook. Note also, that the OOPIF infrastructure is moving way > more of the navigation system to the browser process. You are much better off > sitting on top of the ResourceThrottle infrastructure. What does that not > provide that you need? As a compromise to get this moving forward, how about we land this change but wrap the whole function in ifdef OS_ANDROID. - we'll open a bug to move to the ResourceThrottle if at all possible. - Moving to throttle will be much more practical to do once we have unforked and got automated tests covering this very bug-sensitive piece (not to mention we can switch our canary builds back to master branch then, to do get manual QA and dogfood coverage) - If the resource throttle does prove unworkable, then I'm almost certain the OOPIF changes to move nav to browser will completely remove our need for this hook anyway (we'd much much rather get navigation layer hooks right there in the browser than have to do our own render->browser hops and jumps)
Darin, I had a shot at modifying the FrameLoader. What I ended up doing is to not call FrameLoader::stopAllLoaders from FrameLoader::loadWithNavigationAction but instead plumb ResourceDispatcherHostImpl::DidStartRequest all the way to FrameLoader and cancel the main document loader from there. Is this what you had in mind? I uploaded super raw patches to https://codereview.chromium.org/95393004/ and https://codereview.chromium.org/95633002/ but rietveld spits out chunk mismatch, so you'll probably be unable to view :/
Darin, can you please take a look at the final patch? thanks,
lgtm https://codereview.chromium.org/27551003/diff/113001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/27551003/diff/113001/content/renderer/render_... content/renderer/render_view_impl.cc:3123: request, type, nit: type on next line
thanks! https://codereview.chromium.org/27551003/diff/113001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/27551003/diff/113001/content/renderer/render_... content/renderer/render_view_impl.cc:3123: request, type, On 2013/12/05 07:42:08, jam wrote: > nit: type on next line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/27551003/123001
Failed to apply patch for content/public/renderer/content_renderer_client.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/public/renderer/content_renderer_client.h
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 173 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file
content/public/renderer/content_renderer_client.h.rej
Patch: content/public/renderer/content_renderer_client.h
Index: content/public/renderer/content_renderer_client.h
diff --git a/content/public/renderer/content_renderer_client.h
b/content/public/renderer/content_renderer_client.h
index
a3a6e5bc19079042abb9e9153e0f1076df4fe74d..b0346dc8a3b6bac22b121a4e7be27800da0e90ea
100644
--- a/content/public/renderer/content_renderer_client.h
+++ b/content/public/renderer/content_renderer_client.h
@@ -50,6 +50,7 @@ struct WebURLError;
namespace content {
+class DocumentState;
class RenderView;
class SynchronousCompositor;
struct KeySystemInfo;
@@ -168,13 +169,22 @@ class CONTENT_EXPORT ContentRendererClient {
// Returns true if a popup window should be allowed.
virtual bool AllowPopup();
+#ifdef OS_ANDROID
+ // TODO(sgurun) This callback is deprecated and will be removed as soon
+ // as android webview completes implementation of a resource throttle based
+ // shouldoverrideurl implementation. See crbug.com/325351
+ //
// Returns true if the navigation was handled by the embedder and should be
- // ignored by WebKit. This method is used by CEF.
- virtual bool HandleNavigation(blink::WebFrame* frame,
+ // ignored by WebKit. This method is used by CEF and android_webview.
+ virtual bool HandleNavigation(RenderView* view,
+ DocumentState* document_state,
+ int opener_id,
+ blink::WebFrame* frame,
const blink::WebURLRequest& request,
blink::WebNavigationType type,
blink::WebNavigationPolicy default_policy,
bool is_redirect);
+#endif
// Returns true if we should fork a new process for the given navigation.
// If |send_referrer| is set to false (which is the default), no referrer
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/27551003/163001
Message was sent while issue was closed.
Change committed as 239073 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
