Bug 159410 – When WKWebView prepares the session state blob, we should be able to filter it.

• Component: WebKit2
• Status: RESOLVED FIXED

[Yongjun Zhang]

It would be nice to add a SPI to WKBackForwardList so that a WebKit client can remove the current item.

[Yongjun Zhang]

rdar://problem/26565813

[Yongjun Zhang]

Created attachment 282746 [details]
Patch.

[WebKit Commit Bot]

Attachment 282746 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKBackForwardList.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKBackForwardList.mm:33:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yongjun Zhang]

Created attachment 282747 [details]
Fix style issue.

[Brady Eidson]

Comment on attachment 282747 [details]
Fix style issue.

We've resisted APIs to directly manipulate the back/forward list for the entirety of WK2.

Every time we've heard that one is needed, we've always found an alternate.

Why is this needed?

[Yongjun Zhang]

(In reply to comment #5)
> Comment on attachment 282747 [details]
> Fix style issue.
> 
> We've resisted APIs to directly manipulate the back/forward list for the
> entirety of WK2.
> 
> Every time we've heard that one is needed, we've always found an alternate.
> 
> Why is this needed?

Sometimes when we restore the saved session state from disk, we would like to skip the current item (to prevent crash-loop if the app crashed because of loading the URL in current item). We can't just do bfList->back since that would make it possible to go forward and crash again if user swipes forward.

Seems like there isn't a good way to achieve this with the current API/SPI. Any suggestions?

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 282747 [details]
> > Fix style issue.
> > 
> > We've resisted APIs to directly manipulate the back/forward list for the
> > entirety of WK2.
> > 
> > Every time we've heard that one is needed, we've always found an alternate.
> > 
> > Why is this needed?
> 
> Sometimes when we restore the saved session state from disk, we would like
> to skip the current item (to prevent crash-loop if the app crashed because
> of loading the URL in current item). We can't just do bfList->back since
> that would make it possible to go forward and crash again if user swipes
> forward.
> 
> Seems like there isn't a good way to achieve this with the current API/SPI.
> Any suggestions?

-Take the session state blob
-Restore without navigating
-Make a new session state blob, but filter out the bad url.
-Restore the new session state blob

[Yongjun Zhang]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Comment on attachment 282747 [details]
> > > Fix style issue.
> > > 
> > > We've resisted APIs to directly manipulate the back/forward list for the
> > > entirety of WK2.
> > > 
> > > Every time we've heard that one is needed, we've always found an alternate.
> > > 
> > > Why is this needed?
> > 
> > Sometimes when we restore the saved session state from disk, we would like
> > to skip the current item (to prevent crash-loop if the app crashed because
> > of loading the URL in current item). We can't just do bfList->back since
> > that would make it possible to go forward and crash again if user swipes
> > forward.
> > 
> > Seems like there isn't a good way to achieve this with the current API/SPI.
> > Any suggestions?
> 
> -Take the session state blob
> -Restore without navigating
> -Make a new session state blob, but filter out the bad url.
> -Restore the new session state blob

Thanks Brady! I will try this approach.

[Yongjun Zhang]

Created attachment 282914 [details]
Add a SPI to WKWebView so that we can filter the session state.

[Brady Eidson]

Comment on attachment 282914 [details]
Add a SPI to WKWebView so that we can filter the session state.

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3378
> +    auto handler = adoptNS([filter copy]);
> +    WebKit::SessionState sessionState = _page->sessionState([handler](WebKit::WebBackForwardListItem& item) {

Can put the filter copy inside the lambda:
[handler = adoptNS([filter copy])]

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3384
> +        BOOL (^filterBlock)(WKBackForwardListItem *item) = (BOOL (^)(WKBackForwardListItem *item))handler.get();
> +        if (filterBlock) {
> +            if (!filterBlock(wrapper(item)))
> +                return false;
> +        }
> +        return true;

This should be able to be simpler:

if (!handler)
   return true;

BOOL (^filterBlock)(WKBackForwardListItem *item) = (BOOL (^)(WKBackForwardListItem *item))handler.get();
return filterBlock(wrapper(item));

[Anders Carlsson]

Comment on attachment 282914 [details]
Add a SPI to WKWebView so that we can filter the session state.

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3378
>> +    WebKit::SessionState sessionState = _page->sessionState([handler](WebKit::WebBackForwardListItem& item) {
> 
> Can put the filter copy inside the lambda:
> [handler = adoptNS([filter copy])]

I don't think the block needs to be copied at all? If it does, just use WTF::BlockPtr instead.

[Yongjun Zhang]

(In reply to comment #11)
> Comment on attachment 282914 [details]
> Add a SPI to WKWebView so that we can filter the session state.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282914&action=review
> 
> >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3378
> >> +    WebKit::SessionState sessionState = _page->sessionState([handler](WebKit::WebBackForwardListItem& item) {
> > 
> > Can put the filter copy inside the lambda:
> > [handler = adoptNS([filter copy])]
> 
> I don't think the block needs to be copied at all? If it does, just use
> WTF::BlockPtr instead.

Right, we don't need copy here since WebPageProxy::sessionState is a synchronous call.

[Yongjun Zhang]

Created attachment 282950 [details]
Address review comments.

[Yongjun Zhang]

Created attachment 283023 [details]
Fix build issues.

[Brady Eidson]

Comment on attachment 283023 [details]
Fix build issues.

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3377
> +    WebKit::SessionState sessionState = _page->sessionState([filter](WebKit::WebBackForwardListItem& item) {

[filter = WTFMove(filter)] will avoid a copy. It might just be a pointer copy as I'm not exactly sure how blocks are represented... but probably couldn't hurt!

[WebKit Commit Bot]

Comment on attachment 283023 [details]
Fix build issues.

Clearing flags on attachment: 283023

Committed r203014: <http://trac.webkit.org/changeset/203014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.