Bug 158809 - [Cocoa] Clean up / optimize ResourceResponse::platformLazyInit(InitLevel)
Summary: [Cocoa] Clean up / optimize ResourceResponse::platformLazyInit(InitLevel)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-15 14:47 PDT by Chris Dumez
Modified: 2016-07-06 08:39 PDT (History)
3 users (show)

See Also:

Attachments
Patch (13.18 KB, patch)
2016-06-15 15:02 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.19 KB, patch)
2016-06-15 15:21 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2016-06-15 16:58 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2016-06-15 17:16 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2016-06-15 19:06 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2016-06-15 19:28 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2016-06-15 19:58 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) 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 Chris Dumez 2016-06-15 14:47:54 PDT 
Clean up / optimize ResourceResponse::platformLazyInit(InitLevel).
Comment 1 Chris Dumez 2016-06-15 15:02:27 PDT 
Created attachment 281388 [details]
Patch
Comment 2 Chris Dumez 2016-06-15 15:21:34 PDT 
Created attachment 281392 [details]
Patch
Comment 3 Chris Dumez 2016-06-15 16:58:12 PDT 
Created attachment 281403 [details]
Patch
Comment 4 Chris Dumez 2016-06-15 17:16:32 PDT 
Created attachment 281408 [details]
Patch
Comment 5 Darin Adler 2016-06-15 17:53:00 PDT 
Comment on attachment 281408 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:81
> +AtomicString extractReasonPhraseFromHTTPStatusLine(const String&);

I suggest changing the argument from const String& to StringView.

> Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:171
> +    CFHTTPMessageRef messageRef = CFURLResponseGetHTTPResponse([httpResponse _CFURLResponse]);

I suggest using auto here.

> Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:174
> +    RetainPtr<CFDictionaryRef> headers = adoptCF(CFHTTPMessageCopyAllHeaderFields(messageRef));

I suggest using auto for the result of adoptCF.

> Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:179
> +            CFStringRef value;
> +            if (CFDictionaryGetValueIfPresent(headers.get(), commonHeader, (const void **)&value))
> +                headersMap.set(commonHeader, value);

I suggest using const void* as the type of the local variable and doing the typecast when calling set.
Comment 6 Chris Dumez 2016-06-15 18:16:05 PDT 
Comment on attachment 281408 [details]
Patch

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

>> Source/WebCore/platform/network/HTTPParsers.h:81
>> +AtomicString extractReasonPhraseFromHTTPStatusLine(const String&);
> 
> I suggest changing the argument from const String& to StringView.

I tried doing this but the issue was that the call sites have a CFStringRef (not a String). StringView does not have a constructor that takes a CFStringRef so using StringView here would force the call sites to explicitly construct a String.
Comment 7 Chris Dumez 2016-06-15 19:06:21 PDT 
Created attachment 281424 [details]
Patch
Comment 8 Chris Dumez 2016-06-15 19:28:14 PDT 
Created attachment 281427 [details]
Patch
Comment 9 Chris Dumez 2016-06-15 19:58:47 PDT 
Created attachment 281430 [details]
Patch
Comment 10 Chris Dumez 2016-06-15 19:59:57 PDT 
Committed r202121: <http://trac.webkit.org/changeset/202121>
Comment 11 Chris Dumez 2016-06-16 09:30:03 PDT 
Looks like this may have been a 1% PLT progression on MacBookPro, working on confirming.
Comment 12 Chris Dumez 2016-07-06 08:39:21 PDT 
(In reply to comment #11)
> Looks like this may have been a 1% PLT progression on MacBookPro, working on
> confirming.

Confirmed progression on PLT.