Bug 36335

Summary: Hashchange event is no longer a simple event, needs to be its own interface
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, fishd, gns, mihai, s3lance, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
beidson: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2010-03-18 16:26:50 PDT
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
Comment 1 Brady Eidson 2010-03-18 16:27:21 PDT
<rdar://problem/7769779>
Comment 2 Steven Lai 2010-05-10 19:18:18 PDT
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
Comment 3 Early Warning System Bot 2010-05-10 19:30:44 PDT
Attachment 55643 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2239071
Comment 4 Brady Eidson 2010-05-10 22:33:53 PDT
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)
Comment 5 Steven Lai 2010-05-10 23:01:47 PDT
Created attachment 55667 [details]
Patch
Comment 6 Steven Lai 2010-05-10 23:53:37 PDT
Created attachment 55671 [details]
Patch
Comment 7 Kent Tamura 2010-06-19 09:37:53 PDT
Comment on attachment 55671 [details]
Patch

Looks OK.
Comment 8 WebKit Commit Bot 2010-06-19 14:22:31 PDT
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
Comment 9 Brady Eidson 2010-06-19 18:21:27 PDT
Jeez, I didn't know there was a patch here.

I'll notice when the updated one is posted, I promise.  :)
Comment 10 Eric Seidel (no email) 2010-09-02 13:55:19 PDT
Looks like this can't land as is.  Are there plans to update this patch?
Comment 11 Mihai Parparita 2010-09-08 13:10:12 PDT
(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.
Comment 12 Mihai Parparita 2010-09-08 14:27:48 PDT
Created attachment 66942 [details]
Patch
Comment 13 Mihai Parparita 2010-09-08 14:30:31 PDT
(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.
Comment 14 WebKit Review Bot 2010-09-08 17:27:34 PDT
Attachment 66942 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3924289
Comment 15 Mihai Parparita 2010-09-08 22:20:52 PDT
Created attachment 66996 [details]
Patch
Comment 16 Mihai Parparita 2010-09-08 22:22:20 PDT
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).
Comment 17 Mihai Parparita 2010-09-09 16:41:05 PDT
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?
Comment 18 Brady Eidson 2010-09-13 11:20:53 PDT
(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.
Comment 19 WebKit Commit Bot 2010-09-13 15:30:47 PDT
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
Comment 20 Mihai Parparita 2010-09-13 17:21:01 PDT
Created attachment 67492 [details]
Patch
Comment 21 Mihai Parparita 2010-09-13 17:22:50 PDT
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?
Comment 22 Mihai Parparita 2010-09-20 17:01:14 PDT
Created attachment 68158 [details]
Patch
Comment 23 Mihai Parparita 2010-09-20 17:03:24 PDT
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+.
Comment 24 Dimitri Glazkov (Google) 2010-09-20 17:12:27 PDT
Comment on attachment 68158 [details]
Patch

ok.
Comment 25 Mihai Parparita 2010-09-20 17:22:25 PDT
Comment on attachment 68158 [details]
Patch

Clearing flags on attachment: 68158

Committed r67898: <http://trac.webkit.org/changeset/67898>
Comment 26 Mihai Parparita 2010-09-20 17:22:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 WebKit Review Bot 2010-09-20 17:48:39 PDT
http://trac.webkit.org/changeset/67898 might have broken Qt Linux Release
Comment 28 Mihai Parparita 2010-09-20 17:50:18 PDT
(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.
Comment 29 Mihai Parparita 2010-09-20 18:41:40 PDT
(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.