Bug 132636 – Add WK2 SPI to prevent the previous back/forward item from remaining in the list

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Add WK2 SPI to prevent the previous back/forward item from remaining in the list

<rdar://problem/16248710>

[Brady Eidson]

Created attachment 230962 [details]
Patch v1

[Sam Weinig]

Instead of shouldRemovePreviousBackForwardListItemFromList(), I was thinking this should be something more like shouldAddCurrentBackForwardListItemFromList().  Also, does this work for WebBackForwardList::goToItem()?

[Brady Eidson]

(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.

[Darin Adler]

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]

[Brady Eidson]

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"?

[Brady Eidson]

(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.

[Sam Weinig]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 230969 [details]
Patch v3 - Updated the name

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

Followup to the failures was landed right afterwards in http://trac.webkit.org/changeset/168411

[Brady Eidson]

And I even failed to mention the original patch landing in http://trac.webkit.org/changeset/168407