Attachment 342018 Details for Bug 186331 – Patch

[patch] Patch

bug-186331-20180605183742.patch (text/plain), 15.38 KB, created by Chris Dumez on 2018-06-05 18:37:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-05 18:37:42 PDT

Size: 15.38 KB

patch

obsolete

>Subversion Revision: 232494
>diff --git a/Source/WebKitLegacy/mac/ChangeLog b/Source/WebKitLegacy/mac/ChangeLog
>index 17a1d447b3257fce8edef02ba685903393507892..307e5b9d0141bc07a33711c7002c35152e0892cc 100644
>--- a/Source/WebKitLegacy/mac/ChangeLog
>+++ b/Source/WebKitLegacy/mac/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-06-05  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(r232082): Websites get loaded inside of Messages App chat transcript
>+        https://bugs.webkit.org/show_bug.cgi?id=186331
>+        <rdar://problem/40735446>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        r232082 made it so that if the client implements decidePolicyForMIMEType / decidePolicyForNavigationAction
>+        but does not call use / ignore on the listener, then we would do "use" by default.
>+        The intention was to restore pre-AsyncPolicyDelegates behavior and unbreak Box.app. However,
>+        the pre-AsyncPolicyDelegates behavior was only to "use" by default for decidePolicyForMIMEType,
>+        not decidePolicyForNavigationAction. Doing "use" by default for decidePolicyForNavigationAction
>+        is new behavior and it breaks Messages.app. This patch updates r232082 so that we now do call
>+        "use" by default on the listener for decidePolicyForMIMEType and "ignore" by default for other
>+        policy decisions. This should restore pre-AsyncPolicyDelegates behavior. This fixes Messages.app
>+        and Box.app is still working properly.
>+
>+        * WebCoreSupport/WebFrameLoaderClient.h:
>+        * WebCoreSupport/WebFrameLoaderClient.mm:
>+        (WebFrameLoaderClient::dispatchDecidePolicyForResponse):
>+        (WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction):
>+        (WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
>+        (WebFrameLoaderClient::dispatchWillSubmitForm):
>+        (WebFrameLoaderClient::setUpPolicyListener):
>+        (-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:]):
>+        (-[WebFramePolicyListener initWithFrame:policyFunction:defaultPolicy:appLinkURL:]):
>+        (-[WebFramePolicyListener dealloc]):
>+        (-[WebFramePolicyListener initWithFrame:policyFunction:]): Deleted.
>+        (-[WebFramePolicyListener initWithFrame:policyFunction:appLinkURL:]): Deleted.
>+
> 2018-06-02  Darin Adler  <darin@apple.com>
> 
>         [Cocoa] Update some code to be more ARC-compatible to prepare for future ARC adoption
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>index 4c9d41f45d1fe0c53fe1a2c6ffcc17aad587018b..0d6aaf87af320b8126fd440f041986be5c82334c 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
>@@ -234,7 +234,7 @@ private:
> 
>     RemoteAXObjectRef accessibilityRemoteObject() final { return 0; }
>     
>-    RetainPtr<WebFramePolicyListener> setUpPolicyListener(WebCore::FramePolicyFunction&&, NSURL *appLinkURL = nil);
>+    RetainPtr<WebFramePolicyListener> setUpPolicyListener(WebCore::FramePolicyFunction&&, WebCore::PolicyAction defaultPolicy, NSURL *appLinkURL = nil);
> 
>     NSDictionary *actionDictionary(const WebCore::NavigationAction&, WebCore::FormState*) const;
>     
>diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>index bdb883994c4acaa347cfa5d4596e61aca103be1e..e59c18991c7f4459f209c2240245669ef712497d 100644
>--- a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>+++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
>@@ -180,11 +180,12 @@ @interface WebFramePolicyListener : NSObject <WebPolicyDecisionListener, WebForm
> #if HAVE(APP_LINKS)
>     RetainPtr<NSURL> _appLinkURL;
> #endif
>+    PolicyAction _defaultPolicy;
> }
> 
>-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction;
>+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy;
> #if HAVE(APP_LINKS)
>-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction appLinkURL:(NSURL *)url;
>+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy appLinkURL:(NSURL *)url;
> #endif
> 
> - (void)invalidate;
>@@ -865,7 +866,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForResponse(const ResourceRespons
>                         decidePolicyForMIMEType:response.mimeType()
>                                         request:request.nsURLRequest(UpdateHTTPBody)
>                                           frame:m_webFrame.get()
>-                               decisionListener:setUpPolicyListener(WTFMove(function)).get()];
>+                               decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Use).get()];
> }
> 
> 
>@@ -897,7 +898,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction(const Navigati
>             decidePolicyForNewWindowAction:actionDictionary(action, formState)
>                                    request:request.nsURLRequest(UpdateHTTPBody)
>                               newFrameName:frameName
>-                          decisionListener:setUpPolicyListener(WTFMove(function), tryAppLink ? (NSURL *)request.url() : nil).get()];
>+                          decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Ignore, tryAppLink ? (NSURL *)request.url() : nil).get()];
> }
> 
> void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, PolicyDecisionMode, FramePolicyFunction&& function)
>@@ -909,7 +910,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>                 decidePolicyForNavigationAction:actionDictionary(action, formState)
>                                         request:request.nsURLRequest(UpdateHTTPBody)
>                                           frame:m_webFrame.get()
>-                               decisionListener:setUpPolicyListener(WTFMove(function), tryAppLink ? (NSURL *)request.url() : nil).get()];
>+                               decisionListener:setUpPolicyListener(WTFMove(function), PolicyAction::Ignore, tryAppLink ? (NSURL *)request.url() : nil).get()];
> }
> 
> void WebFrameLoaderClient::cancelPolicyCheck()
>@@ -957,7 +958,7 @@ void WebFrameLoaderClient::dispatchWillSubmitForm(FormState& formState, WTF::Fun
>     }
> 
>     NSDictionary *values = makeFormFieldValuesDictionary(formState);
>-    CallFormDelegate(getWebView(m_webFrame.get()), @selector(frame:sourceFrame:willSubmitForm:withValues:submissionListener:), m_webFrame.get(), kit(formState.sourceDocument().frame()), kit(&formState.form()), values, setUpPolicyListener([function = WTFMove(function)](PolicyAction) { function(); }).get());
>+    CallFormDelegate(getWebView(m_webFrame.get()), @selector(frame:sourceFrame:willSubmitForm:withValues:submissionListener:), m_webFrame.get(), kit(formState.sourceDocument().frame()), kit(&formState.form()), values, setUpPolicyListener([function = WTFMove(function)](PolicyAction) { function(); }, PolicyAction::Ignore).get());
> }
> 
> void WebFrameLoaderClient::revertToProvisionalState(DocumentLoader* loader)
>@@ -1522,7 +1523,7 @@ void WebFrameLoaderClient::dispatchDidBecomeFrameset(bool)
> {
> }
> 
>-RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(FramePolicyFunction&& function, NSURL *appLinkURL)
>+RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(FramePolicyFunction&& function, PolicyAction defaultPolicy, NSURL *appLinkURL)
> {
>     // FIXME: <rdar://5634381> We need to support multiple active policy listeners.
>     [m_policyListener invalidate];
>@@ -1530,10 +1531,10 @@ RetainPtr<WebFramePolicyListener> WebFrameLoaderClient::setUpPolicyListener(Fram
>     RetainPtr<WebFramePolicyListener> policyListener;
> #if HAVE(APP_LINKS)
>     if (appLinkURL)
>-        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) appLinkURL:appLinkURL]);
>+        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) defaultPolicy:defaultPolicy appLinkURL:appLinkURL]);
>     else
> #endif
>-        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function)]);
>+        policyListener = adoptNS([[WebFramePolicyListener alloc] initWithFrame:core(m_webFrame.get()) policyFunction:WTFMove(function) defaultPolicy:defaultPolicy]);
> 
>     m_policyListener = policyListener.get();
> 
>@@ -2386,7 +2387,7 @@ + (void)initialize
> #endif
> }
> 
>-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction
>+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy
> {
>     self = [self init];
>     if (!self)
>@@ -2394,14 +2395,15 @@ - (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFu
> 
>     _frame = frame;
>     _policyFunction = WTFMove(policyFunction);
>+    _defaultPolicy = defaultPolicy;
> 
>     return self;
> }
> 
> #if HAVE(APP_LINKS)
>-- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction appLinkURL:(NSURL *)appLinkURL
>+- (id)initWithFrame:(Frame*)frame policyFunction:(FramePolicyFunction&&)policyFunction defaultPolicy:(PolicyAction)defaultPolicy appLinkURL:(NSURL *)appLinkURL
> {
>-    self = [self initWithFrame:frame policyFunction:WTFMove(policyFunction)];
>+    self = [self initWithFrame:frame policyFunction:WTFMove(policyFunction) defaultPolicy:defaultPolicy];
>     if (!self)
>         return nil;
> 
>@@ -2423,12 +2425,12 @@ - (void)dealloc
>     if (WebCoreObjCScheduleDeallocateOnMainThread([WebFramePolicyListener class], self))
>         return;
> 
>-    // If the app did not respond before the listener is destroyed, then we let the load
>-    // proceed with policy "Use".
>+    // If the app did not respond before the listener is destroyed, then we use the default policy ("Use" for navigation
>+    // response policy decision, "Ignore" for other policy decisions).
>     _frame = nullptr;
>     if (auto policyFunction = std::exchange(_policyFunction, nullptr)) {
>-        RELEASE_LOG_ERROR(Loading, "Client application failed to make a policy decision via WebPolicyDecisionListener, letting the load proceed");
>-        policyFunction(PolicyAction::Use);
>+        RELEASE_LOG_ERROR(Loading, "Client application failed to make a policy decision via WebPolicyDecisionListener, using defaultPolicy %u", _defaultPolicy);
>+        policyFunction(_defaultPolicy);
>     }
> 
>     [super dealloc];
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 2fc4f37a91338e4b7c17e0efe577f8c68e37a94b..a9b3c2576cfb3d1b66e07d638dc3cb969056d965 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-06-05  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(r232082): Websites get loaded inside of Messages App chat transcript
>+        https://bugs.webkit.org/show_bug.cgi?id=186331
>+        <rdar://problem/40735446>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm:
>+        (-[NoDecidePolicyForNavigationActionDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]):
>+        (-[NoDecidePolicyForNavigationActionDecisionDelegate webView:didStartProvisionalLoadForFrame:]):
>+        (TestWebKitAPI::TEST):
>+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForNavigationAction:request:frame:decisionListener:]): Deleted.
>+        (-[NoPolicyDelegateDecisionDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]): Deleted.
>+        (-[NoPolicyDelegateDecisionDelegate webView:didFinishLoadForFrame:]): Deleted.
>+
> 2018-06-04  Alexey Proskuryakov  <ap@apple.com>
> 
>         Add Mojave support to WebKit tools
>diff --git a/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm b/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm
>index cbc3ac6339b81fc88a3144b51a2b91135d991190..a61aea8ee9f883709be05938ea6c9134f5a03e93 100644
>--- a/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm
>+++ b/Tools/TestWebKitAPI/Tests/mac/NoPolicyDelegateResponse.mm
>@@ -28,21 +28,16 @@
> #import <WebKit/WebViewPrivate.h>
> #import <wtf/RetainPtr.h>
> 
>-@interface NoPolicyDelegateDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
>-}
>-@end
>-
> static bool didFinishLoad;
> static bool didNavigationActionCheck;
> static bool didNavigationResponseCheck;
>+static bool didStartProvisionalLoad;
> 
>-@implementation NoPolicyDelegateDecisionDelegate
>-
>-- (void)webView:(WebView *)webView decidePolicyForNavigationAction:(NSDictionary *)actionInformation request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
>-{
>-    // Implements decidePolicyForNavigationAction but fails to call the decision listener.
>-    didNavigationActionCheck = YES;
>+@interface NoDecidePolicyForMIMETypeDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
> }
>+@end
>+
>+@implementation NoDecidePolicyForMIMETypeDecisionDelegate
> 
> - (void)webView:(WebView *)webView decidePolicyForMIMEType:(NSString *)type request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
> {
>@@ -57,12 +52,31 @@ - (void)webView:(WebView *)sender didFinishLoadForFrame:(WebFrame *)frame
> 
> @end
> 
>+@interface NoDecidePolicyForNavigationActionDecisionDelegate : NSObject <WebPolicyDelegate, WebFrameLoadDelegate> {
>+}
>+@end
>+
>+@implementation NoDecidePolicyForNavigationActionDecisionDelegate
>+
>+- (void)webView:(WebView *)webView decidePolicyForNavigationAction:(NSDictionary *)actionInformation request:(NSURLRequest *)request frame:(WebFrame *)frame decisionListener:(id<WebPolicyDecisionListener>)listener
>+{
>+    // Implements decidePolicyForNavigationAction but fails to call the decision listener.
>+    didNavigationActionCheck = YES;
>+}
>+
>+- (void)webView:(WebView *)sender didStartProvisionalLoadForFrame:(WebFrame *)frame
>+{
>+    didStartProvisionalLoad = true;
>+}
>+
>+@end
>+
> namespace TestWebKitAPI {
> 
>-TEST(WebKitLegacy, NoPolicyDelegateDecision)
>+TEST(WebKitLegacy, NoDecidePolicyForMIMETypeDecision)
> {
>     auto webView = adoptNS([[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:nil]);
>-    auto delegate = adoptNS([NoPolicyDelegateDecisionDelegate new]);
>+    auto delegate = adoptNS([NoDecidePolicyForMIMETypeDecisionDelegate new]);
> 
>     webView.get().frameLoadDelegate = delegate.get();
>     webView.get().policyDelegate = delegate.get();
>@@ -70,8 +84,21 @@ TEST(WebKitLegacy, NoPolicyDelegateDecision)
> 
>     Util::run(&didFinishLoad);
> 
>-    EXPECT_TRUE(didNavigationActionCheck);
>     EXPECT_TRUE(didNavigationResponseCheck);
> }
> 
>+TEST(WebKitLegacy, NoDecidePolicyForNavigationActionDecision)
>+{
>+    auto webView = adoptNS([[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:nil]);
>+    auto delegate = adoptNS([NoDecidePolicyForNavigationActionDecisionDelegate new]);
>+
>+    webView.get().frameLoadDelegate = delegate.get();
>+    webView.get().policyDelegate = delegate.get();
>+    [[webView.get() mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"verboseMarkup" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
>+
>+    Util::run(&didNavigationActionCheck);
>+
>+    EXPECT_FALSE(didStartProvisionalLoad);
>+}
>+
> } // namespace TestWebKitAPI

Actions: View | Formatted Diff | Diff

Attachments on bug 186331: 342018