Bug 193831 – REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL

[Dean Jackson]

<rdar://problem/47399263>

[Dean Jackson]

Created attachment 360128 [details]
Patch

[Tim Horton]

Comment on attachment 360128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360128&action=review

> Tools/ChangeLog:14
> [phone]  Dean Jackson  <[e-mail]>

Double changelog

[EWS Watchlist]

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]

Created attachment 360137 [details]
Patch

[EWS Watchlist]

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 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]

Created attachment 360191 [details]
Patch

[EWS Watchlist]

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 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 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]

Committed r240533: <https://trac.webkit.org/changeset/240533>