Remove FileError interface

Remove FileError interface

FileError was removed from File API [1] after Chrome had shipped the
functionality. Other browsers do not use it, returning a DOMError
instead. Using the `code` property has been logging deprecation
warnings since August 2013. The affected APIs will now yield
DOMExceptions instead (as DOMError is being removed from the platform
[2]). Script should inspect the `name` and `message` properties of the
error objects.

This affects:
* FileReader.error
* FileWriter.error
* FileSystem async API: passed to ErrorCallback on failure
* FileSystem sync API: thrown on failure

Intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kJMa2rpAYqI
Dashboard: https://www.chromestatus.com/feature/6687420359639040

[1] https://w3c.github.io/FileAPI/
[2] https://dom.spec.whatwg.org/#domerror

BUG=496901
R=foolip@chromium.org

Committed: https://crrev.com/ef3e334a185b82752f606768ecd3f4c8083b7275
Cr-Commit-Position: refs/heads/master@{#406169}

[jsbell, 2016-06-03 21:41:59]

Initial CL for feedback on this approach.

I tried removing FileError entirely, but the bindings layer mints DOMExceptions
lazily (e.g. deep within  ExceptionState::throwDOMException) and the sync
file/system APIs do many indirection tricks where throw vs. callback is wrangled
by helper classes. It turned into a huge intrusive change.

I have not yet tried replacing the FileError class with e.g. a namespace and
passing FileError::ErrorCode, but that seems like a possible follow-on.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fileapi/FileError.cpp (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fileapi/FileError.cpp:144: // TODO(jsbell):
Special case SecurityError?
I believe this is safe as it's only used in places that would have minted
FileError/DOMError instances before with just the same information, and the
message is completely generic anyway.

I could remove the special case entirely.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fileapi/FileReader.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fileapi/FileReader.idl:61: [Measure] readonly
attribute DOMException? error;
Should probably have a TODO here explaining why it's not a DOMError and pointing
to a FileAPI issue.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/DOMFileSystemSync.cpp (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/DOMFileSystemSync.cpp:189: void
handleEvent(DOMException* error) override
This pattern is repeated. I wasn't sure if having an intermediate subclass of
ErrorCallback was worth it, though.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/FileWriter.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/FileWriter.idl:50: readonly
attribute DOMException? error;
Gains a ?, but this doesn't exist in a non-abandoned spec any more. :(

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:8:
#include "core/dom/DOMException.h"
This could be in PresentationError.h instead.

[jsbell, 2016-06-07 22:58:16]

jsbell@chromium.org changed reviewers:
+ pfeldman@chromium.org

[jsbell, 2016-06-07 22:58:16]

pfeldman@ - can you take a look at the devtools changes?

I have low confidence in my changes, but no tests fail...

[pfeldman, 2016-06-07 23:19:52]

Unfortunately, that won't fix the front-ends retrospectively, so this change is
not sufficient.
In addition to these changes, you would need to update the
installBackwardsCompatibility function in devtools.js that would install the
polyfills for the APIs that you remove.

[foolip, 2016-06-08 10:04:44]

Looks good and makes sense. Just want to make sure I understand what big mess is
caused by trying to do the full removal at once. FWIW, FileError now becomes a
bit like MediaErrorState, although I don't think pursuing any unification is
worthwhile.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fileapi/FileReader.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fileapi/FileReader.idl:61: [Measure] readonly
attribute DOMException? error;
On 2016/06/03 21:41:58, jsbell wrote:
> Should probably have a TODO here explaining why it's not a DOMError and
pointing
> to a FileAPI issue.

Yep. And I guess remove [Measure] if this is the expected final form of this
API?

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/FileWriter.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/FileWriter.idl:50: readonly
attribute DOMException? error;
On 2016/06/03 21:41:59, jsbell wrote:
> Gains a ?, but this doesn't exist in a non-abandoned spec any more. :(

It could already return null, right? For readonly attributes and method return
values, making them nullable doesn't change the generated code, so it's
basically only documentation, which is good.

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/imported/wpt/FileAPI/historical-expected.txt
(right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt/FileAPI/historical-expected.txt:3:
PASS "FileError" should not be supported
This is my favorite line.

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/FileError.cpp (right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.cpp:146: return
DOMException::create(SecurityError, FileError::securityErrorMessage);
I don't follow, AFAICT the case below will do the same thing, errorCodeTo* seem
to handle SECURITY_ERR?

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.h:50: // TODO(jsbell):
Eliminate this class, as it contains just one member.
What would it take to actually remove it?

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.h:51: class CORE_EXPORT
FileError final : public GarbageCollected<FileError> {
Why does this need to be on the Oilpan heap? Can't it just be a simple copyable
type?

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/FileError.idl (left):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.idl:47: [DeprecateAs=FileError]
readonly attribute unsigned short code;
Also remove the entry in UseCounter.h and the deprecation message.

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:34: #include
"core/dom/DOMException.h"
Why these added includes?

[jsbell, 2016-06-08 17:16:42]

On 2016/06/07 23:19:52, pfeldman wrote:
> Unfortunately, that won't fix the front-ends retrospectively, so this change
is
> not sufficient.
> In addition to these changes, you would need to update the
> installBackwardsCompatibility function in devtools.js that would install the
> polyfills for the APIs that you remove.

Just to confirm: this enables old frontend JS code to run in new Chrome? So
something like:

// Support for legacy (<M52) frontends.

// FileError is removed; DOMException will be seen instead, with different
codes.
// Only cases expected by old devtools need to be mapped.
window.FileError = window.FileError || {
  NOT_FOUND_ERR: DOMException.NOT_FOUND_ERR,
  ABORT_ERR: DOMException.ABORT_ERR,
  INVALID_MODIFICATION_ERR: DOMException.INVALID_MODIFICATION_ERR,
  NOT_READABLE_ERR: 0  // No matching DOMException code, so e.code will be 0
};

The last entry is a bit odd. To explain: previously the APIs would generate a
"NotReadableError" FileError w/ code === FileError.NOT_READABLE_ERR. Now they
will generate a "NotReadableError" DOMException w/ code = 0. So the above should
allow the switch statement in devtools/front_end/timeline/TimelineLoader.js to
work, although possibly other error cases (w/o matching DOMException codes, so
code === 0) would fall into that case rather than the default, but that seems
harmless.

Am I understanding correctly?

[jsbell, 2016-06-08 17:27:43]

Thanks for the initial look. I'll see about trying to replace the GC'd class
with a namespace/enum before asking for further reviews.

(The big mess I ran into was trying to replace FileError entirely with eagerly
created DOMException, which led to much tangled bindings code.)

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fileapi/FileReader.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fileapi/FileReader.idl:61: [Measure] readonly
attribute DOMException? error;
On 2016/06/08 10:04:44, Philip Jägenstedt wrote:
> On 2016/06/03 21:41:58, jsbell wrote:
> > Should probably have a TODO here explaining why it's not a DOMError and
> pointing
> > to a FileAPI issue.
> 
> Yep. And I guess remove [Measure] if this is the expected final form of this
> API?

Will do.

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/FileWriter.idl (right):

https://codereview.chromium.org/2040563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/FileWriter.idl:50: readonly
attribute DOMException? error;
On 2016/06/08 10:04:44, Philip Jägenstedt wrote:
> On 2016/06/03 21:41:59, jsbell wrote:
> > Gains a ?, but this doesn't exist in a non-abandoned spec any more. :(
> 
> It could already return null, right? For readonly attributes and method return
> values, making them nullable doesn't change the generated code, so it's
> basically only documentation, which is good.

Correct, only documentation.

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/FileError.cpp (right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.cpp:146: return
DOMException::create(SecurityError, FileError::securityErrorMessage);
On 2016/06/08 10:04:44, Philip Jägenstedt wrote:
> I don't follow, AFAICT the case below will do the same thing, errorCodeTo*
seem
> to handle SECURITY_ERR?

Correct. Note the special case above in throwDOMException. I left this in for
now to attract attention during review. If we agree we don't need to do anything
special here I'll remove it.

I will probably leave a comment about ensuring that the message of a
SecurityError is appropriately sanitized.

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:34: #include
"core/dom/DOMException.h"
On 2016/06/08 10:04:44, Philip Jägenstedt wrote:
> Why these added includes?

"To fix compile errors" is the short answer. :) These seemingly unrelated files
are using DOMException and were previously getting the includes indirectly.

[jsbell, 2016-06-08 17:28:38]

Oh, and don't expect further updates here 'til after BlinkOn (so ~2 weeks).
Thank you both pfeldman@ and foolip@ for guidance so far!

[foolip, 2016-06-08 21:07:56]

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/FileError.cpp (right):

https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/FileError.cpp:146: return
DOMException::create(SecurityError, FileError::securityErrorMessage);
On 2016/06/08 17:27:42, jsbell wrote:
> On 2016/06/08 10:04:44, Philip Jägenstedt wrote:
> > I don't follow, AFAICT the case below will do the same thing, errorCodeTo*
> seem
> > to handle SECURITY_ERR?
> 
> Correct. Note the special case above in throwDOMException. I left this in for
> now to attract attention during review. If we agree we don't need to do
anything
> special here I'll remove it.
> 
> I will probably leave a comment about ensuring that the message of a
> SecurityError is appropriately sanitized.

Oh. Above it must be because throwSecurityError does something more/different
than throwDOMException, but at a quick glance I can't see what it is. If
unsanitizedMessage is used anywhere I'm not finding it in code search, so please
teach me if you know :)

[pfeldman, 2016-06-08 21:11:09]

On 2016/06/08 17:16:42, jsbell wrote:
> On 2016/06/07 23:19:52, pfeldman wrote:
> > Unfortunately, that won't fix the front-ends retrospectively, so this change
> is
> > not sufficient.
> > In addition to these changes, you would need to update the
> > installBackwardsCompatibility function in devtools.js that would install the
> > polyfills for the APIs that you remove.
> 
> Just to confirm: this enables old frontend JS code to run in new Chrome? So
> something like:
> 
> // Support for legacy (<M52) frontends.
> 
> // FileError is removed; DOMException will be seen instead, with different
> codes.
> // Only cases expected by old devtools need to be mapped.
> window.FileError = window.FileError || {
>   NOT_FOUND_ERR: DOMException.NOT_FOUND_ERR,
>   ABORT_ERR: DOMException.ABORT_ERR,
>   INVALID_MODIFICATION_ERR: DOMException.INVALID_MODIFICATION_ERR,
>   NOT_READABLE_ERR: 0  // No matching DOMException code, so e.code will be 0
> };
> 
> The last entry is a bit odd. To explain: previously the APIs would generate a
> "NotReadableError" FileError w/ code === FileError.NOT_READABLE_ERR. Now they
> will generate a "NotReadableError" DOMException w/ code = 0. So the above
should
> allow the switch statement in devtools/front_end/timeline/TimelineLoader.js to
> work, although possibly other error cases (w/o matching DOMException codes, so
> code === 0) would fall into that case rather than the default, but that seems
> harmless.
> 
> Am I understanding correctly?

Yes, it seems exactly right.

[jsbell, 2016-06-09 21:34:09]

Here's a take on eliminating the FileError class entirely in favor of a
namespace. I think this is an improvement.

Still one issue to work through: the closure compiler type checking does not
like the devtools changes; it is unaware of DOMException.prototype.message, it
does not believe window.FileError should be overwritten, and it assumes that
ErrorCallbacks must take FileErrors. Yay for type checking! Boo for overly
aggressive type checking.

I'll need to ponder what to do here - @type casting, block this work on getting
closure's externs updated/rolled, etc.

https://codereview.chromium.org/2040563002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2040563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:8:
#include "core/dom/DOMException.h"
resolve(take(...)) implicitly calls toV8(DOMException*); this include was
previously provided by PresentationError.h which includes FileError.h which used
to include DOMError.h which includes DOMException.h.

https://codereview.chromium.org/2040563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2040563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:11:
#include "core/dom/DOMException.h"
DOMException::create() is used below

[jsbell, 2016-06-17 09:18:41]

On 2016/06/09 21:34:09, jsbell (OOO until 6-20) wrote:
> I'll need to ponder what to do here - @type casting, block this work on
getting
> closure's externs updated/rolled, etc.

Note to self: pfeldman recommended trying to override in
third_party/WebKit/Source/devtools/front_end/externs.js

[jsbell, 2016-06-20 21:57:54]

Okay - I think this is plausible. I'm not thrilled about the closure compiler
@suppress usage but that's the best I could come up with. Better suggestions
welcome!

I've filed https://github.com/google/closure-compiler/issues/1854 and
https://github.com/google/closure-compiler/issues/1855 for the externs.

[foolip, 2016-06-22 10:06:54]

The new FileError.*, but it looks like this is waiting to resolve the closure
issues? Please ping when this needs proper final review.

[jsbell, 2016-06-22 15:35:24]

On 2016/06/22 10:06:54, Philip Jägenstedt wrote:
> The new FileError.*,

^^^ Lost part of the sentence?

> but it looks like this is waiting to resolve the closure
> issues? Please ping when this needs proper final review.

The closure issues are resolved, so *ping*!

(But pfeldman may not be happy with the approach taken for the closure issues,
so more iteration may be needed there.)

[foolip, 2016-06-23 12:04:49]

lgtm, but see comment for what I didn't understand.

https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/ErrorCallback.h (right):

https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/ErrorCallback.h:44: virtual void
handleEvent(FileError::ErrorCode error) {
handleEvent(FileError::createDOMException(error)); }
I don't think I quite understand the new role of the ErrorCallback interface.
It's a thing that's implemented by V8ErrorCallback so that our C++ can invoke
the callback in C++, but the new signature with the enum is for internal use and
in many cases handleEvent(DOMException*) is NOTREACHED(). Should these two
things be one thing (maybe I missed a necessary connection) or would it be
possible to have one thing that's purely for bindings and another for internal
use? It seems like this might get in the way in any refactoring of how callbacks
work, and I have thought about trying to make it such that WebIDL callback
interfaces and callback functions look the same to C++, as part of getting rid
of our bogus callback interfaces that are actually callback functions in the
specs.

[jsbell, 2016-06-23 19:44:30]

https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/ErrorCallback.h (right):

https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/ErrorCallback.h:44: virtual void
handleEvent(FileError::ErrorCode error) {
handleEvent(FileError::createDOMException(error)); }
On 2016/06/23 12:04:48, Philip Jägenstedt wrote:
> I don't think I quite understand the new role of the ErrorCallback interface.
> It's a thing that's implemented by V8ErrorCallback so that our C++ can invoke
> the callback in C++, but the new signature with the enum is for internal use
and
> in many cases handleEvent(DOMException*) is NOTREACHED(). Should these two
> things be one thing (maybe I missed a necessary connection) or would it be
> possible to have one thing that's purely for bindings and another for internal
> use?

It is weird - I'll see if I can split it up.

It shows up where a sync API is implemented by calling the async API with e.g.
LocalErrorCallback which stashes the error code and then the sync API throws.

One approach would be to add ErrorCallbackBase which has only
handleEvent(FileError::ErrorCode). Internal uses subclass from this. Then we
could have a ScriptErrorCallback subclass that holds a the bindings-facing
ErrorCallback. Internal APIs would be changed to take ErrorCallbackBase,
script-facing APIs would wrap an ErrorCallback in ScriptErrorCallback.

[jsbell, 2016-07-11 22:02:27]

Okay, latest PS revamps ErrorCallback as described previously:

ErrorCallback returns to being boilerplate "interface" for bindings.

ErrorCallbackBase is introduced as the type used internally that accept
FileError::ErrorCode, with various derived classes.

ScriptErrorCallback is used to wrap an ErrorCallback as an ErrorCallbackBase.

foolip@, pfeldman@: please take a look?
please take a look?

[foolip, 2016-07-12 12:42:53]

lgtm % nits

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp (right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp:142: //
TODO(jsbell): Just call
m_errorCallback->handleEvent(FileError::createDOMException(error)) directly?
Is that blocked on some additional refactoring?

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp
(right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp:86: void
FileSystemCallbacksBase::handleEventOrScheduleCallback(CB* callback, CBArg arg)
Looking at this context, I guess my top nomination for name would be
s/handleEvent/invoke/. invokeOrScheduleCallback reads nicely I think.

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h (right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h:65: virtual
void handleEvent(FileError::ErrorCode) = 0;
Since this is no longer the thing that interacts with bindings, can the name
"handleEvent" be relegated to only ErrorCallback?

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h:106: virtual
void handleEvent(FileError::ErrorCode);
Should this be override instead?

[jsbell, 2016-07-12 17:43:52]

Thanks, foolip@ !

pfeldman@ - can you please take a look at the devtools changes?

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp (right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp:142: //
TODO(jsbell): Just call
m_errorCallback->handleEvent(FileError::createDOMException(error)) directly?
On 2016/07/12 12:42:53, Philip Jägenstedt wrote:
> Is that blocked on some additional refactoring?

Ah yes, I meant to ask for opinions. I'm waffling over which approach to take -
(1) keep a single place that calls handleEvent() or (2) eliminate the temp
wrapper. I'll go with #2 (i.e. make the change) but feedback welcome!

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp
(right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp:86: void
FileSystemCallbacksBase::handleEventOrScheduleCallback(CB* callback, CBArg arg)
On 2016/07/12 12:42:53, Philip Jägenstedt wrote:
> Looking at this context, I guess my top nomination for name would be
> s/handleEvent/invoke/. invokeOrScheduleCallback reads nicely I think.

Done.

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h (right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h:65: virtual
void handleEvent(FileError::ErrorCode) = 0;
On 2016/07/12 12:42:53, Philip Jägenstedt wrote:
> Since this is no longer the thing that interacts with bindings, can the name
> "handleEvent" be relegated to only ErrorCallback?

Done.

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h:106: virtual
void handleEvent(FileError::ErrorCode);
On 2016/07/12 12:42:53, Philip Jägenstedt wrote:
> Should this be override instead?

Done.

[pfeldman, 2016-07-12 21:29:45]

https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js
(right):

https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js:217:
switch (event.target.error.name) {
Was name previously available? If not, you would need to install it in the
compat layer in devtools.js

[foolip, 2016-07-13 14:39:17]

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp (right):

https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp:142: //
TODO(jsbell): Just call
m_errorCallback->handleEvent(FileError::createDOMException(error)) directly?
On 2016/07/12 17:43:51, jsbell wrote:
> On 2016/07/12 12:42:53, Philip Jägenstedt wrote:
> > Is that blocked on some additional refactoring?
> 
> Ah yes, I meant to ask for opinions. I'm waffling over which approach to take
-
> (1) keep a single place that calls handleEvent() or (2) eliminate the temp
> wrapper. I'll go with #2 (i.e. make the change) but feedback welcome!

This looks good, an extra wrapper just to avoid the characters "handleEvent"
seems like too much purity even for me :)

[jsbell, 2016-07-13 16:55:07]

https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js
(right):

https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js:217:
switch (event.target.error.name) {
On 2016/07/12 21:29:44, pfeldman wrote:
> Was name previously available? If not, you would need to install it in the
> compat layer in devtools.js

Yes; FileError started inheriting from DOMError circa 2013, and DOMError has had
name since 2012 (when the interface was first added).

[pfeldman, 2016-07-13 17:00:39]

devtools lgtm then

[jsbell, 2016-07-18 21:14:41]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-07-18 21:14:42]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2040563002/#ps140001
(title: "handleEvent -> invoke and other review nits")

[commit-bot: I haz the power, 2016-07-18 21:15:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 01:06:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-19 01:12:11]

Description was changed from

==========
Remove FileError interface

FileError was removed from File API [1] after Chrome had shipped the
functionality. Other browsers do not use it, returning a DOMError
instead. Using the `code` property has been logging deprecation
warnings since August 2013. The affected APIs will now yield
DOMExceptions instead (as DOMError is being removed from the platform
[2]). Script should inspect the `name` and `message` properties of the
error objects.

This affects:
* FileReader.error
* FileWriter.error
* FileSystem async API: passed to ErrorCallback on failure
* FileSystem sync API: thrown on failure

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kJMa2rpAYqI
Dashboard: https://www.chromestatus.com/feature/6687420359639040

[1] https://w3c.github.io/FileAPI/
[2] https://dom.spec.whatwg.org/#domerror

BUG=496901
R=foolip@chromium.org
==========

to

==========
Remove FileError interface

FileError was removed from File API [1] after Chrome had shipped the
functionality. Other browsers do not use it, returning a DOMError
instead. Using the `code` property has been logging deprecation
warnings since August 2013. The affected APIs will now yield
DOMExceptions instead (as DOMError is being removed from the platform
[2]). Script should inspect the `name` and `message` properties of the
error objects.

This affects:
* FileReader.error
* FileWriter.error
* FileSystem async API: passed to ErrorCallback on failure
* FileSystem sync API: thrown on failure

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kJMa2rpAYqI
Dashboard: https://www.chromestatus.com/feature/6687420359639040

[1] https://w3c.github.io/FileAPI/
[2] https://dom.spec.whatwg.org/#domerror

BUG=496901
R=foolip@chromium.org

Committed: https://crrev.com/ef3e334a185b82752f606768ecd3f4c8083b7275
Cr-Commit-Position: refs/heads/master@{#406169}
==========

[commit-bot: I haz the power, 2016-07-19 01:12:12]

Patchset 8 (id:??) landed as
https://crrev.com/ef3e334a185b82752f606768ecd3f4c8083b7275
Cr-Commit-Position: refs/heads/master@{#406169}