Bug 114444 – ProgressEvent should not be cancelable

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

[Praveen Jadhav]

Events in FileAPI http://dev.w3.org/2006/webapi/FileAPI/#events specify that the events are NOT cancelable and the value in ProgressEvent must be false. Currently these is no provision to set canBubble and cancelable while creating ProgressEvent.

[Praveen Jadhav]

Created attachment 197591 [details]
Patch

[Soo-Hyun Choi]

FYI - changed the assignee and status of this bug.

[Alexey Proskuryakov]

Should progress events ever be cancelable? The progress events spec says something I can't easily decipher:

"Throughout the web platform the error, abort, and load event types have their bubbles and cancelable attributes initialized to false, so it is suggested that for consistency all events using the ProgressEvent interface do the same."

[Anne van Kesteren]

It is up to the specification dispatching the event to decide that, basically.

[Chris Dumez]

Comment on attachment 197591 [details]
Patch

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

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

"Reviewed by" line usually comes after the bug URL.

> LayoutTests/fast/files/file-reader-event-listener.html:31
> +    if (event.bubbles) {

You should probably use shouldBeFalse() instead.

> LayoutTests/fast/files/file-reader-event-listener.html:35
> +    if (event.cancelable) {

Ditto.

[Praveen Jadhav]

(In reply to comment #4)
> It is up to the specification dispatching the event to decide that, basically.

Going by ProgressEvent specifications, default value of "cancelable" for error, abort and load events should be false. So, is it fine if I set that to false in ProgressEvent.h?

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #4)
> > It is up to the specification dispatching the event to decide that, basically.
> 
> Going by ProgressEvent specifications, default value of "cancelable" for error, abort and load events should be false. So, is it fine if I set that to false in ProgressEvent.h?

Based on the spec and what Anne said, I personally think it is a good idea to set cancelable argument's default value to false, instead of true for ProgressEvent.

[Praveen Jadhav]

Created attachment 198019 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 198019 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        ProgressEvent should provide provision to receive canBubble and cancelable values.

Please update ChangeLog to match current bug title.

> Source/WebCore/dom/ProgressEvent.cpp:56
> +ProgressEvent::ProgressEvent(const AtomicString& type, bool lengthComputable, unsigned long long loaded, unsigned long long total, bool canBubble, bool cancelable)
> +    : Event(type, canBubble, cancelable)

If we never need other values, why make the constructor slower by making this configurable?

> LayoutTests/fast/files/file-reader-event-listener.html:6
> +<script src="../js/resources/js-test-pre.js"></script>

Please use the rest of js-test functionality here (window.jsTestIsAsync, finishJSTest()). testRunner.dumpAsText() is the default when using js-test.

[Praveen Jadhav]

(In reply to comment #9)
> (From update of attachment 198019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198019&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        ProgressEvent should provide provision to receive canBubble and cancelable values.
> 
> Please update ChangeLog to match current bug title.

I will update this in the next patch.

> 
> > Source/WebCore/dom/ProgressEvent.cpp:56
> > +ProgressEvent::ProgressEvent(const AtomicString& type, bool lengthComputable, unsigned long long loaded, unsigned long long total, bool canBubble, bool cancelable)
> > +    : Event(type, canBubble, cancelable)
> 
> If we never need other values, why make the constructor slower by making this configurable?

I checked other specifications which are using ProgressEvent and found that in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable. Here, except for readystatechange, all are ProgressEvents. Maybe, we should retain the code as in first patch, along with setting default value while declaration, in order to support XMLHttpRequest. Let me know your suggestion.

> 
> > LayoutTests/fast/files/file-reader-event-listener.html:6
> > +<script src="../js/resources/js-test-pre.js"></script>
> 
> Please use the rest of js-test functionality here (window.jsTestIsAsync, finishJSTest()). testRunner.dumpAsText() is the default when using js-test.

I will update this in the next patch.

[Alexey Proskuryakov]

> in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable.

I could not easily find this in the spec. Could you please quote it? Does XHR 2 say the same?

If this patch used to break XHR, we should add a test tat would have caught that.

[Praveen Jadhav]

(In reply to comment #11)
> > in XMLHTTPRequest, http://www.w3.org/TR/XMLHttpRequest/#garbage-collection, all events must be cancelable.
> 
> I could not easily find this in the spec. Could you please quote it? Does XHR 2 say the same?
> 
> If this patch used to break XHR, we should add a test tat would have caught that.

There is a layout test http/tests/xmlhttprequest/infoOnProgressEvent.html which checks if event.cancelable is true. Strangely, it din't fail on efl port(not blocked by TestExpectations either). But Mac bot seems to have caught the error.

Specifications doesn't mention anything specific to cancelable in XHR and maybe my interpretation was wrong about canceling fetch algorithm and making ProgressEvents of XHR cancelable.

[Praveen Jadhav]

Created attachment 198036 [details]
Patch

All comments are included in the patch. infoOnProgressEvent-expected.txt is modified to receive corrected state of Cancelable.

[Anne van Kesteren]

FYI, none of the events in http://xhr.spec.whatwg.org/ are cancelable. preventDefault() certainly never had an effect.

[Build Bot]

Comment on attachment 198036 [details]
Patch

Attachment 198036 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/10265

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/ssl/ping-with-unsafe-redirect.html

[Build Bot]

Created attachment 198045 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 198036 [details]
Patch

Attachment 198036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/42423

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[Build Bot]

Created attachment 198171 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Praveen Jadhav]

Created attachment 198221 [details]
Patch

Failed test expectation files are updated. Bug title updated to be more generic.

[Praveen Jadhav]

Created attachment 198222 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 198222 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface

Please refer to a specific version of the spec.

> Source/WebCore/ChangeLog:12
> +        No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated.

It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect).

> LayoutTests/ChangeLog:9
> +        file-reader-event-listener.html, xmlhttprequest-get-expected.txt and infoOnProgressEvent-expected.txt are updated.
> +

Did we used to allow these events to be cancelled? If so, how important is it to maintain the backward compatibility?

[Praveen Jadhav]

(In reply to comment #21)
> (From update of attachment 198222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198222&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface

ok. I will update this in the next patch.

> 
> Please refer to a specific version of the spec.
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated.
> 
> It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect).

The test cases are updated to check 'cancelable' value. I feel that covers all possible scenarios.

> 
> > LayoutTests/ChangeLog:9
> > +        file-reader-event-listener.html, xmlhttprequest-get-expected.txt and infoOnProgressEvent-expected.txt are updated.
> > +
> 
> Did we used to allow these events to be cancelled? If so, how important is it to maintain the backward compatibility?

ProgressEvent spec released in April 2007(http://www.w3.org/TR/2007/WD-progress-events-20070419/) says "These events must not bubble, and must be cancelable.". Around the same time, these test cases too were added. After that, 5 more versions were released starting from October 2007(http://www.w3.org/TR/2007/WD-progress-events-20071023/) and all of them recommand to set 'cancelable' parameter to FALSE. However, the test cases were not updated following the spec changes.
ProgressEvent specification hasn't changed 'cancelable' default value for a long time now and hence backward compatibility shouldn't be an issue, IMO.

[Ryosuke Niwa]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 198222 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=198222&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Spec: https://dvcs.w3.org/hg/progress/raw-file/tip/Overview.html#suggested-names-for-events-using-the-progressevent-interface
> 
> ok. I will update this in the next patch.
> 
> > 
> > Please refer to a specific version of the spec.
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        No new tests. file-reader-event-listener.html and infoOnProgressEvent-expected.txt are updated.
> > 
> > It appears to me that we also need to add tests to ensure these events are indeed NOT cancelable (i.e. canceling event shouldn’t have any effect).
> 
> The test cases are updated to check 'cancelable' value. I feel that covers all possible scenarios.

It just tells us that the 'cancelable' returns the right value. It doesn't tell us that the event is not cancelable.

[Praveen Jadhav]

Created attachment 203310 [details]
Patch

event.preventDefault() is called to update 'canceled flag' of the event after 'cancelable' is set to FALSE.

[WebKit Commit Bot]

Comment on attachment 203310 [details]
Patch

Clearing flags on attachment: 203310

Committed r150949: <http://trac.webkit.org/changeset/150949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.