WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(7.28 KB, patch)
2019-01-25 11:45 PST
,
Dean Jackson
no flags
Details
Formatted Diff
Diff
Patch
(9.76 KB, patch)
2019-01-25 16:45 PST
,
Dean Jackson
cdumez
: review+
cdumez
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Dean Jackson
Comment 1
2019-01-25 10:53:32 PST
<
rdar://problem/47399263
>
Dean Jackson
Comment 2
2019-01-25 10:57:46 PST
Created
attachment 360128
[details]
Patch
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
Created
attachment 360137
[details]
Patch
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
Created
attachment 360191
[details]
Patch
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
Committed
r240533
: <
https://trac.webkit.org/changeset/240533
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug