Bug 167453 - [QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
Summary: [QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id:...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-26 02:16 PST by Andy Estes
Modified: 2017-01-26 18:01 PST (History)
10 users (show)

See Also:

Attachments
Patch (19.41 KB, patch)
2017-01-26 03:05 PST, Andy Estes 		no flags Details | Formatted Diff | Diff
Patch (19.51 KB, patch)
2017-01-26 17:23 PST, Andy Estes 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2017-01-26 02:16:28 PST 
[QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
Comment 1 Andy Estes 2017-01-26 03:05:58 PST 
Created attachment 299801 [details]
Patch
Comment 2 Brent Fulgham 2017-01-26 11:02:40 PST 
Comment on attachment 299801 [details]
Patch

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

Looks good, but I had one question about the code change in the connection implementation. r=me.

> Source/WebCore/loader/ios/QuickLook.mm:-341
> -    [self _sendDidReceiveResponseIfNecessary];

I think I understand everything about this patch, except for this. Do we no longer emit necessary notifications without this send?
Comment 3 Andy Estes 2017-01-26 15:20:26 PST 
(In reply to comment #2)
> Comment on attachment 299801 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299801&action=review
> 
> Looks good, but I had one question about the code change in the connection
> implementation. r=me.
> 
> > Source/WebCore/loader/ios/QuickLook.mm:-341
> > -    [self _sendDidReceiveResponseIfNecessary];
> 
> I think I understand everything about this patch, except for this. Do we no
> longer emit necessary notifications without this send?

I was trying to simplify things by directly calling ResourceLoader::didFail() directly without first calling didReceiveResponse(). In theory this shouldn't change behavior (in practice we can't even test this code path since QuickLook is configured to generate HTML error pages for non-password conversion failures).

Doing this isn't necessary for this patch, though, so you're right to point it out. I'll add this call back.

Thanks for the review!
Comment 4 Andy Estes 2017-01-26 17:23:27 PST 
Created attachment 299886 [details]
Patch
Comment 5 WebKit Commit Bot 2017-01-26 18:01:37 PST 
Comment on attachment 299886 [details]
Patch

Clearing flags on attachment: 299886

Committed r211248: <http://trac.webkit.org/changeset/211248>
Comment 6 WebKit Commit Bot 2017-01-26 18:01:42 PST 
All reviewed patches have been landed.  Closing bug.