Bug 71340

Summary: Remove initProgressEvent method
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: DOMAssignee: Dominic Cooney <dominicc>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, jchaffraix, kaustubh.ra, morrita, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67980    
Bug Blocks: 68791    
Attachments:
Description Flags
Patch
webkit.review.bot: commit-queue-
Patch
dominicc: commit-queue-
Updated Patch none

Dominic Cooney
Reported 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.
Attachments
Patch (6.73 KB, patch)
2011-11-08 00:54 PST, Kaustubh Atrawalkar 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Patch (12.37 KB, patch)
2011-11-08 01:56 PST, Kaustubh Atrawalkar 		dominicc: commit-queue-
DetailsFormatted DiffDiff
Updated Patch (12.71 KB, patch)
2011-11-14 23:41 PST, Kaustubh Atrawalkar 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dominic Cooney
Comment 1 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.
Dominic Cooney
Comment 2 2011-11-06 13:55:12 PST
<http://code.google.com/p/chromium/issues/detail?id=103194> is tracking cleaning up the Chromium side.
Kaustubh Atrawalkar
Comment 3 2011-11-08 00:54:13 PST
Created attachment 114007 [details] Patch Added patch for removing initProgressEvent method.
WebKit Review Bot
Comment 4 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
Kaustubh Atrawalkar
Comment 5 2011-11-08 01:56:06 PST
Created attachment 114015 [details] Patch Fixed failing test cases.
Dominic Cooney
Comment 6 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.
Dominic Cooney
Comment 7 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/>
Julien Chaffraix
Comment 8 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.
Dominic Cooney
Comment 9 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.)
Kaustubh Atrawalkar
Comment 10 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/
Dominic Cooney
Comment 11 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.
Kaustubh Atrawalkar
Comment 12 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.
Dominic Cooney
Comment 13 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?
Kaustubh Atrawalkar
Comment 14 2011-11-14 23:41:46 PST
Created attachment 115108 [details] Updated Patch
Kaustubh Atrawalkar
Comment 15 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.
Dominic Cooney
Comment 16 2011-11-17 22:11:24 PST
The Chromium side is ready for this now… Just needs review.
WebKit Review Bot
Comment 17 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>
WebKit Review Bot
Comment 18 2011-11-17 22:57:26 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.