Summary: | Remove initProgressEvent method | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominic Cooney <dominicc> | ||||||||
Component: | DOM | Assignee: | 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
Dominic Cooney
2011-11-01 17:19:00 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. <http://code.google.com/p/chromium/issues/detail?id=103194> is tracking cleaning up the Chromium side. Created attachment 114007 [details]
Patch
Added patch for removing initProgressEvent method.
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 Created attachment 114015 [details]
Patch
Fixed failing test cases.
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 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/> 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. (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.) (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/ (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. (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. 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? Created attachment 115108 [details]
Updated Patch
(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. The Chromium side is ready for this now… Just needs review. Comment on attachment 115108 [details] Updated Patch Clearing flags on attachment: 115108 Committed r100727: <http://trac.webkit.org/changeset/100727> All reviewed patches have been landed. Closing bug. |