RESOLVED FIXED 193831
REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
https://bugs.webkit.org/show_bug.cgi?id=193831
Summary REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
Dean Jackson
Reported 2019-01-25 10:52:43 PST
REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
Attachments
Patch (7.57 KB, patch)
2019-01-25 10:57 PST, Dean Jackson
no flags
Patch (7.28 KB, patch)
2019-01-25 11:45 PST, Dean Jackson
no flags
Patch (9.76 KB, patch)
2019-01-25 16:45 PST, Dean Jackson
cdumez: review+
cdumez: commit-queue-
Dean Jackson
Comment 1 2019-01-25 10:53:32 PST
Dean Jackson
Comment 2 2019-01-25 10:57:46 PST
Tim Horton
Comment 3 2019-01-25 10:59:51 PST
Comment on attachment 360128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360128&action=review > Tools/ChangeLog:14 > +2019-01-25 Dean Jackson <dino@apple.com> Double changelog
EWS Watchlist
Comment 4 2019-01-25 11:02:00 PST
Attachment 360128 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1588: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1594: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1596: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1602: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dean Jackson
Comment 5 2019-01-25 11:45:17 PST
EWS Watchlist
Comment 6 2019-01-25 11:48:21 PST
Attachment 360137 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1588: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1594: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1596: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1602: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 7 2019-01-25 14:34:58 PST
Comment on attachment 360137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360137&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:2201 > + if (navigation.shouldForceDownload()) I was thinking about this change instead: diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp index e398cf7b73c..e8e1c27d1a4 100644 --- a/Source/WebKit/UIProcess/WebPageProxy.cpp +++ b/Source/WebKit/UIProcess/WebPageProxy.cpp @@ -2687,6 +2687,9 @@ void WebPageProxy::receivedNavigationPolicyDecision(WebPolicyAction policyAction changeWebsiteDataStore(policies->websiteDataStore()->websiteDataStore()); } + if (navigation && navigation->shouldForceDownload() && action == WebPolicyAction::Use) + action = WebPolicyAction::Download; + if (policyAction != WebPolicyAction::Use || !frame.isMainFrame() || !navigation) { receivedPolicyDecision(policyAction, navigation, WTFMove(data), WTFMove(sender)); return; @@ -2732,9 +2735,6 @@ void WebPageProxy::receivedPolicyDecision(WebPolicyAction action, API::Navigatio if (action == WebPolicyAction::Ignore) m_pageLoadState.clearPendingAPIRequestURL(transaction); - if (navigation && navigation->shouldForceDownload() && action == WebPolicyAction::Use) - action = WebPolicyAction::Download; - DownloadID downloadID = { }; if (action == WebPolicyAction::Download) { // Create a download proxy. Our code already takes care of not process-swapping if the policy is "Download". The issue is that shouldForceDownload() sets it to "Download" to late here. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1658 > + shouldConvertToDownload = true; I do not think this covers your change since this does not involve shouldForceDownload(). This causes the decidePolicyForNavigationResponse() to convert the load into a download.
Dean Jackson
Comment 8 2019-01-25 16:45:13 PST
EWS Watchlist
Comment 9 2019-01-25 16:48:11 PST
Attachment 360191 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1588: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1594: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1596: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1602: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 10 2019-01-25 16:53:38 PST
Comment on attachment 360191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360191&action=review r=me with comments > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:560 > +TEST(ProcessSwap, NoSwappingForTLDPlus2) No! this is not a Typo, I really meant eTLD+1. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1632 > + TestWebKitAPI::Util::sleep(1); How about 0.5? 1 seems long. Also, can you set didStartProvisionalLoad to false before clicking the link and make sure it is still false here? > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1667 > + TestWebKitAPI::Util::sleep(1); same comments as above. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3014 > +TEST(ProcessSwap, ProcessReUseTLDPlus2) Not a typo.
Dean Jackson
Comment 11 2019-01-25 16:58:34 PST
Comment on attachment 360191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360191&action=review >> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:560 >> +TEST(ProcessSwap, NoSwappingForTLDPlus2) > > No! this is not a Typo, I really meant eTLD+1. OOPS! fixed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1632 >> + TestWebKitAPI::Util::sleep(1); > > How about 0.5? 1 seems long. > > Also, can you set didStartProvisionalLoad to false before clicking the link and make sure it is still false here? Fixed.
Dean Jackson
Comment 12 2019-01-25 17:01:19 PST
Note You need to log in before you can comment on or make changes to this bug.