Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(104)

Issue 2040563002: Remove FileError interface (Closed)

Created:
3 years, 11 months ago by jsbell
Modified:
3 years, 10 months ago
Reviewers:
foolip, pfeldman
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, tfarina, serviceworker-reviews, blink-reviews-w3ctests_chromium.org, nhiroki, falken, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, kinuko+fileapi
Base URL:
https://chromium.googlesource.com/chromium/src.git@fe-dep
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Devtools/test updates #

Total comments: 9

Patch Set 3 : Review feedback #

Total comments: 2

Patch Set 4 : Rebased, and closure annotations #

Total comments: 2

Patch Set 5 : Rebased, just to get it working again #

Patch Set 6 : Refactor ErrorCallback #

Total comments: 9

Patch Set 7 : Mark single-arg ctor as explicit #

Patch Set 8 : handleEvent -> invoke and other review nits #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+333 lines, -436 lines) Patch
M third_party/WebKit/LayoutTests/fast/dom/script-tests/constructed-objects-prototypes.js View 1 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/js/script-tests/global-constructors.js View 1 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/isolated-filesystem-test.js View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -15 lines 0 comments Download
M third_party/WebKit/LayoutTests/imported/wpt/FileAPI/historical-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -15 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 2 3 4 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt View 1 2 3 4 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/fileapi/FileError.h View 1 2 2 chunks +17 lines, -30 lines 0 comments Download
M third_party/WebKit/Source/core/fileapi/FileError.cpp View 1 2 3 chunks +61 lines, -56 lines 0 comments Download
D third_party/WebKit/Source/core/fileapi/FileError.idl View 1 chunk +0 lines, -48 lines 0 comments Download
M third_party/WebKit/Source/core/fileapi/FileReader.h View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/fileapi/FileReader.cpp View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/fileapi/FileReader.idl View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Deprecation.cpp View 1 2 3 4 1 chunk +0 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/frame/UseCounter.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/devtools.js View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/externs.js View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js View 1 1 chunk +4 lines, -4 lines 2 comments Download
M third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystem.js View 1 2 3 5 chunks +6 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystem.h View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystem.cpp View 1 2 3 4 5 6 7 4 chunks +7 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystemBase.h View 1 2 3 4 5 4 chunks +12 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystemBase.cpp View 1 2 3 4 5 10 chunks +24 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystemSync.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMFileSystemSync.cpp View 1 2 3 4 5 6 7 2 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.cpp View 1 2 3 4 5 2 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DirectoryEntry.cpp View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DirectoryReader.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp View 1 2 3 4 5 6 7 5 chunks +9 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/DirectoryReaderSync.cpp View 1 2 3 4 5 6 7 2 chunks +5 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/Entry.cpp View 1 2 3 4 5 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/ErrorCallback.h View 1 2 3 4 5 1 chunk +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/ErrorCallback.idl View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileEntry.cpp View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.h View 1 2 3 4 5 6 7 8 chunks +46 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileSystemCallbacks.cpp View 1 2 3 4 5 6 7 13 chunks +63 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileWriter.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileWriter.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/FileWriter.idl View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/filesystem/SyncCallbackHelper.h View 1 2 3 4 5 6 7 4 chunks +8 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/modules/filesystem/WorkerGlobalScopeFileSystem.cpp View 1 2 3 4 5 2 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp View 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 30 (4 generated)
jsbell
Initial CL for feedback on this approach. I tried removing FileError entirely, but the bindings ...
3 years, 11 months ago (2016-06-03 21:41:59 UTC) #1
jsbell
pfeldman@ - can you take a look at the devtools changes? I have low confidence ...
3 years, 11 months ago (2016-06-07 22:58:16 UTC) #3
pfeldman
Unfortunately, that won't fix the front-ends retrospectively, so this change is not sufficient. In addition ...
3 years, 11 months ago (2016-06-07 23:19:52 UTC) #4
foolip
Looks good and makes sense. Just want to make sure I understand what big mess ...
3 years, 11 months ago (2016-06-08 10:04:44 UTC) #5
jsbell
On 2016/06/07 23:19:52, pfeldman wrote: > Unfortunately, that won't fix the front-ends retrospectively, so this ...
3 years, 11 months ago (2016-06-08 17:16:42 UTC) #6
jsbell
Thanks for the initial look. I'll see about trying to replace the GC'd class with ...
3 years, 11 months ago (2016-06-08 17:27:43 UTC) #7
jsbell
Oh, and don't expect further updates here 'til after BlinkOn (so ~2 weeks). Thank you ...
3 years, 11 months ago (2016-06-08 17:28:38 UTC) #8
foolip
https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Source/core/fileapi/FileError.cpp File third_party/WebKit/Source/core/fileapi/FileError.cpp (right): https://codereview.chromium.org/2040563002/diff/20001/third_party/WebKit/Source/core/fileapi/FileError.cpp#newcode146 third_party/WebKit/Source/core/fileapi/FileError.cpp:146: return DOMException::create(SecurityError, FileError::securityErrorMessage); On 2016/06/08 17:27:42, jsbell wrote: > ...
3 years, 11 months ago (2016-06-08 21:07:56 UTC) #9
pfeldman
On 2016/06/08 17:16:42, jsbell wrote: > On 2016/06/07 23:19:52, pfeldman wrote: > > Unfortunately, that ...
3 years, 11 months ago (2016-06-08 21:11:09 UTC) #10
jsbell
Here's a take on eliminating the FileError class entirely in favor of a namespace. I ...
3 years, 11 months ago (2016-06-09 21:34:09 UTC) #11
jsbell
On 2016/06/09 21:34:09, jsbell (OOO until 6-20) wrote: > I'll need to ponder what to ...
3 years, 11 months ago (2016-06-17 09:18:41 UTC) #12
jsbell
Okay - I think this is plausible. I'm not thrilled about the closure compiler @suppress ...
3 years, 11 months ago (2016-06-20 21:57:54 UTC) #13
foolip
The new FileError.*, but it looks like this is waiting to resolve the closure issues? ...
3 years, 11 months ago (2016-06-22 10:06:54 UTC) #14
jsbell
On 2016/06/22 10:06:54, Philip Jägenstedt wrote: > The new FileError.*, ^^^ Lost part of the ...
3 years, 11 months ago (2016-06-22 15:35:24 UTC) #15
foolip
lgtm, but see comment for what I didn't understand. https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Source/modules/filesystem/ErrorCallback.h File third_party/WebKit/Source/modules/filesystem/ErrorCallback.h (right): https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Source/modules/filesystem/ErrorCallback.h#newcode44 third_party/WebKit/Source/modules/filesystem/ErrorCallback.h:44: ...
3 years, 11 months ago (2016-06-23 12:04:49 UTC) #16
jsbell
https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Source/modules/filesystem/ErrorCallback.h File third_party/WebKit/Source/modules/filesystem/ErrorCallback.h (right): https://codereview.chromium.org/2040563002/diff/60001/third_party/WebKit/Source/modules/filesystem/ErrorCallback.h#newcode44 third_party/WebKit/Source/modules/filesystem/ErrorCallback.h:44: virtual void handleEvent(FileError::ErrorCode error) { handleEvent(FileError::createDOMException(error)); } On 2016/06/23 ...
3 years, 11 months ago (2016-06-23 19:44:30 UTC) #17
jsbell
Okay, latest PS revamps ErrorCallback as described previously: ErrorCallback returns to being boilerplate "interface" for ...
3 years, 10 months ago (2016-07-11 22:02:27 UTC) #18
foolip
lgtm % nits https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp File third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp (right): https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp#newcode142 third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp:142: // TODO(jsbell): Just call m_errorCallback->handleEvent(FileError::createDOMException(error)) directly? ...
3 years, 10 months ago (2016-07-12 12:42:53 UTC) #19
jsbell
Thanks, foolip@ ! pfeldman@ - can you please take a look at the devtools changes? ...
3 years, 10 months ago (2016-07-12 17:43:52 UTC) #20
pfeldman
https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js (right): https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js#newcode217 third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js:217: switch (event.target.error.name) { Was name previously available? If not, ...
3 years, 10 months ago (2016-07-12 21:29:45 UTC) #21
foolip
https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp File third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp (right): https://codereview.chromium.org/2040563002/diff/100001/third_party/WebKit/Source/modules/filesystem/DirectoryReader.cpp#newcode142 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, ...
3 years, 10 months ago (2016-07-13 14:39:17 UTC) #22
jsbell
https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js (right): https://codereview.chromium.org/2040563002/diff/140001/third_party/WebKit/Source/devtools/front_end/timeline/TimelineLoader.js#newcode217 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: > ...
3 years, 10 months ago (2016-07-13 16:55:07 UTC) #23
pfeldman
devtools lgtm then
3 years, 10 months ago (2016-07-13 17:00:39 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2040563002/140001
3 years, 10 months ago (2016-07-18 21:15:22 UTC) #27
commit-bot: I haz the power
Committed patchset #8 (id:140001)
3 years, 10 months ago (2016-07-19 01:06:52 UTC) #28
commit-bot: I haz the power
3 years, 10 months ago (2016-07-19 01:12:12 UTC) #30
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/ef3e334a185b82752f606768ecd3f4c8083b7275
Cr-Commit-Position: refs/heads/master@{#406169}

Powered by Google App Engine
This is Rietveld 408576698