Bug 36335 – Hashchange event is no longer a simple event, needs to be its own interface

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

Spun off from https://bugs.webkit.org/show_bug.cgi?id=36201

The hashchange event is no longer a simple event, needs to be its own interface.  It has two fields so it can be better utilized.

http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-hashchange

[Brady Eidson]

<rdar://problem/7769779>

[Steven Lai]

Created attachment 55643 [details]
Patch

here, defined HashChangeEvent.newURL, HashChangeEvent.oldURL as strings

this fix is for https://bugs.webkit.org/show_bug.cgi?id=38754
so that we don't have to rollback https://bugs.webkit.org/show_bug.cgi?id=36201

[Early Warning System Bot]

Attachment 55643 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2239071

[Brady Eidson]

Comment on attachment 55643 [details]
Patch

Looks fine in code, but can't land without fixing the build breakage failures for other platforms.

I'm wondering if maybe they need the JSHashChangeEvent.cpp added to their build?  (I didn't actually look at the Qt failure that the bots caught)

[Steven Lai]

Created attachment 55667 [details]
Patch

[Steven Lai]

Created attachment 55671 [details]
Patch

[Kent Tamura]

Comment on attachment 55671 [details]
Patch

Looks OK.

[WebKit Commit Bot]

Comment on attachment 55671 [details]
Patch

Rejecting patch 55671 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1
Last 500 characters of output:
lobal-constructors-expected.txt
Hunk #1 succeeded at 113 (offset 5 lines).
patching file LayoutTests/fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html
Hunk #3 FAILED at 61.
Hunk #4 succeeded at 83 (offset 11 lines).
1 out of 4 hunks FAILED -- saving rejects to file LayoutTests/fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html.rej
patching file LayoutTests/fast/loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html

Full output: http://webkit-commit-queue.appspot.com/results/3342068

[Brady Eidson]

Jeez, I didn't know there was a patch here.

I'll notice when the updated one is posted, I promise.  :)

[Eric Seidel (no email)]

Looks like this can't land as is.  Are there plans to update this patch?

[Mihai Parparita]

(In reply to comment #10)
> Looks like this can't land as is.  Are there plans to update this patch?

I'm working on resurrecting the patch.

[Mihai Parparita]

Created attachment 66942 [details]
Patch

[Mihai Parparita]

(In reply to comment #9)
> I'll notice when the updated one is posted, I promise.  :)

Brady, I'll hold you to this, but could you not actually flip the review bit till the EWS bots have had a chance to run? It looks like there were cross-platform troubles with past versions of this patch.

[WebKit Review Bot]

Attachment 66942 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3924289

[Mihai Parparita]

Created attachment 66996 [details]
Patch

[Mihai Parparita]

New version of the patch should hopefully build with GTK (as of http://trac.webkit.org/changeset/63324 derived sources have to be listed in GNUmakefile.am).

[Mihai Parparita]

New version of the patch seems to build on GTK (and other platforms). The Windows EWS bot rejected it because it couldn't apply the patch, but based on https://webkit-commit-queue.appspot.com/queue-status/win-ews it seems to do that a lot, so I'm not going to read too much into it.

Brady, mind r+/cq+-ing this?

[Brady Eidson]

(In reply to comment #17)
> New version of the patch seems to build on GTK (and other platforms). The Windows EWS bot rejected it because it couldn't apply the patch, but based on https://webkit-commit-queue.appspot.com/queue-status/win-ews it seems to do that a lot, so I'm not going to read too much into it.
> 
> Brady, mind r+/cq+-ing this?

T'was on vacation till this morning, taking a look shortly.

[WebKit Commit Bot]

Comment on attachment 66996 [details]
Patch

Rejecting patch 66996 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Brady Eidson', u'--force']" exit_code: 1
Last 500 characters of output:
23018.
2 out of 7 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file WebCore/bindings/js/JSEventCustom.cpp
patching file WebCore/bindings/v8/custom/V8EventCustom.cpp
patching file WebCore/dom/Document.cpp
Hunk #2 succeeded at 4622 (offset -31 lines).
patching file WebCore/dom/Event.cpp
patching file WebCore/dom/Event.h
patching file WebCore/dom/HashChangeEvent.h
patching file WebCore/dom/HashChangeEvent.idl
patching file WebCore/page/DOMWindow.idl

Full output: http://queues.webkit.org/results/3921439

[Mihai Parparita]

Created attachment 67492 [details]
Patch

[Mihai Parparita]

I've uploaded a new version of the patch that should merge cleanly. Brady (or Darin), would you mind landing this for me, so that it doesn't have to sit in the commit queue for hours and risk getting a merge failure again?

[Mihai Parparita]

Created attachment 68158 [details]
Patch

[Mihai Parparita]

Even newer version of the patch that should merge cleanly. I can land this myself now (got SVN access today) if I can get another r+.

[Dimitri Glazkov (Google)]

Comment on attachment 68158 [details]
Patch

ok.

[Mihai Parparita]

Comment on attachment 68158 [details]
Patch

Clearing flags on attachment: 68158

Committed r67898: <http://trac.webkit.org/changeset/67898>

[Mihai Parparita]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67898 might have broken Qt Linux Release

[Mihai Parparita]

(In reply to comment #27)
> http://trac.webkit.org/changeset/67898 might have broken Qt Linux Release

It broke Windows too (attempted to fix that with http://trac.webkit.org/changeset/67901).

The Qt failures look like Qt expectations that need to be updated (they have their own expectations of the tests that this patch touched). I'll attempt to rebaseline.

[Mihai Parparita]

(In reply to comment #28)
> It broke Windows too (attempted to fix that with http://trac.webkit.org/changeset/67901).

Windows build is fixed.
 
> The Qt failures look like Qt expectations that need to be updated (they have their own expectations of the tests that this patch touched). I'll attempt to rebaseline.

GTK and Qt are rebaselined with http://trac.webkit.org/changeset/67907. Windows may need a rebaseline too, but the bot is really behind.