Bug 196284

Summary: [Cocoa] Add new API around WKWebpagePreferences in WKNavigationDelegate and WKWebViewConfiguration
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bdakin, bfulgham, cdumez, commit-queue, thorton, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 195989, 196173    
Bug Blocks: 196005    
Attachments:
Description Flags
Patch
none
Patch
none
Rebase on trunk
none
Patch
thorton: review+
Patch for landing none

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>