It would be nice to add a SPI to WKBackForwardList so that a WebKit client can remove the current item.
rdar://problem/26565813
Created attachment 282746 [details] Patch.
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.
Created attachment 282747 [details] Fix style issue.
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?
(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?
(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
(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.
Created attachment 282914 [details] Add a SPI to WKWebView so that we can filter the session state.
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));
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.
(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.
Created attachment 282950 [details] Address review comments.
Created attachment 283023 [details] Fix build issues.
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!
Comment on attachment 283023 [details] Fix build issues. Clearing flags on attachment: 283023 Committed r203014: <http://trac.webkit.org/changeset/203014>
All reviewed patches have been landed. Closing bug.