REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
<rdar://problem/47399263>
Created attachment 360128 [details] Patch
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
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.
Created attachment 360137 [details] Patch
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.
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.
Created attachment 360191 [details] Patch
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.
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.
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.
Committed r240533: <https://trac.webkit.org/changeset/240533>