Bug 162382 - [Fetch API] Request constructor should provide exception messages
Summary: [Fetch API] Request constructor should provide exception messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-09-22 05:11 PDT by youenn fablet
Modified: 2016-10-08 07:52 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.36 KB, patch)
2016-09-22 05:16 PDT, youenn fablet
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.08 MB, application/zip)
2016-09-22 06:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (920.14 KB, application/zip)
2016-09-22 06:21 PDT, Build Bot
no flags Details
Rebasing failing test (22.33 KB, patch)
2016-09-22 07:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (22.22 KB, patch)
2016-10-08 07:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.