Summary: | Add the message property to DOMError | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shivakumar J M <shiva.jm> | ||||||||||||||||||||
Component: | DOM | Assignee: | Brady Eidson <beidson> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, arv, beidson, buildbot, cdumez, commit-queue, darin, gyuyoung.kim, ossy, rniwa, syoichi | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 149117, 115701 | ||||||||||||||||||||||
Attachments: |
|
Description
Shivakumar J M
2014-12-01 22:00:02 PST
Created attachment 242387 [details]
Patch
DOMError should be constructible as per the DOM4 specification.
Comment on attachment 242387 [details] Patch Attachment 242387 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5503715603644416 New failing tests: js/dom/global-constructors-attributes.html Created attachment 242391 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 242387 [details] Patch Attachment 242387 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6385315623206912 New failing tests: js/dom/global-constructors-attributes.html Created attachment 242393 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
*** Bug 115644 has been marked as a duplicate of this bug. *** Working to fix test failures, will upload the patch after test fix. Comment on attachment 242387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242387&action=review > Source/WebCore/ChangeLog:9 > + I have confirmed this indeed matches the behavior of Chrome. According to https://src.chromium.org/viewvc/blink?view=rev&revision=172718, this also matches Firefox (tested 28). BTW, not the first time I wonder but are you porting patches over from Blink? If so, please point to the Blink commit in your Changelog. > Source/WebCore/dom/DOMError.h:53 > explicit DOMError(const String& name); We can probably get rid of this constructor and add a default value to the second argument below. > Source/WebCore/dom/DOMError.h:54 > + DOMError(const String& name, const String& message); explicit if you mark the second argument as optional. Created attachment 242874 [details]
Patch-Updated-Review1
Updated the patch and tests as per review comments.
Created attachment 242875 [details]
Patch-Updated-Review1
Corrected the previously updated patch.
(In reply to comment #8) > Comment on attachment 242387 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242387&action=review > > > Source/WebCore/ChangeLog:9 > > + I have confirmed this indeed matches the behavior of Chrome. > > According to https://src.chromium.org/viewvc/blink?view=rev&revision=172718, > this also matches Firefox (tested 28). BTW, not the first time I wonder but > are you porting patches over from Blink? If so, please point to the Blink > commit in your Changelog. > > > Source/WebCore/dom/DOMError.h:53 > > explicit DOMError(const String& name); > > We can probably get rid of this constructor and add a default value to the > second argument below. > > > Source/WebCore/dom/DOMError.h:54 > > + DOMError(const String& name, const String& message); > > explicit if you mark the second argument as optional. Dear Chris, Thanks for the inputs, updated the patch with comments and also added Blink commit id in the change log. I did not know the procedure to get exact Blink commit revision link. But Usually we read the latest/draft specs and compare the current webkit implementation suppourt, if webkit does not match to latest spec then test with other browser by writing simple manual test, if other browsers suppourts the feature then learn from the other browser, if they already suppourt. I will try to mention the exact change id of patches for future patches, if its learnt/port/inspired from other browser suppourt. Thanks Comment on attachment 242875 [details] Patch-Updated-Review1 Attachment 242875 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6547176834465792 New failing tests: js/dom/global-constructors-attributes.html Created attachment 242878 [details]
Archive of layout-test-results from ews103 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 242875 [details] Patch-Updated-Review1 Attachment 242875 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5362549155430400 New failing tests: js/dom/global-constructors-attributes.html Created attachment 242882 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 243111 [details]
Patch-Updated-Review3
Updated the patch with test failures.
Comment on attachment 243111 [details] Patch-Updated-Review3 View in context: https://bugs.webkit.org/attachment.cgi?id=243111&action=review > Source/WebCore/ChangeLog:3 > + DOMError should be constructible as per the DOM4 specification. There is no DOM4 specification anymore. I guess you are referring to the DOM living standard (We usually provide a link to the specification when doing such change). However, DOMError is actually not even part of the DOM living standard anymore: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23367 https://github.com/whatwg/dom/commit/7fe5736d3ed2717d71a3d2627b07dee97e10ef05 https://dom.spec.whatwg.org/#domerror As such, I am not sure it is worth updating DOMError. Our current implementation matches DOM Core level 3 and has been this way for a long time. I don't think updating it makes sense if it is no longer part of newer revisions of the DOM spec. Dear All, This Bug has been marked as a BlinkMergeCandidate ( https://bugs.webkit.org/show_bug.cgi?id=115701) to Make DOM objects constructible with new, possible per DOM4. though it will be replaced with DOMExceptions in future, as per DOM living standard https://dom.spec.whatwg.org/#domerror. so shall we accept these change or mark as WONTFIX ?. please share your inputs. Thanks Yes, my proposal would be to mark as WONTFIX (given that I think there is little point in updating something that is no longer part of the standard). That said, Darin or Ryosuke may have a different opinion so let's give them a chance to comment. I think our path should be to remove DOMError rather than updating it. Is that practical? Are there websites that depend on it? Dear All, DOMError can be removed from Webkit, since File APIs are not using it, though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the error attribute still uses the DOMError. Also in MDN link DOMError is still used in File APIs: https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error Thanks (In reply to comment #21) > Dear All, > > DOMError can be removed from Webkit, since File APIs are not using it, > though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the > error attribute still uses the DOMError. Also in MDN link DOMError is still > used in File APIs: > https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error > > Thanks We still use DOMError in WebKit (for e.g. MediaStream and IndexedDB API, CSSFontFaceLoadEvent). It is likely that the corresponding specifications are not using DOMError anymore so our implementations would have to be updated first. (In reply to comment #22) > (In reply to comment #21) > > Dear All, > > > > DOMError can be removed from Webkit, since File APIs are not using it, > > though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the > > error attribute still uses the DOMError. Also in MDN link DOMError is still > > used in File APIs: > > https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error > > > > Thanks > > We still use DOMError in WebKit (for e.g. MediaStream and IndexedDB API, > CSSFontFaceLoadEvent). It is likely that the corresponding specifications > are not using DOMError anymore so our implementations would have to be > updated first. Dear Chris, Yes right, we need to update our implementations for other module to support using DOMError, may be later we can accept these changes, is it right? Thanks (In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Dear All, > > > > > > DOMError can be removed from Webkit, since File APIs are not using it, > > > though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the > > > error attribute still uses the DOMError. Also in MDN link DOMError is still > > > used in File APIs: > > > https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error > > > > > > Thanks > > > > We still use DOMError in WebKit (for e.g. MediaStream and IndexedDB API, > > CSSFontFaceLoadEvent). It is likely that the corresponding specifications > > are not using DOMError anymore so our implementations would have to be > > updated first. > > > Dear Chris, > > Yes right, we need to update our implementations for other module to > support using DOMError, may be later we can accept these changes, is it > right? DOMError is still used in the W3C recommendation for IndexedDB (http://www.w3.org/TR/IndexedDB/) The W3C recommendation for DOM4 (http://www.w3.org/TR/dom/) still includes DOMError in an appendix. It warns to not use it, and instead refer to "Error" in WebIDL (http://www.w3.org/TR/WebIDL-1/) The previous working draft for DOM4 (http://www.w3.org/TR/2015/PR-dom-20151006/) explains the real impetus behind that change - DOMError can't be removed because some specs still use it (Namely IndexedDB). WebIDL defines "Error" as the set of all objects that can be used to represent an error, which correspond to the ECMAScript error objects (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-error-objects) ECMAScript error objects have a message property. I have a few points for listing the current state of the specs WRT DOMError: 1 - DOMError is baked into the final IndexedDB spec, and therefore we can't remove it. 2 - But DOMError is strongly deprecated. 3 - DOMError's non-deprecated replacement is basically "an object representing an Error that has a message property" 4 - IndexedDB code sure would love to have custom messages in its DOMErrors instead of just a generic error code. My personal takeaway from all of the above is that we *should* add the the message property to DOMError so it can better fulfill its purpose, but we should NOT add the constructor. I still think the message part of this patch is relevant, and I would love to see it land soon. (In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > Dear All, > > > > > > > > DOMError can be removed from Webkit, since File APIs are not using it, > > > > though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the > > > > error attribute still uses the DOMError. Also in MDN link DOMError is still > > > > used in File APIs: > > > > https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error > > > > > > > > Thanks > > > > > > We still use DOMError in WebKit (for e.g. MediaStream and IndexedDB API, > > > CSSFontFaceLoadEvent). It is likely that the corresponding specifications > > > are not using DOMError anymore so our implementations would have to be > > > updated first. > > > > > > Dear Chris, > > > > Yes right, we need to update our implementations for other module to > > support using DOMError, may be later we can accept these changes, is it > > right? > > DOMError is still used in the W3C recommendation for IndexedDB > (http://www.w3.org/TR/IndexedDB/) > > The W3C recommendation for DOM4 (http://www.w3.org/TR/dom/) still includes > DOMError in an appendix. It warns to not use it, and instead refer to > "Error" in WebIDL (http://www.w3.org/TR/WebIDL-1/) > > The previous working draft for DOM4 > (http://www.w3.org/TR/2015/PR-dom-20151006/) explains the real impetus > behind that change - DOMError can't be removed because some specs still use > it (Namely IndexedDB). > > WebIDL defines "Error" as the set of all objects that can be used to > represent an error, which correspond to the ECMAScript error objects > (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-error-objects) > > ECMAScript error objects have a message property. > > I have a few points for listing the current state of the specs WRT DOMError: > 1 - DOMError is baked into the final IndexedDB spec, and therefore we can't > remove it. > 2 - But DOMError is strongly deprecated. > 3 - DOMError's non-deprecated replacement is basically "an object > representing an Error that has a message property" > 4 - IndexedDB code sure would love to have custom messages in its DOMErrors > instead of just a generic error code. > > My personal takeaway from all of the above is that we *should* add the the > message property to DOMError so it can better fulfill its purpose, but we > should NOT add the constructor. > > I still think the message part of this patch is relevant, and I would love > to see it land soon. Sure, I don't have any issue with adding the message attribute to DOMError. My main concern was about starting to expose DOMError on the global object, considering it is legacy. (In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > (In reply to comment #22) > > > > (In reply to comment #21) > > > > > Dear All, > > > > > > > > > > DOMError can be removed from Webkit, since File APIs are not using it, > > > > > though as per FileReader Spec:http://dev.w3.org/2006/webapi/FileAPI/, the > > > > > error attribute still uses the DOMError. Also in MDN link DOMError is still > > > > > used in File APIs: > > > > > https://developer.mozilla.org/en-US/docs/Web/API/FileReader.error > > > > > > > > > > Thanks > > > > > > > > We still use DOMError in WebKit (for e.g. MediaStream and IndexedDB API, > > > > CSSFontFaceLoadEvent). It is likely that the corresponding specifications > > > > are not using DOMError anymore so our implementations would have to be > > > > updated first. > > > > > > > > > Dear Chris, > > > > > > Yes right, we need to update our implementations for other module to > > > support using DOMError, may be later we can accept these changes, is it > > > right? > > > > DOMError is still used in the W3C recommendation for IndexedDB > > (http://www.w3.org/TR/IndexedDB/) > > > > The W3C recommendation for DOM4 (http://www.w3.org/TR/dom/) still includes > > DOMError in an appendix. It warns to not use it, and instead refer to > > "Error" in WebIDL (http://www.w3.org/TR/WebIDL-1/) > > > > The previous working draft for DOM4 > > (http://www.w3.org/TR/2015/PR-dom-20151006/) explains the real impetus > > behind that change - DOMError can't be removed because some specs still use > > it (Namely IndexedDB). > > > > WebIDL defines "Error" as the set of all objects that can be used to > > represent an error, which correspond to the ECMAScript error objects > > (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-error-objects) > > > > ECMAScript error objects have a message property. > > > > I have a few points for listing the current state of the specs WRT DOMError: > > 1 - DOMError is baked into the final IndexedDB spec, and therefore we can't > > remove it. > > 2 - But DOMError is strongly deprecated. > > 3 - DOMError's non-deprecated replacement is basically "an object > > representing an Error that has a message property" > > 4 - IndexedDB code sure would love to have custom messages in its DOMErrors > > instead of just a generic error code. > > > > My personal takeaway from all of the above is that we *should* add the the > > message property to DOMError so it can better fulfill its purpose, but we > > should NOT add the constructor. > > > > I still think the message part of this patch is relevant, and I would love > > to see it land soon. > > Sure, I don't have any issue with adding the message attribute to DOMError. > My main concern was about starting to expose DOMError on the global object, > considering it is legacy. Thanks, OK will update the patch to add the message attribute to DOMError. Taking this. Created attachment 276457 [details]
Patch v1
The only tests I ran before uploading this patch were IndexedDB tests, because I *knew* this change would affect them.
I just started the full suite of layouttests locally and will also let EWS do its thing.
If there are failures, please review anyways and just cq-, and I will update the changed tests before landing.
Comment on attachment 276457 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=276457&action=review > Source/WebCore/dom/DOMError.idl:33 > + readonly attribute DOMString message; This is in http://www.w3.org/TR/dom/#sec-domerror Are there no web conformance tests that cover this besides the new IndexedDB tests? (In reply to comment #29) > Comment on attachment 276457 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276457&action=review > > > Source/WebCore/dom/DOMError.idl:33 > > + readonly attribute DOMString message; > > This is in http://www.w3.org/TR/dom/#sec-domerror > Are there no web conformance tests that cover this besides the new IndexedDB > tests? Not that I could find - Seems like the name/message form was added in DOM4 but then deprecated in DOM4. /me shrugs Comment on attachment 276457 [details] Patch v1 Clearing flags on attachment: 276457 Committed r199581: <http://trac.webkit.org/changeset/199581> All reviewed patches have been landed. Closing bug. (In reply to comment #30) > (In reply to comment #29) > > Comment on attachment 276457 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=276457&action=review > > > > > Source/WebCore/dom/DOMError.idl:33 > > > + readonly attribute DOMString message; > > > > This is in http://www.w3.org/TR/dom/#sec-domerror > > Are there no web conformance tests that cover this besides the new IndexedDB > > tests? > > Not that I could find - Seems like the name/message form was added in DOM4 > but then deprecated in DOM4. > > /me shrugs The issue is really that our IndexedDB implementation is outdated. DOMError no longer exists and updating it has little value IMHO. I see that our IndexedDB implementation uses DOMError type for IDBTransaction.error and IDBRequest.error, but the latest spec says they should use DOMException: - http://w3c.github.io/IndexedDB/#transaction - http://w3c.github.io/IndexedDB/#request-api I think what we really want to do is update our IDB implementation to use DOMException and eventually drop DOMError. (In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #29) > > > Comment on attachment 276457 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=276457&action=review > > > > > > > Source/WebCore/dom/DOMError.idl:33 > > > > + readonly attribute DOMString message; > > > > > > This is in http://www.w3.org/TR/dom/#sec-domerror > > > Are there no web conformance tests that cover this besides the new IndexedDB > > > tests? > > > > Not that I could find - Seems like the name/message form was added in DOM4 > > but then deprecated in DOM4. > > > > /me shrugs > > The issue is really that our IndexedDB implementation is outdated. DOMError > no longer exists and updating it has little value IMHO. > > I see that our IndexedDB implementation uses DOMError type for > IDBTransaction.error and IDBRequest.error, but the latest spec says they > should use DOMException: > - http://w3c.github.io/IndexedDB/#transaction > - http://w3c.github.io/IndexedDB/#request-api > > I think what we really want to do is update our IDB implementation to use > DOMException and eventually drop DOMError. DOMException has moved from DOM to WebIDL a while back: https://heycam.github.io/webidl/#dfn-DOMException (In reply to comment #33) > (In reply to comment #30) > > (In reply to comment #29) > > > Comment on attachment 276457 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=276457&action=review > > > > > > > Source/WebCore/dom/DOMError.idl:33 > > > > + readonly attribute DOMString message; > > > > > > This is in http://www.w3.org/TR/dom/#sec-domerror > > > Are there no web conformance tests that cover this besides the new IndexedDB > > > tests? > > > > Not that I could find - Seems like the name/message form was added in DOM4 > > but then deprecated in DOM4. > > > > /me shrugs > > The issue is really that our IndexedDB implementation is outdated. DOMError > no longer exists and updating it has little value IMHO. > > I see that our IndexedDB implementation uses DOMError type for > IDBTransaction.error and IDBRequest.error, but the latest spec says they > should use DOMException: > - http://w3c.github.io/IndexedDB/#transaction > - http://w3c.github.io/IndexedDB/#request-api Those are the latest in-development versions of the spec. We are focusing solely on the stable CR version - http://www.w3.org/TR/IndexedDB/ - which still uses DOMError. This includes the stable W3C test suite. We're not going to be changing it at this time. > I think what we really want to do is update our IDB implementation to use > DOMException and eventually drop DOMError. Dropping DOMError altogether is a great goal going forward, but is outside the scope of bringing it up to standard. (Yes, it is deprecated, but still standard, and exercised by tests.) |