Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2597133002:
Do not navigate on loading iframes in StaticHTMLViewController. (Closed)
|
Created:
4 years ago by Olivier Modified:
3 years, 11 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, jam, pkl (ping after 24h if needed), darin-cc_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not navigate on loading iframes in StaticHTMLViewController.
If the static HTML contains iframes, a call to
decidePolicyForNavigationAction is done with a navigationType other.
This should not trigger a navigation to URLLoader.
BUG=676575
Committed: https://crrev.com/99d86ffdb2de89d7c7ee0d74a9ba92157ece5e4d
Cr-Commit-Position: refs/heads/master@{#440859}
Patch Set 1 #Patch Set 2 : typo #
Total comments: 7
Patch Set 3 : feedback #
Total comments: 1
Patch Set 4 : isMainFrame #
Total comments: 2
Patch Set 5 : feedback #Messages
Total messages: 18 (8 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:90: type:(WKNavigationType)type; nit: s/type/navigationType: https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:235: // All other navigation URLs will be loaded by our UrlLoader if we have one. nit: Please avoid "we" in the comments (go/avoidwe) https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:236: // If type is |WKNavigationTypeOther|, the URL corresponds to an external What does "external resource mean"? I think WKNavigationTypeOther can represent regular link clicks: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller....
Description was changed from ========== Do not navigate on external resources in StaticHTMLViewController. If the static HTML contains external resources, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 ========== to ========== Do not navigate on loading iframes in StaticHTMLViewController. If the static HTML contains iframes, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 ==========
https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:90: type:(WKNavigationType)type; On 2016/12/22 16:21:56, Eugene But wrote: > nit: s/type/navigationType: Done. https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:235: // All other navigation URLs will be loaded by our UrlLoader if we have one. On 2016/12/22 16:21:56, Eugene But wrote: > nit: Please avoid "we" in the comments (go/avoidwe) I did not add the we :) Removed. https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:236: // If type is |WKNavigationTypeOther|, the URL corresponds to an external On 2016/12/22 16:21:56, Eugene But wrote: > What does "external resource mean"? I think WKNavigationTypeOther can represent > regular link clicks: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.... Clicking on regular link is WKNavigationTypeLinkActivated https://developer.apple.com/reference/webkit/wknavigationtype/wknavigationtyp... I can add another check like [_webView isLoading] I checked and the element causing the navigation is an iframe. Updated the comment.
https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2597133002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:90: type:(WKNavigationType)type; On 2016/12/22 16:42:59, Olivier Robin wrote: > On 2016/12/22 16:21:56, Eugene But wrote: > > nit: s/type/navigationType: > > Done. Sorry I meant words before arguments (s/type:/navigationType:) but I guess you won't need this anymore :) https://codereview.chromium.org/2597133002/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2597133002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:201: decidePolicyForNavigationAction:(WKNavigationAction*)navigationAction Per offline conversation, you probably want to use WKNavigationAction.targetFrame.mainFrame
Ping
lgtm. Sorry I did not know that you updated CL and was expecting review. https://codereview.chromium.org/2597133002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2597133002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:206: fromMainFrame:navigationAction.sourceFrame.isMainFrame] navigationAction.sourceFrame.mainFrame or [navigationAction.sourceFrame isMainFrame] but not navigationAction.sourceFrame.isMainFrame https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Objective-C_Meth... https://codereview.chromium.org/2597133002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:236: // If type is |WKNavigationTypeOther|, the URL corresponds to an iframe. This comment looks obsoleted.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2597133002/#ps100001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482945018007480,
"parent_rev": "ec604d66c3b8503bb33bd1265b898469f347b04b", "commit_rev":
"569f823de1abfbcacf88796d15ba1124019b9875"}
Message was sent while issue was closed.
Description was changed from ========== Do not navigate on loading iframes in StaticHTMLViewController. If the static HTML contains iframes, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 ========== to ========== Do not navigate on loading iframes in StaticHTMLViewController. If the static HTML contains iframes, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 Review-Url: https://codereview.chromium.org/2597133002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Do not navigate on loading iframes in StaticHTMLViewController. If the static HTML contains iframes, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 Review-Url: https://codereview.chromium.org/2597133002 ========== to ========== Do not navigate on loading iframes in StaticHTMLViewController. If the static HTML contains iframes, a call to decidePolicyForNavigationAction is done with a navigationType other. This should not trigger a navigation to URLLoader. BUG=676575 Committed: https://crrev.com/99d86ffdb2de89d7c7ee0d74a9ba92157ece5e4d Cr-Commit-Position: refs/heads/master@{#440859} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/99d86ffdb2de89d7c7ee0d74a9ba92157ece5e4d Cr-Commit-Position: refs/heads/master@{#440859} |
