Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1155713005:
Use a resource throttle to implement shouldOverrideUrlLoading. (Closed)
DescriptionUse resource throttle to implement shouldOverrideUrlLoading, core change
We have been using both a resource throttle and a sync IPC to implement
shouldOverrideUrlLoading, with this patch we use only a resource
throttle instead.
This patch depends on https://codereview.chromium.org/1194383003 which
adds a flag indicating that the current request was overridden. The flag
is passed down to AwWebContentsObserver.didFailLoad. That CL in turn
depends on https://codereview.chromium.org/1178273007/ which adds the
flag to blink errors.
BUG=325351
Committed: https://crrev.com/777bb78a111aee919e07f5206267915a87639f88
Cr-Commit-Position: refs/heads/master@{#339109}
Patch Set 1 #Patch Set 2 : Added test for XHR cancellation #Patch Set 3 : Add flag showing that shouldOverrideUrl cancelled the navigation #
Total comments: 13
Patch Set 4 : Change XHR test and remove HandleNavigation path. #
Total comments: 1
Patch Set 5 : Move addition of was_ignored_by_handler flag to its own CL. #Patch Set 6 : Remove shouldOverrideUrlLoading from awContentsClientBridge #
Total comments: 2
Patch Set 7 : Make test server wait for shouldOverrideUrlLoading call before returning xhr request #Patch Set 8 : Rebase #Patch Set 9 : Fix trybot failure (use static inner class in instrumentation test) #
Total comments: 11
Patch Set 10 : Remove unused function and includes #Patch Set 11 : Change userGestureCarryOverInfo logic #
Total comments: 4
Patch Set 12 : Rebase and fix qinmin nit #Messages
Total messages: 83 (17 generated)
gsennton@chromium.org changed reviewers: + sgurun@chromium.org
On 2015/05/26 18:13:22, gsennton wrote: Out of the instrumentation tests in AwContentsClientShouldOverrideUrlLoadingTest this is failing 2 tests: testCanIgnoreLoading testCalledWhenTopLevelAboutBlankNavigation The first test is failing because we are calling onPageFinished from AwWebContentsObserver.didFailLoad for a navigation that should be ignored. The second fails an assert at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (ASSERT(scriptState->contextIsValid());)
On 2015/05/28 09:53:22, gsennton wrote: > On 2015/05/26 18:13:22, gsennton wrote: > > Out of the instrumentation tests in AwContentsClientShouldOverrideUrlLoadingTest > this is failing 2 tests: > testCanIgnoreLoading > testCalledWhenTopLevelAboutBlankNavigation > > The first test is failing because we are calling onPageFinished from > AwWebContentsObserver.didFailLoad for a navigation that should be ignored. > The second fails an assert at > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > (ASSERT(scriptState->contextIsValid());) It seems the second failure is unrelated: https://code.google.com/p/chromium/issues/detail?id=462306
On 2015/05/28 12:24:21, gsennton wrote: > On 2015/05/28 09:53:22, gsennton wrote: > > On 2015/05/26 18:13:22, gsennton wrote: > > > > Out of the instrumentation tests in > AwContentsClientShouldOverrideUrlLoadingTest > > this is failing 2 tests: > > testCanIgnoreLoading > > testCalledWhenTopLevelAboutBlankNavigation > > > > The first test is failing because we are calling onPageFinished from > > AwWebContentsObserver.didFailLoad for a navigation that should be ignored. > > The second fails an assert at > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > (ASSERT(scriptState->contextIsValid());) > > It seems the second failure is unrelated: > https://code.google.com/p/chromium/issues/detail?id=462306 So the issue here (for testCanIgnoreLoading) seems to be that when we were using a sync ipc the didFailLoad function was used for calling onPageFinished in the case where we have been redirected by the server and shouldOverrideUrl returns true for the redirected url (this was introduced by https://codereview.chromium.org/144283007 to mimic the behavior of classic webview). Now with a resource throttle every navigation where shouldOverrideUrl is true ends up in didFailLoad and thus calls onPageFinished (which is unwanted behavior).
I added a test for mkosibas document loading fix, and it passes for the resource throttle so it seems his fix is working :)
Ok, so PS3 is passing webview's instrumentation tests but the behaviour is still a bit off when it comes to redirects. When a url is redirected we call didFailLoad with the original url instead of the new one because the result of the resource throttle is interpreted before we reset the original url to being the redirected one.
Actually it is more reasonable then I thought it is :) the cl needs work obviously, but since the tests also work, I guess it is ok. https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = was_ignored_by_handler; maybe this can be a melded into "reason"?
On 2015/06/04 06:37:34, sgurun wrote: > Actually it is more reasonable then I thought it is :) the cl needs work > obviously, but since the tests also work, I guess it is ok. > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > File content/child/web_url_request_util.cc (right): > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = > was_ignored_by_handler; > maybe this can be a melded into "reason"? I will pass over it a second time tomorrow.
A couple of notes/questions: Regarding the weird behaviour of redirects (calling didFailLoad for the original URL) this does not seem to affect any callbacks (e.g. onPageFinished). This behaviour should only arise when shouldOverrideUrlLoading is true and in that case we set wasIgnoredByHandler to true and therefore never call OnPageFinished in AwWebContentsObserver.didFailLoad (no matter if the passed down url is the original one or the redirected one). What I am a bit worried about is if we would want to change the behaviour of AwWebContentsObserver.didFailLoad for redirects in the future -- then we would have to act on the original URL rather than the redirected one... (but I guess if we have already called OnPageFinished earlier there would be no reason for us to do anything at that point?) Does it matter whether OnPageStarted is called before or after shouldOverrideUrl? -- with the resource throttle we always call shouldOverrideUrlLoading first but for the sync IPC this only seems to happen when creating a popup (since OnPageStarted is called in the resource throttler). https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = was_ignored_by_handler; On 2015/06/04 06:37:34, sgurun wrote: > maybe this can be a melded into "reason"? That might be easier. I thought such a change might be more scary than adding a new flag since we would have to make sure that a lot of places looking for err:ABORTED would also check for the new 'reason' but it might be simpler than changing all the didFail* functions? (I should definitely have asked about this earlier...). Especially since the didFail* functions are overridden by several files not even listed in this CL... (I have only updated the ones needed to build WebView). If we merge it into 'reason' we might not need to do any blink changes as well (the current CL would need the addition of a wasIgnoredByHandler flag to WebURLError and ResourceError :/ ).
mnaganov@chromium.org changed reviewers: + mnaganov@chromium.org
https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59: if (!wasIgnoredByHandler) { nit: Why not to put this condition check into the outer 'if' statement? https://codereview.chromium.org/1155713005/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:908: // sure that xhr request won't be handled before shouldInterruptUrlLoading? typo: shouldOverrideUrlLoading? https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... File android_webview/renderer/aw_content_renderer_client.cc (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... android_webview/renderer/aw_content_renderer_client.cc:68: bool AwContentRendererClient::HandleNavigation( Hmm. Does it mean we can remove this override, as it now always returns false, and this is exactly the same as what the Android version of ContentRendererClient::HandleNavigation does? https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = was_ignored_by_handler; On 2015/06/04 14:55:19, gsennton wrote: > On 2015/06/04 06:37:34, sgurun wrote: > > maybe this can be a melded into "reason"? > > That might be easier. I thought such a change might be more scary than adding a > new flag since we would have to make sure that a lot of places looking for > err:ABORTED would also check for the new 'reason' but it might be simpler than > changing all the didFail* functions? (I should definitely have asked about this > earlier...). Especially since the didFail* functions are overridden by several > files not even listed in this CL... (I have only updated the ones needed to > build WebView). > > If we merge it into 'reason' we might not need to do any blink changes as well > (the current CL would need the addition of a wasIgnoredByHandler flag to > WebURLError and ResourceError :/ ). A possible alternative is to avoid routing this flag all the way from the resource loader on the browser side via renderer back again to the browser side :) This flag originates from ResourceRequestInfo, which can be retrieved for any request that we are processing. The only challenge is that we need this information after we have lost track of the request itself (and probably after the request has already ceased). But since all requests have global IDs, it should be possible to establish this tracking and store a map that we can query later.
> Does it matter whether OnPageStarted is called before or after > shouldOverrideUrl? -- with the resource throttle we always call > shouldOverrideUrlLoading first but for the sync IPC this only seems to happen > when creating a popup (since OnPageStarted is called in the resource throttler). maybe I did not understand your comment well, but we already do have a test for it (testCalledBeforeOnPageStarted).
https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:490: // load, redirect and backforward should also be filtered out. no-op if expression is true? you should be able to combine the expressions. https://codereview.chromium.org/1155713005/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:928: jsInterface.waitForEvent(WAIT_TIMEOUT_MS); // TODO waiting timeout ok? I was expecting a little bit different test: one where a link is clicked to a url such as foo://bar, which also generates an XHR and the test would verify XHR is actually sent out even when shouldoverrideurlloading returns true for foo://bar https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... File android_webview/renderer/aw_content_renderer_client.cc (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... android_webview/renderer/aw_content_renderer_client.cc:68: bool AwContentRendererClient::HandleNavigation( On 2015/06/04 19:09:27, mnaganov wrote: > Hmm. Does it mean we can remove this override, as it now always returns false, > and this is exactly the same as what the Android version of > ContentRendererClient::HandleNavigation does? I think all the handleNavigation path should be removed.
https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:490: // load, redirect and backforward should also be filtered out. On 2015/06/05 07:52:49, sgurun wrote: > no-op if expression is true? you should be able to combine the expressions. Done. https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59: if (!wasIgnoredByHandler) { On 2015/06/04 19:09:27, mnaganov wrote: > nit: Why not to put this condition check into the outer 'if' statement? Done. https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... File android_webview/renderer/aw_content_renderer_client.cc (right): https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... android_webview/renderer/aw_content_renderer_client.cc:68: bool AwContentRendererClient::HandleNavigation( On 2015/06/05 07:52:49, sgurun wrote: > On 2015/06/04 19:09:27, mnaganov wrote: > > Hmm. Does it mean we can remove this override, as it now always returns false, > > and this is exactly the same as what the Android version of > > ContentRendererClient::HandleNavigation does? > > I think all the handleNavigation path should be removed. I have removed the handleNavigation path but there is some stuff left (e.g. in aw_content_browser_client.cc) that was only used from the handleNavigation path, should this be cleaned up in this CL or in a follow-up? (This CL is already touching lots of files...) https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = was_ignored_by_handler; On 2015/06/04 19:09:27, mnaganov wrote: > On 2015/06/04 14:55:19, gsennton wrote: > > On 2015/06/04 06:37:34, sgurun wrote: > > > maybe this can be a melded into "reason"? > > > > That might be easier. I thought such a change might be more scary than adding > a > > new flag since we would have to make sure that a lot of places looking for > > err:ABORTED would also check for the new 'reason' but it might be simpler than > > changing all the didFail* functions? (I should definitely have asked about > this > > earlier...). Especially since the didFail* functions are overridden by several > > files not even listed in this CL... (I have only updated the ones needed to > > build WebView). > > > > If we merge it into 'reason' we might not need to do any blink changes as well > > (the current CL would need the addition of a wasIgnoredByHandler flag to > > WebURLError and ResourceError :/ ). > > A possible alternative is to avoid routing this flag all the way from the > resource loader on the browser side via renderer back again to the browser side > :) > > This flag originates from ResourceRequestInfo, which can be retrieved for any > request that we are processing. The only challenge is that we need this > information after we have lost track of the request itself (and probably after > the request has already ceased). But since all requests have global IDs, it > should be possible to establish this tracking and store a map that we can query > later. I think we would still have the same problem here of having to pass some extra information down to didFailLoad (a request ID instead of the new boolean flag)?
gsennton@chromium.org changed reviewers: + jam@chromium.org
Hi jam@ I added you as reviewer as you might know how to proceed here -- we need to know in AwWebContentsObserver.didFailLoad whether the current request was aborted because of shouldOverrideUrlLoading (which means that the request will be cancelled and ignored). PS4 and earlier patch sets use a boolean flag to pass this information (which means a lot of added changes + the need to add a similar flag to WebUrlError and ResourceError in blink). One alternative would be to use a different error code (something else than ERR_ABORTED), though then we would probably have to make sure that we check for both these error codes at places where we used to only check for ERR_ABORTED. Another alternative might be to store a map of all request IDs to their shouldOverrideUrlLoading return values. But I think we would have the same problem then as with passing a boolean flag, i.e. that we would have to pass down the request ID all the way to AwWebContentsObserver.didFailLoad. Any thoughts about this? :)
On 2015/06/08 14:07:24, gsennton wrote: > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContents.java:490: // > load, redirect and backforward should also be filtered out. > On 2015/06/05 07:52:49, sgurun wrote: > > no-op if expression is true? you should be able to combine the expressions. > > Done. > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > (right): > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59: > if (!wasIgnoredByHandler) { > On 2015/06/04 19:09:27, mnaganov wrote: > > nit: Why not to put this condition check into the outer 'if' statement? > > Done. > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... > File android_webview/renderer/aw_content_renderer_client.cc (right): > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... > android_webview/renderer/aw_content_renderer_client.cc:68: bool > AwContentRendererClient::HandleNavigation( > On 2015/06/05 07:52:49, sgurun wrote: > > On 2015/06/04 19:09:27, mnaganov wrote: > > > Hmm. Does it mean we can remove this override, as it now always returns > false, > > > and this is exactly the same as what the Android version of > > > ContentRendererClient::HandleNavigation does? > > > > I think all the handleNavigation path should be removed. > > I have removed the handleNavigation path but there is some stuff left (e.g. in > aw_content_browser_client.cc) that was only used from the handleNavigation path, > should this be cleaned up in this CL or in a follow-up? (This CL is already > touching lots of files...) > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > File content/child/web_url_request_util.cc (right): > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = > was_ignored_by_handler; > On 2015/06/04 19:09:27, mnaganov wrote: > > On 2015/06/04 14:55:19, gsennton wrote: > > > On 2015/06/04 06:37:34, sgurun wrote: > > > > maybe this can be a melded into "reason"? > > > > > > That might be easier. I thought such a change might be more scary than > adding > > a > > > new flag since we would have to make sure that a lot of places looking for > > > err:ABORTED would also check for the new 'reason' but it might be simpler > than > > > changing all the didFail* functions? (I should definitely have asked about > > this > > > earlier...). Especially since the didFail* functions are overridden by > several > > > files not even listed in this CL... (I have only updated the ones needed to > > > build WebView). > > > > > > If we merge it into 'reason' we might not need to do any blink changes as > well > > > (the current CL would need the addition of a wasIgnoredByHandler flag to > > > WebURLError and ResourceError :/ ). > > > > A possible alternative is to avoid routing this flag all the way from the > > resource loader on the browser side via renderer back again to the browser > side > > :) > > > > This flag originates from ResourceRequestInfo, which can be retrieved for any > > request that we are processing. The only challenge is that we need this > > information after we have lost track of the request itself (and probably after > > the request has already ceased). But since all requests have global IDs, it > > should be possible to establish this tracking and store a map that we can > query > > later. > > I think we would still have the same problem here of having to pass some extra > information down to didFailLoad (a request ID instead of the new boolean flag)? Right, that complicates things. But you may also look at request and response extra data structures: https://code.google.com/p/chromium/codesearch#chromium/src/content/child/requ... https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webu...
On 2015/06/11 19:44:12, mnaganov wrote: > On 2015/06/08 14:07:24, gsennton wrote: > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > > File android_webview/java/src/org/chromium/android_webview/AwContents.java > > (right): > > > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > > android_webview/java/src/org/chromium/android_webview/AwContents.java:490: // > > load, redirect and backforward should also be filtered out. > > On 2015/06/05 07:52:49, sgurun wrote: > > > no-op if expression is true? you should be able to combine the expressions. > > > > Done. > > > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > > File > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java > > (right): > > > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/java/sr... > > > android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:59: > > if (!wasIgnoredByHandler) { > > On 2015/06/04 19:09:27, mnaganov wrote: > > > nit: Why not to put this condition check into the outer 'if' statement? > > > > Done. > > > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... > > File android_webview/renderer/aw_content_renderer_client.cc (right): > > > > > https://codereview.chromium.org/1155713005/diff/40001/android_webview/rendere... > > android_webview/renderer/aw_content_renderer_client.cc:68: bool > > AwContentRendererClient::HandleNavigation( > > On 2015/06/05 07:52:49, sgurun wrote: > > > On 2015/06/04 19:09:27, mnaganov wrote: > > > > Hmm. Does it mean we can remove this override, as it now always returns > > false, > > > > and this is exactly the same as what the Android version of > > > > ContentRendererClient::HandleNavigation does? > > > > > > I think all the handleNavigation path should be removed. > > > > I have removed the handleNavigation path but there is some stuff left (e.g. in > > aw_content_browser_client.cc) that was only used from the handleNavigation > path, > > should this be cleaned up in this CL or in a follow-up? (This CL is already > > touching lots of files...) > > > > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > > File content/child/web_url_request_util.cc (right): > > > > > https://codereview.chromium.org/1155713005/diff/40001/content/child/web_url_r... > > content/child/web_url_request_util.cc:308: error.wasIgnoredByHandler = > > was_ignored_by_handler; > > On 2015/06/04 19:09:27, mnaganov wrote: > > > On 2015/06/04 14:55:19, gsennton wrote: > > > > On 2015/06/04 06:37:34, sgurun wrote: > > > > > maybe this can be a melded into "reason"? > > > > > > > > That might be easier. I thought such a change might be more scary than > > adding > > > a > > > > new flag since we would have to make sure that a lot of places looking for > > > > err:ABORTED would also check for the new 'reason' but it might be simpler > > than > > > > changing all the didFail* functions? (I should definitely have asked about > > > this > > > > earlier...). Especially since the didFail* functions are overridden by > > several > > > > files not even listed in this CL... (I have only updated the ones needed > to > > > > build WebView). > > > > > > > > If we merge it into 'reason' we might not need to do any blink changes as > > well > > > > (the current CL would need the addition of a wasIgnoredByHandler flag to > > > > WebURLError and ResourceError :/ ). > > > > > > A possible alternative is to avoid routing this flag all the way from the > > > resource loader on the browser side via renderer back again to the browser > > side > > > :) > > > > > > This flag originates from ResourceRequestInfo, which can be retrieved for > any > > > request that we are processing. The only challenge is that we need this > > > information after we have lost track of the request itself (and probably > after > > > the request has already ceased). But since all requests have global IDs, it > > > should be possible to establish this tracking and store a map that we can > > query > > > later. > > > > I think we would still have the same problem here of having to pass some extra > > information down to didFailLoad (a request ID instead of the new boolean > flag)? > > Right, that complicates things. But you may also look at request and response > extra data structures: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/requ... > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webu... Jam, can you please take a look at content/public changes. Do you have any suggestions there?
On 2015/06/08 17:15:46, gsennton wrote: > Hi jam@ I added you as reviewer as you might know how to proceed here -- we need > to know in AwWebContentsObserver.didFailLoad whether the current request was > aborted because of shouldOverrideUrlLoading (which means that the request will > be cancelled and ignored). PS4 and earlier patch sets use a boolean flag to pass > this information (which means a lot of added changes + the need to add a similar > flag to WebUrlError and ResourceError in blink). > > One alternative would be to use a different error code (something else than > ERR_ABORTED), though then we would probably have to make sure that we check for > both these error codes at places where we used to only check for ERR_ABORTED. > > Another alternative might be to store a map of all request IDs to their > shouldOverrideUrlLoading return values. But I think we would have the same > problem then as with passing a boolean flag, i.e. that we would have to pass > down the request ID all the way to AwWebContentsObserver.didFailLoad. > > Any thoughts about this? :) Sorry I just saw this (I think i didn't click earlier because it said it wasn't ready in the subject). Adding an extra boolean to the WCO method is totally fine. Updating the other implementations of that interface is trivial work.
https://codereview.chromium.org/1155713005/diff/60001/android_webview/rendere... File android_webview/renderer/aw_content_renderer_client.cc (left): https://codereview.chromium.org/1155713005/diff/60001/android_webview/rendere... android_webview/renderer/aw_content_renderer_client.cc:117: RenderThread::Get()->Send(new AwViewHostMsg_ShouldOverrideUrlLoading( Note that this is the only place where we send this message, so it can also be removed together with the receiving part in android_webview/browser/aw_content_browser_client.cc.
On 2015/06/18 18:08:20, mnaganov wrote: > https://codereview.chromium.org/1155713005/diff/60001/android_webview/rendere... > File android_webview/renderer/aw_content_renderer_client.cc (left): > > https://codereview.chromium.org/1155713005/diff/60001/android_webview/rendere... > android_webview/renderer/aw_content_renderer_client.cc:117: > RenderThread::Get()->Send(new AwViewHostMsg_ShouldOverrideUrlLoading( > Note that this is the only place where we send this message, so it can also be > removed together with the receiving part in > android_webview/browser/aw_content_browser_client.cc. Even better -- as soon as you remove AwContentsMessageFilter::OnShouldOverrideUrlLoading, you can also remove the whole chain of AwContentsClientBridgeBase::ShouldOverrideUrlLoading, down to AwContentsClientBridge.shouldOverrideUrlLoading in Java. w00t!
Ok, so the blink change needed for this CL to work has landed (yay :D) https://codereview.chromium.org/1178273007/ Since the changes to WebContentsObserver touch so many files (50+) it seems better to split out the addition of the ignored-by-handler flag to its own CL. In this way the actual change to use a resource throttle and remove the whole HandleNavigation path will be more easily reviewable (and easier for me to handle as well). I have uploaded the change to add the flag in the following CL: https://codereview.chromium.org/1194383003 The CL for actually using the resource throttle will be uploaded later and will depend on the flag-CL. (If this is a bad idea I can keep using the current CL but that will be even more difficult to handle)
Ok, so I moved the addition of the was-ignored-by-handler flag to its own CL so we can actually see what is going on in this one :D Should I remove everything that has to do with HandleNavigation in this CL or do that in a follow-up? Doing it in this CL would add changes to 6 files, all of the changes would be removals under android_webview/.
Whatever is easier to you. We can just honor this CL status as draft, and do all real work in separate CLs. On Jun 23, 2015 4:06 AM, <gsennton@chromium.org> wrote: > Ok, so I moved the addition of the was-ignored-by-handler flag to its own > CL so > we can actually see what is going on in this one :D > > Should I remove everything that has to do with HandleNavigation in this CL > or do > that in a follow-up? > Doing it in this CL would add changes to 6 files, all of the changes would > be > removals under android_webview/. > > https://codereview.chromium.org/1155713005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
gsennton@chromium.org changed reviewers: + phajdan.jr@chromium.org
Ok, I removed the path used by HandleNavigation in this CL. All the files touching AwContentsClientBridge are clean removals of ShouldOverrideUrlLoading used only by the HandleNavigation path. Also added phajdan.jr@ as reviewer for net/test/ ptal :)
https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java (right): https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1021: mWebServer.setResponseDelay(WAIT_TIMEOUT_MS / 5); No, that's an awfully flaky way. You can use TestWebServer.setResponseWithRunnableAction to run your code before sending a reply. And in this code, you may use something like CountdownLatch in order to block the server until you set up everything on the test's side.
gsennton@chromium.org changed reviewers: - phajdan.jr@chromium.org
Right, so I fixed the test so that the xhr is not answered until shouldOverrideUrlLoading has been called. Though we might still finish the xhr before the navigation that was overridden has been properly cancelled? If so, is there a way to make sure that the navigation has been cancelled before we let the test server proceed with the xhr? Removed phajdan.jr@ from reviewers since we no longer touch net/test/. https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java (right): https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1021: mWebServer.setResponseDelay(WAIT_TIMEOUT_MS / 5); On 2015/06/23 19:06:08, mnaganov wrote: > No, that's an awfully flaky way. You can use > TestWebServer.setResponseWithRunnableAction to run your code before sending a > reply. And in this code, you may use something like CountdownLatch in order to > block the server until you set up everything on the test's side. Done.
On 2015/06/24 11:14:16, gsennton wrote: > Right, so I fixed the test so that the xhr is not answered until > shouldOverrideUrlLoading has been called. Though we might still finish the xhr > before the navigation that was overridden has been properly cancelled? If so, is > there a way to make sure that the navigation has been cancelled before we let > the test server proceed with the xhr? > Sorry, I don't actually see how this can happen. Are you talking about a potential situation where setShouldOverrideUrlLoadingReturnValueOnUiThread does something async, and it doesn't complete by the time when you call clickOnLinkUsingJs? Also, as you wait on shouldOverrideUrlLoadingHelper.waitForCallback, the test code will not unblock the server until shouldOverrideUrlLoading has been called. > Removed phajdan.jr@ from reviewers since we no longer touch net/test/. > > https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java > (right): > > https://codereview.chromium.org/1155713005/diff/100001/android_webview/javate... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1021: > mWebServer.setResponseDelay(WAIT_TIMEOUT_MS / 5); > On 2015/06/23 19:06:08, mnaganov wrote: > > No, that's an awfully flaky way. You can use > > TestWebServer.setResponseWithRunnableAction to run your code before sending a > > reply. And in this code, you may use something like CountdownLatch in order to > > block the server until you set up everything on the test's side. > > Done.
On 2015/06/24 15:37:10, mnaganov wrote: > On 2015/06/24 11:14:16, gsennton wrote: > > Right, so I fixed the test so that the xhr is not answered until > > shouldOverrideUrlLoading has been called. Though we might still finish the xhr > > before the navigation that was overridden has been properly cancelled? If so, > is > > there a way to make sure that the navigation has been cancelled before we let > > the test server proceed with the xhr? > > > > Sorry, I don't actually see how this can happen. Are you talking about a > potential situation where setShouldOverrideUrlLoadingReturnValueOnUiThread does > something async, and it doesn't complete by the time when you call > clickOnLinkUsingJs? > > Also, as you wait on shouldOverrideUrlLoadingHelper.waitForCallback, the test > code will not unblock the server until shouldOverrideUrlLoading has been called. > What I meant was that we are only waiting for the shouldOverrideUrlLoading call but in reality the request is not cancelled by that time, right? If I am not mistaken the request should be cancelled directly after shouldOverrideUrlLoading returns true but there isn't really any guarantee that executing the cancellation code (stopping the loading) will be quicker than the server responds to the xhr?
On 2015/06/24 15:45:11, gsennton wrote: > On 2015/06/24 15:37:10, mnaganov wrote: > > On 2015/06/24 11:14:16, gsennton wrote: > > > Right, so I fixed the test so that the xhr is not answered until > > > shouldOverrideUrlLoading has been called. Though we might still finish the > xhr > > > before the navigation that was overridden has been properly cancelled? If > so, > > is > > > there a way to make sure that the navigation has been cancelled before we > let > > > the test server proceed with the xhr? > > > > > > > Sorry, I don't actually see how this can happen. Are you talking about a > > potential situation where setShouldOverrideUrlLoadingReturnValueOnUiThread > does > > something async, and it doesn't complete by the time when you call > > clickOnLinkUsingJs? > > > > Also, as you wait on shouldOverrideUrlLoadingHelper.waitForCallback, the test > > code will not unblock the server until shouldOverrideUrlLoading has been > called. > > > > What I meant was that we are only waiting for the shouldOverrideUrlLoading call > but in reality the request is not cancelled by that time, right? If I am not > mistaken the request should be cancelled directly after shouldOverrideUrlLoading > returns true but there isn't really any guarantee that executing the > cancellation code (stopping the loading) will be quicker than the server > responds to the xhr? Yeah, I see. Well, unless we issue onPageFinished in this case (which I doubt, because it's an XHR) I don't think there is any signal we can use. The changes LGTM (it's mostly deletions, which is great! :)
On 2015/06/24 20:40:45, mnaganov wrote: > Yeah, I see. Well, unless we issue onPageFinished in this case (which I doubt, > because it's an XHR) I don't think there is any signal we can use. > > The changes LGTM (it's mostly deletions, which is great! :) Also the navigation is overridden so it shouldn't issue an onPageFinished :P The patch over in https://codereview.chromium.org/1194383003/ has landed so all that is needed now is a an L G T M for content/ here, jam@? :)
On 2015/06/25 09:33:41, gsennton wrote: > On 2015/06/24 20:40:45, mnaganov wrote: > > Yeah, I see. Well, unless we issue onPageFinished in this case (which I doubt, > > because it's an XHR) I don't think there is any signal we can use. > > > > The changes LGTM (it's mostly deletions, which is great! :) > > Also the navigation is overridden so it shouldn't issue an onPageFinished :P > > The patch over in https://codereview.chromium.org/1194383003/ has landed so all > that is needed now is a an L G T M for content/ here, jam@? :) @jam PTAL :) (it might be a while before we want to land this because it is difficult to assess whether we will introduce new bugs with this change but it would be nice to be able to land it quickly when we decide to go ahead and land it)
The CQ bit was checked by sgurun@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155713005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by gsennton@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org Link to the patchset: https://codereview.chromium.org/1155713005/#ps140001 (title: "Rebase")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155713005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:78: void AwContentsMessageFilter::OverrideThreadForMessage( nit:remove empty method. https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); so we do not enter updateusergesturecarryoverinfo for subframes any more. this sounds wrong to me, can you confirm it is correct here. https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... File android_webview/renderer/aw_content_renderer_client.cc (left): https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... android_webview/renderer/aw_content_renderer_client.cc:24: #include "content/public/renderer/document_state.h" nit: I don't think this included is used after your change https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... android_webview/renderer/aw_content_renderer_client.cc:36: #include "third_party/WebKit/public/web/WebNavigationType.h" same as above. Can you basically check all the unused includes and remove them in this file and every other after your change.
Ok, so I am not familiar with the UserGestureCarryOver code and as sgurun@ pointed out I changed the condition for calling UpdateUserGestureCarryoverInfo in android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc jaekyun@ could you PTAL at that (there is a comment on it in PS8 in android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc) https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/aw_content_browser_client.cc:78: void AwContentsMessageFilter::OverrideThreadForMessage( On 2015/07/08 01:08:54, sgurun wrote: > nit:remove empty method. Done. https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); On 2015/07/08 01:08:54, sgurun wrote: > so we do not enter updateusergesturecarryoverinfo for subframes any more. this > sounds wrong to me, can you confirm it is correct here. Right, I should have spotted this :(. So instead of } else { InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); } we should use the following? } if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); cc'ing jaekyun@ who added the UserGestureCarryOver flag. https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... File android_webview/renderer/aw_content_renderer_client.cc (left): https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... android_webview/renderer/aw_content_renderer_client.cc:24: #include "content/public/renderer/document_state.h" On 2015/07/08 01:08:54, sgurun wrote: > nit: I don't think this included is used after your change Done. https://codereview.chromium.org/1155713005/diff/160001/android_webview/render... android_webview/renderer/aw_content_renderer_client.cc:36: #include "third_party/WebKit/public/web/WebNavigationType.h" On 2015/07/08 01:08:54, sgurun wrote: > same as above. Can you basically check all the unused includes and remove them > in this file and every other after your change. Done.
jaekyun@chromium.org changed reviewers: + jaekyun@chromium.org
https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); On 2015/07/08 11:12:55, gsennton wrote: > On 2015/07/08 01:08:54, sgurun wrote: > > so we do not enter updateusergesturecarryoverinfo for subframes any more. this > > sounds wrong to me, can you confirm it is correct here. > > Right, I should have spotted this :(. So instead of > > } else { > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > } > > we should use the following? > > } > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. The latter seems correct. But is this to handle external protocol in subframe? In Chrome, we use ExternalProtocolHandler for the purpose instead; https://codereview.chromium.org/1091253008/. Can we use the same logic here?
https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); On 2015/07/08 21:26:18, Jaekyun Seok wrote: > On 2015/07/08 11:12:55, gsennton wrote: > > On 2015/07/08 01:08:54, sgurun wrote: > > > so we do not enter updateusergesturecarryoverinfo for subframes any more. > this > > > sounds wrong to me, can you confirm it is correct here. > > > > Right, I should have spotted this :(. So instead of > > > > } else { > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > } > > > > we should use the following? > > > > } > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > The latter seems correct. > > But is this to handle external protocol in subframe? In Chrome, we use > ExternalProtocolHandler for the purpose instead; > https://codereview.chromium.org/1091253008/. > > Can we use the same logic here? This is to call the shouldOverrideUrlLoading callback letting the owning application decide whether it wants to override the navigation, i.e. make WebView ignore it (and then the application can do whatever it wants to do with the url). Why doesn't the resource throttle used in chrome work for subframes (which the commit message says in the CL you linked to) and why does the external protocol handler work instead? AFAICT shouldOverrideUrlLoading is being called on subframes for WebView with this Cl, is there any specific reason for using the ExternalProtocolHandler path/logic?
jaekyun@chromium.org changed reviewers: + qinmin@chromium.org
qinmin@, could you please look at this CL? In this CL, InterceptNavigationResourceThrottle is being used to handle external protocols in subframes instead of ExternalProtocolHandler, which is a different approach from your CL - https://codereview.chromium.org/1091253008/. What do you think of this? https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); On 2015/07/09 12:53:48, gsennton wrote: > On 2015/07/08 21:26:18, Jaekyun Seok wrote: > > On 2015/07/08 11:12:55, gsennton wrote: > > > On 2015/07/08 01:08:54, sgurun wrote: > > > > so we do not enter updateusergesturecarryoverinfo for subframes any more. > > this > > > > sounds wrong to me, can you confirm it is correct here. > > > > > > Right, I should have spotted this :(. So instead of > > > > > > } else { > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > } > > > > > > we should use the following? > > > > > > } > > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > > > The latter seems correct. > > > > But is this to handle external protocol in subframe? In Chrome, we use > > ExternalProtocolHandler for the purpose instead; > > https://codereview.chromium.org/1091253008/. > > > > Can we use the same logic here? > > This is to call the shouldOverrideUrlLoading callback letting the owning > application decide whether it wants to override the navigation, i.e. make > WebView ignore it (and then the application can do whatever it wants to do with > the url). > InterceptNavigationDelegate#ShouldIgnoreNavigation is called eventually when ExternalProtocolHandler is used; https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... So both are identical. > Why doesn't the resource throttle used in chrome work for subframes (which the > commit message says in the CL you linked to) and why does the external protocol > handler work instead? AFAICT shouldOverrideUrlLoading is being called on > subframes for WebView with this Cl, is there any specific reason for using the > ExternalProtocolHandler path/logic? (CCed qinmin@ for more details) As I know, historically InterceptNavigationResourceThrottle is being used only for mainframe and it handles not only external protocols but also internal ones. And ExternalProtocolHandler is to handle only external protocols, and so in the CL it was used for external protocols from subframe. And InterceptNavigationResourceThrottle always sets true as NavigationParams.is_main_frame; https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat... So you couldn't use InterceptNavigationResourceThrottle as it is anyway.
On 2015/07/09 20:33:58, Jaekyun Seok wrote: > qinmin@, could you please look at this CL? > > In this CL, InterceptNavigationResourceThrottle is being used to handle external > protocols in subframes instead of ExternalProtocolHandler, which is a different > approach from your CL - https://codereview.chromium.org/1091253008/. > > What do you think of this? > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > File > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc > (right): > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > On 2015/07/09 12:53:48, gsennton wrote: > > On 2015/07/08 21:26:18, Jaekyun Seok wrote: > > > On 2015/07/08 11:12:55, gsennton wrote: > > > > On 2015/07/08 01:08:54, sgurun wrote: > > > > > so we do not enter updateusergesturecarryoverinfo for subframes any > more. > > > this > > > > > sounds wrong to me, can you confirm it is correct here. > > > > > > > > Right, I should have spotted this :(. So instead of > > > > > > > > } else { > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > } > > > > > > > > we should use the following? > > > > > > > > } > > > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > > > > > The latter seems correct. > > > > > > But is this to handle external protocol in subframe? In Chrome, we use > > > ExternalProtocolHandler for the purpose instead; > > > https://codereview.chromium.org/1091253008/. > > > > > > Can we use the same logic here? > > > > This is to call the shouldOverrideUrlLoading callback letting the owning > > application decide whether it wants to override the navigation, i.e. make > > WebView ignore it (and then the application can do whatever it wants to do > with > > the url). > > > > InterceptNavigationDelegate#ShouldIgnoreNavigation is called eventually when > ExternalProtocolHandler is used; > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > So both are identical. > > > Why doesn't the resource throttle used in chrome work for subframes (which the > > commit message says in the CL you linked to) and why does the external > protocol > > handler work instead? AFAICT shouldOverrideUrlLoading is being called on > > subframes for WebView with this Cl, is there any specific reason for using the > > ExternalProtocolHandler path/logic? > > (CCed qinmin@ for more details) > > As I know, historically InterceptNavigationResourceThrottle is being used only > for mainframe and it handles not only external protocols but also internal ones. > And ExternalProtocolHandler is to handle only external protocols, and so in the > CL it was used for external protocols from subframe. > > And InterceptNavigationResourceThrottle always sets true as > NavigationParams.is_main_frame; > https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat... > So you couldn't use InterceptNavigationResourceThrottle as it is anyway. interesting, I thought we we had tests to verify shouldoverrideurlloading is called for subframes, how come these tests passed then? (or we don't have tests which means we need to write them :) )
On 2015/07/09 22:42:30, sgurun wrote: > On 2015/07/09 20:33:58, Jaekyun Seok wrote: > > qinmin@, could you please look at this CL? > > > > In this CL, InterceptNavigationResourceThrottle is being used to handle > external > > protocols in subframes instead of ExternalProtocolHandler, which is a > different > > approach from your CL - https://codereview.chromium.org/1091253008/. > > > > What do you think of this? > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > File > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc > > (right): > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > On 2015/07/09 12:53:48, gsennton wrote: > > > On 2015/07/08 21:26:18, Jaekyun Seok wrote: > > > > On 2015/07/08 11:12:55, gsennton wrote: > > > > > On 2015/07/08 01:08:54, sgurun wrote: > > > > > > so we do not enter updateusergesturecarryoverinfo for subframes any > > more. > > > > this > > > > > > sounds wrong to me, can you confirm it is correct here. > > > > > > > > > > Right, I should have spotted this :(. So instead of > > > > > > > > > > } else { > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > } > > > > > > > > > > we should use the following? > > > > > > > > > > } > > > > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > > > > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > > > > > > > The latter seems correct. > > > > > > > > But is this to handle external protocol in subframe? In Chrome, we use > > > > ExternalProtocolHandler for the purpose instead; > > > > https://codereview.chromium.org/1091253008/. > > > > > > > > Can we use the same logic here? > > > > > > This is to call the shouldOverrideUrlLoading callback letting the owning > > > application decide whether it wants to override the navigation, i.e. make > > > WebView ignore it (and then the application can do whatever it wants to do > > with > > > the url). > > > > > > > InterceptNavigationDelegate#ShouldIgnoreNavigation is called eventually when > > ExternalProtocolHandler is used; > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > So both are identical. > > > > > Why doesn't the resource throttle used in chrome work for subframes (which > the > > > commit message says in the CL you linked to) and why does the external > > protocol > > > handler work instead? AFAICT shouldOverrideUrlLoading is being called on > > > subframes for WebView with this Cl, is there any specific reason for using > the > > > ExternalProtocolHandler path/logic? > > > > (CCed qinmin@ for more details) > > > > As I know, historically InterceptNavigationResourceThrottle is being used only > > for mainframe and it handles not only external protocols but also internal > ones. > > And ExternalProtocolHandler is to handle only external protocols, and so in > the > > CL it was used for external protocols from subframe. > > > > And InterceptNavigationResourceThrottle always sets true as > > NavigationParams.is_main_frame; > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat... > > So you couldn't use InterceptNavigationResourceThrottle as it is anyway. > > interesting, I thought we we had tests to verify shouldoverrideurlloading is > called for subframes, how come these tests passed then? (or we don't have tests > which means we need to write them :) ) shouldoverrideurlloading will be called with this CL, but the value of NavigationParams.is_main_frame is always true even for external protocols in subframes.
On 2015/07/09 22:51:28, Jaekyun Seok wrote: > On 2015/07/09 22:42:30, sgurun wrote: > > On 2015/07/09 20:33:58, Jaekyun Seok wrote: > > > qinmin@, could you please look at this CL? > > > > > > In this CL, InterceptNavigationResourceThrottle is being used to handle > > external > > > protocols in subframes instead of ExternalProtocolHandler, which is a > > different > > > approach from your CL - https://codereview.chromium.org/1091253008/. > > > > > > What do you think of this? > > > > > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > > File > > > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > > > > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > On 2015/07/09 12:53:48, gsennton wrote: > > > > On 2015/07/08 21:26:18, Jaekyun Seok wrote: > > > > > On 2015/07/08 11:12:55, gsennton wrote: > > > > > > On 2015/07/08 01:08:54, sgurun wrote: > > > > > > > so we do not enter updateusergesturecarryoverinfo for subframes any > > > more. > > > > > this > > > > > > > sounds wrong to me, can you confirm it is correct here. > > > > > > > > > > > > Right, I should have spotted this :(. So instead of > > > > > > > > > > > > } else { > > > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > } > > > > > > > > > > > > we should use the following? > > > > > > > > > > > > } > > > > > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > > > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > > > > > > > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > > > > > > > > > The latter seems correct. > > > > > > > > > > But is this to handle external protocol in subframe? In Chrome, we use > > > > > ExternalProtocolHandler for the purpose instead; > > > > > https://codereview.chromium.org/1091253008/. > > > > > > > > > > Can we use the same logic here? > > > > > > > > This is to call the shouldOverrideUrlLoading callback letting the owning > > > > application decide whether it wants to override the navigation, i.e. make > > > > WebView ignore it (and then the application can do whatever it wants to do > > > with > > > > the url). > > > > > > > > > > InterceptNavigationDelegate#ShouldIgnoreNavigation is called eventually when > > > ExternalProtocolHandler is used; > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > > So both are identical. > > > > > > > Why doesn't the resource throttle used in chrome work for subframes (which > > the > > > > commit message says in the CL you linked to) and why does the external > > > protocol > > > > handler work instead? AFAICT shouldOverrideUrlLoading is being called on > > > > subframes for WebView with this Cl, is there any specific reason for using > > the > > > > ExternalProtocolHandler path/logic? > > > > > > (CCed qinmin@ for more details) > > > > > > As I know, historically InterceptNavigationResourceThrottle is being used > only > > > for mainframe and it handles not only external protocols but also internal > > ones. > > > And ExternalProtocolHandler is to handle only external protocols, and so in > > the > > > CL it was used for external protocols from subframe. > > > > > > And InterceptNavigationResourceThrottle always sets true as > > > NavigationParams.is_main_frame; > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat... > > > So you couldn't use InterceptNavigationResourceThrottle as it is anyway. > > > > interesting, I thought we we had tests to verify shouldoverrideurlloading is > > called for subframes, how come these tests passed then? (or we don't have > tests > > which means we need to write them :) ) > > shouldoverrideurlloading will be called with this CL, but the value of > NavigationParams.is_main_frame is always true even for external protocols in > subframes. It seems we don't make use of navigationparams.is_main_frame at the webview side for handling shouldoverrideurlloading, I did not look at the code careful enough to understand why subframes have to follow the externalprotocolhandler path though.
On 2015/07/10 06:46:31, sgurun wrote: > On 2015/07/09 22:51:28, Jaekyun Seok wrote: > > On 2015/07/09 22:42:30, sgurun wrote: > > > On 2015/07/09 20:33:58, Jaekyun Seok wrote: > > > > qinmin@, could you please look at this CL? > > > > > > > > In this CL, InterceptNavigationResourceThrottle is being used to handle > > > external > > > > protocols in subframes instead of ExternalProtocolHandler, which is a > > > different > > > > approach from your CL - https://codereview.chromium.org/1091253008/. > > > > > > > > What do you think of this? > > > > > > > > > > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > > > File > > > > > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1155713005/diff/160001/android_webview/browse... > > > > > > > > > > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > On 2015/07/09 12:53:48, gsennton wrote: > > > > > On 2015/07/08 21:26:18, Jaekyun Seok wrote: > > > > > > On 2015/07/08 11:12:55, gsennton wrote: > > > > > > > On 2015/07/08 01:08:54, sgurun wrote: > > > > > > > > so we do not enter updateusergesturecarryoverinfo for subframes > any > > > > more. > > > > > > this > > > > > > > > sounds wrong to me, can you confirm it is correct here. > > > > > > > > > > > > > > Right, I should have spotted this :(. So instead of > > > > > > > > > > > > > > } else { > > > > > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > } > > > > > > > > > > > > > > we should use the following? > > > > > > > > > > > > > > } > > > > > > > if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) > > > > > > > > > > InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); > > > > > > > > > > > > > > > > > > > > > cc'ing jaekyun@ who added the UserGestureCarryOver flag. > > > > > > > > > > > > The latter seems correct. > > > > > > > > > > > > But is this to handle external protocol in subframe? In Chrome, we use > > > > > > ExternalProtocolHandler for the purpose instead; > > > > > > https://codereview.chromium.org/1091253008/. > > > > > > > > > > > > Can we use the same logic here? > > > > > > > > > > This is to call the shouldOverrideUrlLoading callback letting the owning > > > > > application decide whether it wants to override the navigation, i.e. > make > > > > > WebView ignore it (and then the application can do whatever it wants to > do > > > > with > > > > > the url). > > > > > > > > > > > > > InterceptNavigationDelegate#ShouldIgnoreNavigation is called eventually > when > > > > ExternalProtocolHandler is used; > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > > > So both are identical. > > > > > > > > > Why doesn't the resource throttle used in chrome work for subframes > (which > > > the > > > > > commit message says in the CL you linked to) and why does the external > > > > protocol > > > > > handler work instead? AFAICT shouldOverrideUrlLoading is being called on > > > > > subframes for WebView with this Cl, is there any specific reason for > using > > > the > > > > > ExternalProtocolHandler path/logic? > > > > > > > > (CCed qinmin@ for more details) > > > > > > > > As I know, historically InterceptNavigationResourceThrottle is being used > > only > > > > for mainframe and it handles not only external protocols but also internal > > > ones. > > > > And ExternalProtocolHandler is to handle only external protocols, and so > in > > > the > > > > CL it was used for external protocols from subframe. > > > > > > > > And InterceptNavigationResourceThrottle always sets true as > > > > NavigationParams.is_main_frame; > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat... > > > > So you couldn't use InterceptNavigationResourceThrottle as it is anyway. > > > > > > interesting, I thought we we had tests to verify shouldoverrideurlloading is > > > called for subframes, how come these tests passed then? (or we don't have > > tests > > > which means we need to write them :) ) > > > > shouldoverrideurlloading will be called with this CL, but the value of > > NavigationParams.is_main_frame is always true even for external protocols in > > subframes. > > It seems we don't make use of navigationparams.is_main_frame at the webview side > for handling shouldoverrideurlloading, I did not look at the code careful enough > to understand why subframes have to follow the externalprotocolhandler path > though. Diverging logics doesn't look good, but I don't have a strong opinion on this. qinmin@, do you have any?
ping for qinmin@ regarding #56
https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:231: !request->url().SchemeIs(url::kHttpScheme) && This will allow sub frames to open arbitrary intents, I am not sure about the security impact, better ask someone from the security team to review this. https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:237: if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME) { nit: no {} needed
gsennton@chromium.org changed reviewers: + palmer@chromium.org
Added palmer@ for the potential security issue mentioned in #58 https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:231: !request->url().SchemeIs(url::kHttpScheme) && On 2015/07/13 19:18:47, qinmin wrote: > This will allow sub frames to open arbitrary intents, I am not sure about the > security impact, better ask someone from the security team to review this. If I am not incorrect this was already allowed in the HandleNavigation path, see the removed code in android_webview/renderer/aw_content_renderer_client.cc, i.e. if (frame->parent() && (gurl.SchemeIs(url::kHttpScheme) || gurl.SchemeIs(url::kHttpsScheme) || gurl.SchemeIs(url::kAboutScheme))) return false;
https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... File android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:231: !request->url().SchemeIs(url::kHttpScheme) && On 2015/07/13 20:53:38, gsennton wrote: > On 2015/07/13 19:18:47, qinmin wrote: > > This will allow sub frames to open arbitrary intents, I am not sure about the > > security impact, better ask someone from the security team to review this. > > If I am not incorrect this was already allowed in the HandleNavigation path, see > the removed code in android_webview/renderer/aw_content_renderer_client.cc, i.e. > > if (frame->parent() && > (gurl.SchemeIs(url::kHttpScheme) || gurl.SchemeIs(url::kHttpsScheme) || > gurl.SchemeIs(url::kAboutScheme))) > return false; It sounds like the behavior before was buggy, too. I.e. is this why ads in iframes would always cause my phone to open up the Play Store and try to get me to install come crappy app? Let's stop that from happening.
On 2015/07/13 21:33:11, palmer wrote: > https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... > File > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc > (right): > > https://codereview.chromium.org/1155713005/diff/200001/android_webview/browse... > android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:231: > !request->url().SchemeIs(url::kHttpScheme) && > On 2015/07/13 20:53:38, gsennton wrote: > > On 2015/07/13 19:18:47, qinmin wrote: > > > This will allow sub frames to open arbitrary intents, I am not sure about > the > > > security impact, better ask someone from the security team to review this. > > > > If I am not incorrect this was already allowed in the HandleNavigation path, > see > > the removed code in android_webview/renderer/aw_content_renderer_client.cc, > i.e. > > > > if (frame->parent() && > > (gurl.SchemeIs(url::kHttpScheme) || gurl.SchemeIs(url::kHttpsScheme) || > > gurl.SchemeIs(url::kAboutScheme))) > > return false; > > It sounds like the behavior before was buggy, too. I.e. is this why ads in > iframes would always cause my phone to open up the Play Store and try to get me > to install come crappy app? Let's stop that from happening. No, not because of this, and actually doesn't require the ad to be in an iframe, see crbug.com/501633.
> No, not because of this, and actually doesn't require the ad to be in an iframe, > see crbug.com/501633. OK, thanks. I still don't think it's a good idea for iframes to be formulating arbitrary Intents. Do we know what would break if we removed this?
On 2015/07/14 00:59:55, palmer wrote: > > No, not because of this, and actually doesn't require the ad to be in an > iframe, > > see crbug.com/501633. > > OK, thanks. > > I still don't think it's a good idea for iframes to be formulating arbitrary > Intents. Do we know what would break if we removed this? We do not. Looking at the code, I think this always was the behavior. My preference is to file a bug to address this separately from the refactoring/fix that is happening as part of this bug. ShouldOverrideUrlLoading path is already complex with so much apps depending on it in unknown ways, so I want to take things slowly. Gustav, if nobody has an objection, let's file two further bugs: 1 to combine the path so we investigate using externalprotocolhandler, 2. to investigate sending intents from subframes, and address it in further releases. but let's not try to pile everything in one CL for this very widely used callback.. lgtm
> We do not. Looking at the code, I think this always was the behavior. My > preference is to file a bug to address this separately from the refactoring/fix > that is happening as part of this bug. ShouldOverrideUrlLoading path is already > complex with so much apps depending on it in unknown ways, so I want to take > things slowly. > > Gustav, if nobody has an objection, let's file two further bugs: 1 to combine > the path so we investigate using externalprotocolhandler, 2. to investigate > sending intents from subframes, and address it in further releases. but let's > not try to pile everything in one CL for this very widely used callback.. > > lgtm Ok, so I added issues crbug.com/509963 and crbug.com/509970 (I added a restrict-view label to the latter one since it is a security issue, feel free to remove that label if it is not needed). PS12 is rebased over https://codereview.chromium.org/1180223004.
The CQ bit was checked by gsennton@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155713005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we prevent external protocols from launching within subframes. Why AWResourceDispatcherHostDelegate need to do it differently?
On 2015/07/14 14:06:14, qinmin wrote: > In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we prevent > external protocols from launching within subframes. Why > AWResourceDispatcherHostDelegate need to do it differently? The original CL where we added subframe url overriding is https://chromiumcodereview.appspot.com/11417061 The comment provided with that code states: "We allow intercepting navigations within subframes, but only if the scheme other than http or https. This is because the embedder can't distinguish main frame and subframe callbacks (which could lead to broken content if the embedder decides to not ignore the main frame navigation, but ignores the subframe navigation). The reason this is supported at all is that certain JavaScript-based frameworks use iframe navigation as a form of communication with the embedder."
On 2015/07/14 14:36:19, gsennton wrote: > On 2015/07/14 14:06:14, qinmin wrote: > > In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we prevent > > external protocols from launching within subframes. Why > > AWResourceDispatcherHostDelegate need to do it differently? > > The original CL where we added subframe url overriding is > https://chromiumcodereview.appspot.com/11417061 > The comment provided with that code states: > > "We allow intercepting navigations within subframes, but only if the scheme > other than http or https. This is because the embedder can't distinguish main > frame and subframe callbacks (which could lead to broken content if the embedder > decides to not ignore the main frame navigation, but ignores the subframe > navigation). > The reason this is supported at all is that certain JavaScript-based frameworks > use iframe navigation as a form of communication with the embedder." Right, we cannot change this behavior without breaking apps, we have to be backwards compatible. In crbug/509970 we will discuss a much narrower slice of it, allowing subframes to be able to send intents (that is when the app does not provide a webviewclient).
On 2015/07/14 15:33:04, sgurun wrote: > On 2015/07/14 14:36:19, gsennton wrote: > > On 2015/07/14 14:06:14, qinmin wrote: > > > In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we prevent > > > external protocols from launching within subframes. Why > > > AWResourceDispatcherHostDelegate need to do it differently? > > > > The original CL where we added subframe url overriding is > > https://chromiumcodereview.appspot.com/11417061 > > The comment provided with that code states: > > > > "We allow intercepting navigations within subframes, but only if the scheme > > other than http or https. This is because the embedder can't distinguish main > > frame and subframe callbacks (which could lead to broken content if the > embedder > > decides to not ignore the main frame navigation, but ignores the subframe > > navigation). > > The reason this is supported at all is that certain JavaScript-based > frameworks > > use iframe navigation as a form of communication with the embedder." > > Right, we cannot change this behavior without breaking apps, we have to be > backwards compatible. > In crbug/509970 we will discuss a much narrower slice of it, allowing subframes > to be able to send intents (that is when the app does not provide a > webviewclient). jam, can you please take a look at this. It is now ready for review. Thanks,
On 2015/07/15 18:40:59, sgurun wrote: > On 2015/07/14 15:33:04, sgurun wrote: > > On 2015/07/14 14:36:19, gsennton wrote: > > > On 2015/07/14 14:06:14, qinmin wrote: > > > > In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we > prevent > > > > external protocols from launching within subframes. Why > > > > AWResourceDispatcherHostDelegate need to do it differently? > > > > > > The original CL where we added subframe url overriding is > > > https://chromiumcodereview.appspot.com/11417061 > > > The comment provided with that code states: > > > > > > "We allow intercepting navigations within subframes, but only if the scheme > > > other than http or https. This is because the embedder can't distinguish > main > > > frame and subframe callbacks (which could lead to broken content if the > > embedder > > > decides to not ignore the main frame navigation, but ignores the subframe > > > navigation). > > > The reason this is supported at all is that certain JavaScript-based > > frameworks > > > use iframe navigation as a form of communication with the embedder." > > > > Right, we cannot change this behavior without breaking apps, we have to be > > backwards compatible. > > In crbug/509970 we will discuss a much narrower slice of it, allowing > subframes > > to be able to send intents (that is when the app does not provide a > > webviewclient). > > jam, can you please take a look at this. It is now ready for review. Thanks, by the way, Jam, this is for reviewing content/* (I noticed that with the long list of comments, it is not clear what was asked for review)
On 2015/07/15 18:40:59, sgurun wrote: > On 2015/07/14 15:33:04, sgurun wrote: > > On 2015/07/14 14:36:19, gsennton wrote: > > > On 2015/07/14 14:06:14, qinmin wrote: > > > > In ChromeResourceDispatcherHostDelegate::HandleExternalProtocol, we > prevent > > > > external protocols from launching within subframes. Why > > > > AWResourceDispatcherHostDelegate need to do it differently? > > > > > > The original CL where we added subframe url overriding is > > > https://chromiumcodereview.appspot.com/11417061 > > > The comment provided with that code states: > > > > > > "We allow intercepting navigations within subframes, but only if the scheme > > > other than http or https. This is because the embedder can't distinguish > main > > > frame and subframe callbacks (which could lead to broken content if the > > embedder > > > decides to not ignore the main frame navigation, but ignores the subframe > > > navigation). > > > The reason this is supported at all is that certain JavaScript-based > > frameworks > > > use iframe navigation as a form of communication with the embedder." > > > > Right, we cannot change this behavior without breaking apps, we have to be > > backwards compatible. > > In crbug/509970 we will discuss a much narrower slice of it, allowing > subframes > > to be able to send intents (that is when the app does not provide a > > webviewclient). > > jam, can you please take a look at this. It is now ready for review. Thanks, by the way, Jam, this is for reviewing content/* (I noticed that with the long list of comments, it is not clear what was asked for review)
IPC security LGTM. Thanks for filing the additional bugs!
On 2015/07/16 18:41:09, palmer wrote: > IPC security LGTM. Thanks for filing the additional bugs! thanks for the lgtm!
content lgtm
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org Link to the patchset: https://codereview.chromium.org/1155713005/#ps220001 (title: "Rebase and fix qinmin nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155713005/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/777bb78a111aee919e07f5206267915a87639f88 Cr-Commit-Position: refs/heads/master@{#339109} |
