Bug 32052 - Implement HTML5 state object history API
: Implement HTML5 state object history API
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Brady Eidson
: InRadar
Depends on:
Blocks: 32053
  Show dependency treegraph
 
Reported: 2009-12-01 21:06 PST by Brady Eidson
Modified: 2009-12-03 11:06 PST (History)
7 users (show)

See Also:


Attachments
Fix + load of layout tests (138.70 KB, patch)
2009-12-02 10:58 PST, Brady Eidson
beidson: commit‑queue-
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-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 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 Brady Eidson 2009-12-01 21:07:25 PST
<rdar://problem/7214236>
Comment 2 Brady Eidson 2009-12-02 10:58:48 PST
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
Comment 3 WebKit Review Bot 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 Brady Eidson 2009-12-02 11:08:52 PST
Created attachment 44163 [details]
Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms.
Comment 5 WebKit Review Bot 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 Brady Eidson 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 Sam Weinig 2009-12-02 19:37:53 PST
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.
Comment 8 Adam Barth 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 Brady Eidson 2009-12-03 11:06:00 PST
Addressed all of Sam's comments, landed in http://trac.webkit.org/changeset/51644