Bug 32052 – Implement HTML5 state object history API

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

HTML5 adds 2 new methods - replaceState() and pushState() - to the History object meant for more powerfully handling back/forward state in AJAX applications.

See http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#history for more details.

WebKit should support these.

Patch forthcoming.

[Brady Eidson]

<rdar://problem/7214236>

[Brady Eidson]

Created attachment 44161 [details]
Fix + load of layout tests

Implements the PopStateEvent, pushState and replaceState, and combines scrolling to fragments with activating state entries into a "load in same document" concept.

Some edge cases are missing but should be handled separately - tracking that work in https://bugs.webkit.org/show_bug.cgi?id=32053

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/loader/HistoryController.cpp:655:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/dom/Document.h:79:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/Document.h:80:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/Document.h:97:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/history/HistoryItem.cpp:402:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/loader/FrameLoader.cpp:3755:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 6

[Brady Eidson]

Created attachment 44163 [details]
Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:79:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/Document.h:80:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/Document.h:97:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/loader/FrameLoader.cpp:3755:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 4

[Brady Eidson]

Based on feedback from John Sullivan, I'm going break the FrameLoaderClient call into 3 calls:

didPushStateWithinPageForFrame
didReplaceStateWithinPageForFrame
didPopStateWithinPageForFrame

That task is largely mechanical and a review on the current patch is still highly relevant in the meantime.

[Sam Weinig]

Comment on attachment 44163 [details]
Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms.

> Index: WebCore/bindings/js/JSHistoryCustom.cpp
> ===================================================================
> --- WebCore/bindings/js/JSHistoryCustom.cpp	(revision 51545)
> +++ WebCore/bindings/js/JSHistoryCustom.cpp	(working copy)
> @@ -163,4 +163,52 @@ void JSHistory::getOwnPropertyNames(Exec
>      Base::getOwnPropertyNames(exec, propertyNames);
>  }
>  
> +JSValue JSHistory::pushState(ExecState* exec, const ArgList& args)
> +{
> +    PassRefPtr<SerializedScriptValue> historyState = SerializedScriptValue::create(exec, args.at(0));

This should be a RefPtr.


> +JSValue JSHistory::replaceState(ExecState* exec, const ArgList& args)
> +{
> +    PassRefPtr<SerializedScriptValue> historyState = SerializedScriptValue::create(exec, args.at(0));

Here too.

> +
> +JSValue JSPopStateEvent::initPopStateEvent(ExecState* exec, const ArgList& args)
> +{
> +    const UString& typeArg = args.at(0).toString(exec);
> +    bool canBubbleArg = args.at(1).toBoolean(exec);
> +    bool cancelableArg = args.at(2).toBoolean(exec);
> +    PassRefPtr<SerializedScriptValue> stateObjectArg = SerializedScriptValue::create(exec, args.at(3));

Here too.


> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> + *
> + */
> +#include "config.h"
> +#include "PopStateEvent.h"

Missing newline.

> @@ -54,12 +55,16 @@ BackForwardList::~BackForwardList()
>  
>  void BackForwardList::addItem(PassRefPtr<HistoryItem> prpItem)
>  {
> +    insertItemAfterCurrent(prpItem, true);
> +}
> +void BackForwardList::insertItemAfterCurrent(PassRefPtr<HistoryItem> prpItem, bool removeForwardList)
> +{

Missing newline.


> +
> +void HistoryItem::setDocument(Document* document)
> +{
> +    if (m_document == document)
> +        return;
> +    
> +    if (m_document)
> +        m_document->unregisterHistoryItem(this);40

40 eh?

> +    if (document)
> +        document->registerHistoryItem(this);
> +        
> +    m_document = document;
> +    
> +}

Extra newline.

>      
> +    void setStateObject(PassRefPtr<SerializedScriptValue> object);
> +    SerializedScriptValue* stateObject() { return m_stateObject.get(); }

This should be const.

> +    void setDocument(Document* document);
> +    Document* document() { return m_document; }

This should be const.

> @@ -209,6 +216,7 @@ private:
>      String m_parent;
>      String m_title;
>      String m_displayTitle;
> +    String m_stateData;

I don't think this is use.

> +    if (hashChange)
> +        if (FrameView* view = m_frame->view())
> +            view->scrollToFragment(m_URL);

The top if needs braces.


> +    
> +    // Check if we'll be using the page cache.  We only use the page cache
> +    // if one exists and it is less than _backForwardCacheExpirationInterval
> +    // seconds old.  If the cache is expired it gets flushed here.
> +    if (RefPtr<CachedPage> cachedPage = pageCache()->get(item)) {
> +            
> +        // FIXME: 1800 should not be hardcoded, it should come from

Extra newline.


> +            case FrameLoadTypeIndexedBackForward:
> +                if (itemURL.protocol() != "https")
> +                    request.setCachePolicy(ReturnCacheDataElseLoad);

This should use protocolIs.


>          [DoNotCheckDomainSecurity] void go(in long distance);
> +        
> +        [DoNotCheckDomainSecurity, Custom] void pushState(in any data, in DOMString title, in optional DOMString url)
> +            raises(DOMException);
> +        [DoNotCheckDomainSecurity, Custom] void replaceState(in any data, in DOMString title, in optional DOMString url)
> +            raises(DOMException);

These two methods should not have DoNotCheckDomainSecurity.


You are missing the definitions of the attribute event listers for onpopstate on HTMLBodyElement and HTMLFrameSetElement.  The latter is also missing the parsedMappedAttribute implementation.

[Adam Barth]

> Same patch, removing a printf() I missed and fixing *some* of the
> style-guideline bot's qualms.

Thanks for addressing some of the qualms.  Sorry for the noise about the namespace indent issue.  Levin just wrote a patch that should quiet that warning in cases where the issue already exists in the file.

[Brady Eidson]

Addressed all of Sam's comments, landed in http://trac.webkit.org/changeset/51644