Bug 139173 – Add the message property to DOMError

• Component: DOM
• Status: RESOLVED FIXED

[Shivakumar J M]

DOMError should be constructible as per the DOM4 specification: http://w3c.github.io/dom/#interface-domerror, though it will be replaced with DOMExceptions in future, but DOMError is still used by several specifications (e.g. File API, IndexedDB).

[Shivakumar J M]

Created attachment 242387 [details]
Patch

DOMError should be constructible as per the DOM4 specification.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Csaba Osztrogonác]

*** Bug 115644 has been marked as a duplicate of this bug. ***

[Shivakumar J M]

Working to fix test failures, will upload the patch after test fix.

[Chris Dumez]

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.

[Shivakumar J M]

Created attachment 242874 [details]
Patch-Updated-Review1

Updated the patch and tests as per review comments.

[Shivakumar J M]

Created attachment 242875 [details]
Patch-Updated-Review1

Corrected the previously updated patch.

[Shivakumar J M]

(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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Shivakumar J M]

Created attachment 243111 [details]
Patch-Updated-Review3

Updated the patch with test failures.

[Chris Dumez]

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.

[Shivakumar J M]

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

[Chris Dumez]

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.

[Darin Adler]

I think our path should be to remove DOMError rather than updating it. Is that practical? Are there websites that depend on it?

[Shivakumar J M]

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

[Chris Dumez]

(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.

[Shivakumar J M]

(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

[Brady Eidson]

(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.

[Chris Dumez]

(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.

[Shivakumar J M]

(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.

[Brady Eidson]

Taking this.

[Brady Eidson]

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.

[Alex Christensen]

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?

[Brady Eidson]

(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

[WebKit Commit Bot]

Comment on attachment 276457 [details]
Patch v1

Clearing flags on attachment: 276457

Committed r199581: <http://trac.webkit.org/changeset/199581>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(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.

[Chris Dumez]

(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

[Brady Eidson]

(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.)