Add WK2 SPI to prevent the previous back/forward item from remaining in the list <rdar://problem/16248710>
Created attachment 230962 [details] Patch v1
Instead of shouldRemovePreviousBackForwardListItemFromList(), I was thinking this should be something more like shouldAddCurrentBackForwardListItemFromList(). Also, does this work for WebBackForwardList::goToItem()?
(In reply to comment #2) > Instead of shouldRemovePreviousBackForwardListItemFromList(), I was thinking this should be something more like shouldAddCurrentBackForwardListItemFromList(). shouldAddCurrentBackForwardListItemFromList() sounds like the item isn't already in the list, but it is. And that's not an internal concept either, as it's exposed in API: WKBackForwardListItemRef WKBackForwardListGetCurrentItem(WKBackForwardListRef list); > Also, does this work for WebBackForwardList::goToItem()? This patch doesn't, I didn't know that was also a requirement (though it makes sense) Followup on its way.
Comment on attachment 230962 [details] Patch v1 EWS says: /Volumes/Data/EWS/WebKit/Tools/WebKitTestRunner/TestController.cpp:502:5: error: missing field 'shouldRemovePreviousBackForwardListItemFromList' initializer [-Werror,-Wmissing-field-initializers]
Created attachment 230965 [details] Patch v2 - Update to handle gotoitem, and hopefully keep builds working This patch doesn't change the name/function of the SPI, per my previous comments. If you really think it should be a positive action instead of a negative action, I don't mind that. I just really dislike the name of the reason stated. Maybe instead of "shouldAddCurrentBackForwardListItemFromList" it could be "shouldKeepCurrentBackForwardListItemInList"?
(In reply to comment #4) > (From update of attachment 230962 [details]) > EWS says: > > /Volumes/Data/EWS/WebKit/Tools/WebKitTestRunner/TestController.cpp:502:5: error: missing field 'shouldRemovePreviousBackForwardListItemFromList' initializer [-Werror,-Wmissing-field-initializers] Yup, fixed in v2.
(In reply to comment #5) > Created an attachment (id=230965) [details] > Patch v2 - Update to handle gotoitem, and hopefully keep builds working > > This patch doesn't change the name/function of the SPI, per my previous comments. > > If you really think it should be a positive action instead of a negative action, I don't mind that. > > I just really dislike the name of the reason stated. > > Maybe instead of "shouldAddCurrentBackForwardListItemFromList" it could be "shouldKeepCurrentBackForwardListItemInList"? My biggest concern is with the word previous, which I don't think is accurate in all cases. Perhaps we need to define terms here. Here are some proposed names - The items you can go back to are the "back list". - The items you can go forward to are the "forward list". - The current item you are on is called the "current item" - The entire list (back list, forward list, current item) is the "back forward list". So maybe the name should be shouldMoveCurrentItemIntoBackOrForwardLists() (gross). split it into two, shouldMoveCurrentItemIntoBackList() and shouldMoveCurrentItemIntoForwardList(). Maybe shouldKeepCurrentBackForwardListItemInList() is good enough.
(In reply to comment #7) > (In reply to comment #5) > > Created an attachment (id=230965) [details] [details] > > Patch v2 - Update to handle gotoitem, and hopefully keep builds working > > > > This patch doesn't change the name/function of the SPI, per my previous comments. > > > > If you really think it should be a positive action instead of a negative action, I don't mind that. > > > > I just really dislike the name of the reason stated. > > > > Maybe instead of "shouldAddCurrentBackForwardListItemFromList" it could be "shouldKeepCurrentBackForwardListItemInList"? > > My biggest concern is with the word previous, which I don't think is accurate in all cases. I can totally see how "previous" conflicts with "back" and that makes it not-always-accurate. > Perhaps we need to define terms here. Here are some proposed names > > - The items you can go back to are the "back list". > - The items you can go forward to are the "forward list". > - The current item you are on is called the "current item" > - The entire list (back list, forward list, current item) is the "back forward list". > > So maybe the name should be shouldMoveCurrentItemIntoBackOrForwardLists() (gross). Agreed! > split it into two, shouldMoveCurrentItemIntoBackList() and shouldMoveCurrentItemIntoForwardList(). Seems weird that a client would have to implement both. On its way. > Maybe shouldKeepCurrentBackForwardListItemInList() is good enough. I think I'm going to switch over to that.
Created attachment 230969 [details] Patch v3 - Updated the name
Comment on attachment 230969 [details] Patch v3 - Updated the name Attachment 230969 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6488967805403136 New failing tests: http/tests/history/redirect-303.html http/tests/history/popstate-fires-with-pending-requests.html fast/events/backspace-navigates-back.html http/tests/history/redirect-200-refresh-2-seconds.pl http/tests/history/redirect-301.html http/tests/cache/subresource-failover-to-network.html fast/history/gesture-before-onload-location-href.html fast/dom/location-hash.html fast/events/onunload-back-to-page-cache.html fast/images/animated-gif-restored-from-bfcache.html fast/history/go-back-to-changed-name.html http/tests/history/redirect-js-document-location-2-seconds.html http/tests/history/redirect-307.html fast/forms/autocomplete-off-with-default-value-does-not-clear.html compositing/iframes/page-cache-layer-tree.html fast/history/form-submit-in-frame-via-onclick.html fast/forms/select/select-state-restore.html http/tests/history/redirect-302.html fast/history/gesture-before-onload-form-submit.html animations/resume-after-page-cache.html http/tests/history/redirect-js-document-location-0-seconds.html fast/history/form-submit-in-frame.html fast/dom/Window/timer-resume-on-navigation-back.html http/tests/history/back-to-post.php fast/forms/textarea/textarea-state-restore.html http/tests/history/history-navigations-set-referrer.html compositing/fixed-position-scroll-offset-history-restore.html fast/forms/radio/state-restore-radio-group.html fast/css/target-fragment-match.html http/tests/history/back-during-onload-triggered-by-back.html
Created attachment 230970 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Followup to the failures was landed right afterwards in http://trac.webkit.org/changeset/168411
And I even failed to mention the original patch landing in http://trac.webkit.org/changeset/168407