Bug 62099 – createEvent("PopStateEvent") raises NOT_SUPPORTED_ERR: DOM Exception 9

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Paul Kinlan]

It is not possible to create a PopStateEvent using:
document.createEvent("PopStateEvent");

It raises a:
Error: NOT_SUPPORTED_ERR: DOM Exception 9

Therefore it is not possible to test applications that respond to History API changes (nav back for example) without invoking a navigation.

I have a fix for this that I should be able to land pretty soon.

[Paul Kinlan]

Created attachment 96033 [details]
A test case for createEvent and dispatch.

[Paul Kinlan]

Created attachment 96036 [details]
My proposed patch and test

Added support for letting developers createEvent("PopStateEvent").
Added tests.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/fast/events/fire..." exit_code: 1

ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Paul Kinlan]

Created attachment 96037 [details]
My proposed patch and test +ChangeLog Fix

Fixing the ChangeLog so that it should pass the style check.

Adds support for createEvent("PopStateEvent") and associated dispatch.  Adds tests.

[Jeremy Orlow]

Comment on attachment 96037 [details]
My proposed patch and test +ChangeLog Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=96037&action=review

Looks pretty good.

> Source/WebCore/dom/PopStateEvent.cpp:34
> +PopStateEvent::PopStateEvent()

You need to call Event's consturctor and initialize m_stateObject

> Source/WebCore/dom/PopStateEvent.h:40
> +    static PassRefPtr<PopStateEvent> create()

Would slightly prefer these factory methods being moved inside the .cpp

> Source/WebCore/dom/PopStateEvent.h:54
> +protected:

Does anythign sub-class this?  If not, put it inside "private:"

[Paul Kinlan]

Created attachment 96053 [details]
Fixes based on feedback from previous review.

[Paul Kinlan]

Comment on attachment 96053 [details]
Fixes based on feedback from previous review. 

(In reply to comment #5)
> (From update of attachment 96037 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96037&action=review
> 
> Looks pretty good.
> 
> > Source/WebCore/dom/PopStateEvent.cpp:34
> > +PopStateEvent::PopStateEvent()
> 
> You need to call Event's consturctor and initialize m_stateObject

Done

> 
> > Source/WebCore/dom/PopStateEvent.h:40
> > +    static PassRefPtr<PopStateEvent> create()

I was following the guidance from another Event, however I then checked some others and it is inconsistent.  I have moved the implementation to PopStateEvent.cpp for both create's.
> 
> Would slightly prefer these factory methods being moved inside the .cpp
> 
> > Source/WebCore/dom/PopStateEvent.h:54
> > +protected:
> 
> Does anythign sub-class this?  If not, put it inside "private:"
Done.  Given that nothing sub-classes this and it is new for this bug fix it makes sense.

[Sam Weinig]

Comment on attachment 96053 [details]
Fixes based on feedback from previous review. 

View in context: https://bugs.webkit.org/attachment.cgi?id=96053&action=review

Otherwise it looks fine.  Lets go another round.

> ChangeLog:13
> +
> +        * ../LayoutTests/fast/events/fire-popstate-event-expected.txt: Added.
> +        * ../LayoutTests/fast/events/fire-popstate-event.html: Added.
> +        * ../Source/WebCore/dom/Document.cpp:
> +        * ../Source/WebCore/dom/PopStateEvent.cpp:
> +        * ../Source/WebCore/dom/PopStateEvent.h:

This ChangeLog should not be here. Please use the ones in /Source/WebCore and /LayouTests

> Source/WebCore/dom/PopStateEvent.cpp:36
> +    , m_stateObject()

This line is not necessary, the construction is implicit.

> Source/WebCore/dom/PopStateEvent.h:41
> +    static PassRefPtr<PopStateEvent> create(PassRefPtr<SerializedScriptValue> stateObject);

I don't think the parameter name adds much here.

[Paul Kinlan]

Created attachment 96094 [details]
Fixes based on feedback from previous review. 

Moved the ChangeLogs to the two individual sections + removed the paramter name from the 2nd create method.

[Jeremy Orlow]

Comment on attachment 96094 [details]
Fixes based on feedback from previous review. 

View in context: https://bugs.webkit.org/attachment.cgi?id=96094&action=review

> Source/WebCore/dom/PopStateEvent.h:-53
> -

This space didn't need to be removed.

[Jeremy Orlow]

r=me

[WebKit Review Bot]

Comment on attachment 96094 [details]
Fixes based on feedback from previous review. 

Clearing flags on attachment: 96094

Committed r88187: <http://trac.webkit.org/changeset/88187>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.