Bug 183865 – Move request checking out of PingLoad for future reuse in NetworkLoad

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Move request checking out of PingLoad for future reuse in NetworkLoad

[youenn fablet]

Created attachment 336225 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/38720873>

[youenn fablet]

Created attachment 336227 [details]
Patch

[Alex Christensen]

Comment on attachment 336227 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:84
> -    m_completionCallback(Result::Failure);
> +    returnResult(Result::Failure);

CompletionHandlers null themselves out when called, so these changes are redundant with the change in NetworkCORSPreflightChecker.h

[youenn fablet]

Created attachment 336231 [details]
Patch

[youenn fablet]

Created attachment 336232 [details]
Patch

[Daniel Bates]

Comment on attachment 336232 [details]
Patch

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

> Source/WebKit/ChangeLog:16
> +        Fixed a bug in PingLoad to ensure that credentials are not sent when going from a third-party

Would it be straightforward to separate out this change into its own bug? If so, I suggest we do so and keeps this patch focused  on the refactoring and correctness of the refactoring. The benefit of this approach is that in the event that the refactoring causes a regression we can roll it out without rolling out the bug fix. It would also enable us to take only the bug fix on a branch if desired.

[youenn fablet]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 336232 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336232&action=review
> 
> > Source/WebKit/ChangeLog:16
> > +        Fixed a bug in PingLoad to ensure that credentials are not sent when going from a third-party
> 
> Would it be straightforward to separate out this change into its own bug? If
> so, I suggest we do so and keeps this patch focused  on the refactoring and
> correctness of the refactoring. The benefit of this approach is that in the
> event that the refactoring causes a regression we can roll it out without
> rolling out the bug fix. It would also enable us to take only the bug fix on
> a branch if desired.

I can prepare the bug fix patch tomorrow.
I don't think this changes much of the review of this patch though.
Putting it in r? for now.

[youenn fablet]

Created attachment 336383 [details]
Patch

[EWS Watchlist]

Comment on attachment 336383 [details]
Patch

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

New failing tests:
media/modern-media-controls/tracks-panel/tracks-panel-hide-esc-key.html

[EWS Watchlist]

Created attachment 336401 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Comment on attachment 336383 [details]
Patch

Test failure is unrelated

[Chris Dumez]

Comment on attachment 336383 [details]
Patch

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

r=me with tiny nit.

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:50
> +    enum class Result { Success, Failure, Cancelled };

We prefer Canceled with 1 'l'.

[youenn fablet]

Created attachment 336617 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336617 [details]
Patch for landing

Clearing flags on attachment: 336617

Committed r230014: <https://trac.webkit.org/changeset/230014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.