Bug 162382

Summary: [Fetch API] Request constructor should provide exception messages
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, darin, dbates, esprehn+autocc, kangil.han, kondapallykalyan, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Rebasing failing test
none
Patch for landing none

Description youenn fablet 2016-09-22 05:11:29 PDT
Request constructor should provide exception messages
Comment 1 youenn fablet 2016-09-22 05:16:59 PDT
Created attachment 289550 [details]
Patch
Comment 2 Build Bot 2016-09-22 06:05:17 PDT
Comment on attachment 289550 [details]
Patch

Attachment 289550 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2125290

New failing tests:
fetch/fetch-url-serialization.html
Comment 3 Build Bot 2016-09-22 06:05:21 PDT
Created attachment 289553 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Build Bot 2016-09-22 06:21:08 PDT
Comment on attachment 289550 [details]
Patch

Attachment 289550 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2125356

New failing tests:
fetch/fetch-url-serialization.html
Comment 5 Build Bot 2016-09-22 06:21:12 PDT
Created attachment 289554 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 youenn fablet 2016-09-22 07:13:29 PDT
Created attachment 289559 [details]
Rebasing failing test
Comment 7 Darin Adler 2016-10-07 09:17:57 PDT
Comment on attachment 289559 [details]
Rebasing failing test

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

Looks OK. After my next patch, you could have used ExceptionOr<void> instead of Optional<Exception> and we should decide which style we prefer.

> Source/WebCore/dom/Exception.h:38
> +    explicit Exception(ExceptionCode, const String&);

We should consider taking a String&& instead to avoid reference count churn, since we almost always want to store a message in here.

> Source/WebCore/dom/Exception.h:41
> +    const String& message() const { return m_message; }

We should consider adding a releaseMessage function here so we can avoid reference count churn when turning this into a JavaScript exception.

> Source/WebCore/dom/ExceptionOr.h:41
> +    const String& exceptionMessage() const;

We should consider adding a releaseMessage function here so we can avoid reference count churn when turning this into a JavaScript exception.
Comment 8 youenn fablet 2016-10-08 07:14:08 PDT
Thanks for the review.

> Looks OK. After my next patch, you could have used ExceptionOr<void> instead
> of Optional<Exception> and we should decide which style we prefer.

I don't have a strong preference.
Returning Nullopt seems a tad more explicit than returning { }.

> > Source/WebCore/dom/Exception.h:38
> > +    explicit Exception(ExceptionCode, const String&);
> 
> We should consider taking a String&& instead to avoid reference count churn,
> since we almost always want to store a message in here.

Right.
Filed bug 163165 for that purpose.
Comment 9 youenn fablet 2016-10-08 07:18:37 PDT
Created attachment 291013 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-10-08 07:52:19 PDT
Comment on attachment 291013 [details]
Patch for landing

Clearing flags on attachment: 291013

Committed r206954: <http://trac.webkit.org/changeset/206954>
Comment 11 WebKit Commit Bot 2016-10-08 07:52:23 PDT
All reviewed patches have been landed.  Closing bug.