Bug 32052 - Implement HTML5 state object history API
: Implement HTML5 state object history API
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 32053
  Show dependency treegraph
 
Reported: 2009-12-01 21:06 PST by
Modified: 2009-12-03 11:06 PST (History)


Attachments
Fix + load of layout tests (138.70 KB, patch)
2009-12-02 10:58 PST, Brady Eidson
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms. (138.58 KB, patch)
2009-12-02 11:08 PST, Brady Eidson
sam: review+
beidson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-12-01 21:06:59 PST
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.
------- Comment #1 From 2009-12-01 21:07:25 PST -------
<rdar://problem/7214236>
------- Comment #2 From 2009-12-02 10:58:48 PST -------
Created an attachment (id=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
------- Comment #3 From 2009-12-02 11:00:15 PST -------
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
------- Comment #4 From 2009-12-02 11:08:52 PST -------
Created an attachment (id=44163) [details]
Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms.
------- Comment #5 From 2009-12-02 11:11:14 PST -------
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
------- Comment #6 From 2009-12-02 16:34:13 PST -------
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.
------- Comment #7 From 2009-12-02 19:37:53 PST -------
(From update of attachment 44163 [details])
> 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.
------- Comment #8 From 2009-12-02 21:31:04 PST -------
> 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.
------- Comment #9 From 2009-12-03 11:06:00 PST -------
Addressed all of Sam's comments, landed in http://trac.webkit.org/changeset/51644