Bug 196284 - [Cocoa] Add new API around WKWebpagePreferences in WKNavigationDelegate and WKWebViewConfiguration
Summary: [Cocoa] Add new API around WKWebpagePreferences in WKNavigationDelegate and W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: DoNotImportToRadar
Depends on: 195989 196173
Blocks: 196005
  Show dependency treegraph
 
Reported: 2019-03-26 19:12 PDT by Wenson Hsieh
Modified: 2019-04-02 22:02 PDT (History)
7 users (show)

See Also:


Attachments
Patch (28.31 KB, patch)
2019-03-26 19:50 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (25.81 KB, patch)
2019-03-29 15:23 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (25.89 KB, patch)
2019-04-01 19:20 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (32.04 KB, patch)
2019-04-02 15:07 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (31.93 KB, patch)
2019-04-02 21:16 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-03-26 19:12:37 PDT
More WIP towards <rdar://problem/47228232>
Comment 1 Wenson Hsieh 2019-03-26 19:50:18 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-03-29 15:23:34 PDT
Created attachment 366313 [details]
Patch
Comment 3 Tim Horton 2019-04-01 14:55:43 PDT
Comment on attachment 366313 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:87
> + @discussion If you do not implement this method,
> + -webView:decidePolicyForNavigationAction:decisionHandler: will be called

More importantly, if you DO implement this one, the historical one won't be called.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:133
> +/*! @abstract The set of default web page preferences to use when loading and rendering content.

Just putting it out there...  we went with Webpage because we write it webpage. If we wrote it "web page" it would be WebPage. So you've got the non-camel-cased version wrong here.
Comment 4 Wenson Hsieh 2019-04-01 15:44:07 PDT
Comment on attachment 366313 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:87
>> + -webView:decidePolicyForNavigationAction:decisionHandler: will be called
> 
> More importantly, if you DO implement this one, the historical one won't be called.

Good point — changed the @discussion here to make that clearer.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:133
>> +/*! @abstract The set of default web page preferences to use when loading and rendering content.
> 
> Just putting it out there...  we went with Webpage because we write it webpage. If we wrote it "web page" it would be WebPage. So you've got the non-camel-cased version wrong here.

Old habits die hard :|

Replaced "web page" with "webpage" here, and in other places (including WKNavigationDelegate.h)
Comment 5 Wenson Hsieh 2019-04-01 19:20:19 PDT
Created attachment 366454 [details]
Rebase on trunk
Comment 6 Wenson Hsieh 2019-04-02 08:49:29 PDT
>     /bin/sh -c /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release-iphoneos/WebKit.build/Script-5DF408C6131DD49700130071.sh
> error: WebKit.framework/Headers/WKWebpagePreferences.h included <WebKitAdditions/WKWebpagePreferencesAdditionsBefore.h> but I could not find a header of that name!
> error: WebKit.framework/Headers/WKWebpagePreferences.h included <WebKitAdditions/WKWebpagePreferencesAdditionsAfter.h> but I could not find a header of that name!
> 
> ** BUILD FAILED **
> 
> 
> The following build commands failed:
> 	PhaseScriptExecution Check\ For\ Framework\ Include\ Consistency /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release-iphoneos/WebKit.build/Script-5DF408C6131DD49700130071.sh

Well, good to know "Check For Framework Include Consistency" is working as intended :/

It seems `WK_FRAMEWORK_HEADER_POSTPROCESSING_DISABLED=YES` for certain build configurations, which includes macOS prior to 10.13. Perhaps I need to pull my "Replace WebKitAdditions includes" script out of postprocess-framework-headers.sh, and into either a separate build phase, or run it alongside postprocess-framework-headers.sh in the "Postprocess Framework Headers" build phase.
Comment 7 Wenson Hsieh 2019-04-02 08:51:25 PDT
> It seems `WK_FRAMEWORK_HEADER_POSTPROCESSING_DISABLED=YES` for certain build
> configurations, which includes macOS prior to 10.13. Perhaps I need to pull

*prior to 10.14.
Comment 8 Wenson Hsieh 2019-04-02 15:07:09 PDT
Created attachment 366543 [details]
Patch
Comment 9 Tim Horton 2019-04-02 17:27:28 PDT
Comment on attachment 366543 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:89
> + Otherwise, -webView:decidePolicyForNavigationAction:decisionHandler: will be
> + called if it is implemented.

What is this sentence for? I think you just need the previous one
Comment 10 Wenson Hsieh 2019-04-02 18:41:50 PDT
Comment on attachment 366543 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegate.h:89
>> + called if it is implemented.
> 
> What is this sentence for? I think you just need the previous one

Sounds good — removed the "Otherwise, …" sentence.
Comment 11 Wenson Hsieh 2019-04-02 21:16:57 PDT
Created attachment 366576 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2019-04-02 21:58:10 PDT
Comment on attachment 366576 [details]
Patch for landing

Clearing flags on attachment: 366576

Committed r243787: <https://trac.webkit.org/changeset/243787>