Bug 193831

Summary: REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ews-watchlist, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch cdumez: review+, cdumez: commit-queue-

Description Dean Jackson 2019-01-25 10:52:43 PST
REGRESSION: Some USDz from 3rd party websites don't go directly to AR QL
Comment 1 Dean Jackson 2019-01-25 10:53:32 PST
<rdar://problem/47399263>
Comment 2 Dean Jackson 2019-01-25 10:57:46 PST
Created attachment 360128 [details]
Patch
Comment 3 Tim Horton 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
Comment 4 EWS Watchlist 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.
Comment 5 Dean Jackson 2019-01-25 11:45:17 PST
Created attachment 360137 [details]
Patch
Comment 6 EWS Watchlist 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.
Comment 7 Chris Dumez 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.
Comment 8 Dean Jackson 2019-01-25 16:45:13 PST
Created attachment 360191 [details]
Patch
Comment 9 EWS Watchlist 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.
Comment 10 Chris Dumez 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.
Comment 11 Dean Jackson 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.
Comment 12 Dean Jackson 2019-01-25 17:01:19 PST
Committed r240533: <https://trac.webkit.org/changeset/240533>