Bug 71340 - Remove initProgressEvent method
Summary: Remove initProgressEvent method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Cooney
URL:
Keywords:
Depends on: 67980
Blocks: 68791
  Show dependency treegraph
 
Reported: 2011-11-01 17:19 PDT by Dominic Cooney
Modified: 2011-11-17 22:57 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.73 KB, patch)
2011-11-08 00:54 PST, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (12.37 KB, patch)
2011-11-08 01:56 PST, Kaustubh Atrawalkar
dominicc: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (12.71 KB, patch)
2011-11-14 23:41 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-11-01 17:19:00 PDT
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.
Comment 1 Dominic Cooney 2011-11-05 17:40:57 PDT
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.
Comment 2 Dominic Cooney 2011-11-06 13:55:12 PST
<http://code.google.com/p/chromium/issues/detail?id=103194> is tracking cleaning up the Chromium side.
Comment 3 Kaustubh Atrawalkar 2011-11-08 00:54:13 PST
Created attachment 114007 [details]
Patch

Added patch for removing initProgressEvent method.
Comment 4 WebKit Review Bot 2011-11-08 01:28:50 PST
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
Comment 5 Kaustubh Atrawalkar 2011-11-08 01:56:06 PST
Created attachment 114015 [details]
Patch

Fixed failing test cases.
Comment 6 Dominic Cooney 2011-11-08 06:46:59 PST
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.
Comment 7 Dominic Cooney 2011-11-08 06:50:23 PST
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/>
Comment 8 Julien Chaffraix 2011-11-09 10:47:23 PST
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.
Comment 9 Dominic Cooney 2011-11-09 12:27:44 PST
(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.)
Comment 10 Kaustubh Atrawalkar 2011-11-14 21:55:31 PST
(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/
Comment 11 Dominic Cooney 2011-11-14 22:17:09 PST
(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.
Comment 12 Kaustubh Atrawalkar 2011-11-14 22:30:39 PST
(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.
Comment 13 Dominic Cooney 2011-11-14 23:03:47 PST
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?
Comment 14 Kaustubh Atrawalkar 2011-11-14 23:41:46 PST
Created attachment 115108 [details]
Updated Patch
Comment 15 Kaustubh Atrawalkar 2011-11-14 23:47:05 PST
(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.
Comment 16 Dominic Cooney 2011-11-17 22:11:24 PST
The Chromium side is ready for this now… Just needs review.
Comment 17 WebKit Review Bot 2011-11-17 22:57:20 PST
Comment on attachment 115108 [details]
Updated Patch

Clearing flags on attachment: 115108

Committed r100727: <http://trac.webkit.org/changeset/100727>
Comment 18 WebKit Review Bot 2011-11-17 22:57:26 PST
All reviewed patches have been landed.  Closing bug.