Bug 46544 – Fix event sequencing in FileWriter

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

[Eric U.]

There are a couple of problems there.  Some of the progress events are synchronous, and should really be asynchronous.  The variable readyState is set too early [see info at http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/1045.html].

[Eric U.]

Created attachment 68796 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 68796 [details]
Patch

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

LGTM.

> WebCore/fileapi/FileWriter.cpp:172
> +    fireEvent(eventNames().writestartEvent);

hmm... so we fire all of three events when it's finished. ok (seems like it conforms to the spec).

[Adam Barth]

Comment on attachment 68796 [details]
Patch

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

We really need testing for this code / these patches.

> WebCore/ChangeLog:13
> +        No new tests, as none of this is fully implemented yet.

What does that mean?  It's too bad we can't test these patches.

[Eric U.]

(In reply to comment #3)
> (From update of attachment 68796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review
> 
> We really need testing for this code / these patches.
> 
> > WebCore/ChangeLog:13
> > +        No new tests, as none of this is fully implemented yet.
> 
> What does that mean?  It's too bad we can't test these patches.

It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature.  None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work.

It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon].  That's the webkit way of doing the testing, right?

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 68796 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review
> > 
> > We really need testing for this code / these patches.
> > 
> > > WebCore/ChangeLog:13
> > > +        No new tests, as none of this is fully implemented yet.
> > 
> > What does that mean?  It's too bad we can't test these patches.
> 
> It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature.  None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work.
> 
> It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon].  That's the webkit way of doing the testing, right?

It is sort of. However with complicated features it is easy to do some minimal testing at the end (but it would have been much greater if the testing had been added as the code went along -- I'm guilty here as well).

Is it possible to add a disabled test that would verify this if the functionality were working?

[Eric U.]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 68796 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=68796&action=review
> > > 
> > > We really need testing for this code / these patches.
> > > 
> > > > WebCore/ChangeLog:13
> > > > +        No new tests, as none of this is fully implemented yet.
> > > 
> > > What does that mean?  It's too bad we can't test these patches.
> > 
> > It means that I'm tweaking code that makes up only part of a feature, and we can't really test it until I put in the rest of the feature.  None of this is exposed yet, and it needs an implementation of AsyncFileWriter to do most of its work.
> > 
> > It could theoretically be unit-tested by making a mock AsyncFileWriter backend, but I was planning just to use layout tests once I got the real thing in [which should be quite soon].  That's the webkit way of doing the testing, right?
> 
> It is sort of. However with complicated features it is easy to do some minimal testing at the end (but it would have been much greater if the testing had been added as the code went along -- I'm guilty here as well).
> 
> Is it possible to add a disabled test that would verify this if the functionality were working?

Well, of course it's possible ;'>.  The API is defined in the spec, so I could write any number of FileWriter and FileSystem layout tests based on that and disable them all.  But then we'd have a bunch of code that wasn't getting run, and could rot by the time it was ready to be turned on.  These APIs are unfortunately changing as we implement them and run into bits that don't quite work in the spec.  I think it may be premature to write the tests before we have even a single port implementing them.

I expect to have both the Chromium and WebCore ports implemented within the next couple of months, though, and the Chromium one will have at least part of its functionality within a couple of weeks [for M8].  How would you feel about me adding the first test once I've got one port that could run it?

[Eric U.]

Comment on attachment 68796 [details]
Patch

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

>> WebCore/fileapi/FileWriter.cpp:172
>> +    fireEvent(eventNames().writestartEvent);
> 
> hmm... so we fire all of three events when it's finished. ok (seems like it conforms to the spec).

Yeah, a truncate is sort of an all-at-once operation, so as soon as we know it's started, it's done.

[WebKit Commit Bot]

Comment on attachment 68796 [details]
Patch

Rejecting patch 68796 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
th/git/webkit-queue/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py", line 501, in start_websocket_server
    self._websocket_server.start()
  File "/Users/abarth/git/webkit-queue/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py", line 219, in start
    (self._server_name, self._port))
PyWebSocketNotStarted: Failed to start PyWebSocket server on port 8880.

----------------------------------------------------------------------
Ran 607 tests in 25.087s

FAILED (errors=1)

Full output: http://queues.webkit.org/results/4042144

[WebKit Commit Bot]

Comment on attachment 68796 [details]
Patch

Clearing flags on attachment: 68796

Committed r68483: <http://trac.webkit.org/changeset/68483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.