Bug 167453

Summary: [QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, commit-queue, darin, dbates, ddkilzer, japhet, kling, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=163240
https://bugs.webkit.org/show_bug.cgi?id=135651
Attachments:
Description Flags
Patch
none
Patch none

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.