Bug 71340 – Remove initProgressEvent method

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

This has been removed from the spec:

<http://www.w3.org/TR/progress-events/#interface-progressevent>

It looks like Chromium (PPAPI) uses it, but could change. This fuzzer uses it <http://code.google.com/p/cissrfuzzer> but looks like it is just tracking WebKit for fuzzing. jQuery File Upload <https://github.com/blueimp/jQuery-File-Upload> used to use it, but doesn't on ToT of that project. It shows up in some codesearch results in forks, though.

[Dominic Cooney]

We need new ProgressEvent(…) to work in the V8 bindings before we can update Chromium PPAPI to use the constructor instead of initProgressEvent and kill this method.

[Dominic Cooney]

<http://code.google.com/p/chromium/issues/detail?id=103194> is tracking cleaning up the Chromium side.

[Kaustubh Atrawalkar]

Created attachment 114007 [details]
Patch

Added patch for removing initProgressEvent method.

[WebKit Review Bot]

Comment on attachment 114007 [details]
Patch

Attachment 114007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10370024

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/events/init-events.html

[Kaustubh Atrawalkar]

Created attachment 114015 [details]
Patch

Fixed failing test cases.

[Dominic Cooney]

Comment on attachment 114015 [details]
Patch

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

Looks good to me, although your ChangeLogs need some tweaks.

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

I think this line should go after the bug URL, separated by one blank line, and the commentary about the spec should come next.

> LayoutTests/ChangeLog:15
> +        * fast/xmlhttprequest/xmlhttprequest-get-expected.txt:

You are missing init-events.js, init-events-expected.txt and the platform expectations here; you should update this ChangeLog.

[Dominic Cooney]

Comment on attachment 114015 [details]
Patch

This should not be committed yet or it will break Chromium. This patch has to land first: <http://codereview.chromium.org/8488002/>

[Julien Chaffraix]

Do we have some data as to the popularity of initProgressEvent?

The fact that it is mentioned (and supported) by IE and FF documentations makes me fear about the compatibility with the existing web content.

[Dominic Cooney]

(In reply to comment #8)
> Do we have some data as to the popularity of initProgressEvent?
> 
> The fact that it is mentioned (and supported) by IE and FF documentations makes me fear about the compatibility with the existing web content.

The only indication we have that it is not popular is that we did a Code Search and found Chromium is the only thing indexed by Code Search that was using it (apart from people fuzzing the WebKit API.)

[Kaustubh Atrawalkar]

(In reply to comment #9)
> (In reply to comment #8)
> > Do we have some data as to the popularity of initProgressEvent?
> > 
> > The fact that it is mentioned (and supported) by IE and FF documentations makes me fear about the compatibility with the existing web content.
> 
> The only indication we have that it is not popular is that we did a Code Search and found Chromium is the only thing indexed by Code Search that was using it (apart from people fuzzing the WebKit API.)

The issue seems to be about to fix here in chromium. polina had asked to commit the changes in chromium port. 
http://codereview.chromium.org/8488002/

[Dominic Cooney]

(In reply to comment #10)
> The issue seems to be about to fix here in chromium. polina had asked to commit the changes in chromium port. 
> http://codereview.chromium.org/8488002/

That is right, but please wait. morrita@ will commit the Chromium change, and then we can CQ+ this change. We need the Chromium change to land first or we will break Chromium.

[Kaustubh Atrawalkar]

(In reply to comment #11)
> (In reply to comment #10)
> > The issue seems to be about to fix here in chromium. polina had asked to commit the changes in chromium port. 
> > http://codereview.chromium.org/8488002/
> 
> That is right, but please wait. morrita@ will commit the Chromium change, and then we can CQ+ this change. We need the Chromium change to land first or we will break Chromium.

Surely, I thought just to drop it here.

[Dominic Cooney]

Actually… this still needs a review. Can you upload a patch with an updated changelog per my previous comment so that it is ready for review?

[Kaustubh Atrawalkar]

Created attachment 115108 [details]
Updated Patch

[Kaustubh Atrawalkar]

(In reply to comment #13)
> Actually… this still needs a review. Can you upload a patch with an updated changelog per my previous comment so that it is ready for review?

Surely, I have updated the patch considering your review comments. Thanks.

[Dominic Cooney]

The Chromium side is ready for this now… Just needs review.

[WebKit Review Bot]

Comment on attachment 115108 [details]
Updated Patch

Clearing flags on attachment: 115108

Committed r100727: <http://trac.webkit.org/changeset/100727>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.