Bug 52637

Summary: WebCore should call alternate CFHTTPCookie functions, if available
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: PlatformAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch darin: review+

Description Adam Roben (:aroben) 2011-01-18 10:15:57 PST
WebCore should call alternate CFHTTPCookie functions, if available
Comment 1 Adam Roben (:aroben) 2011-01-18 10:17:15 PST
Created attachment 79292 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-01-18 10:18:19 PST
<rdar://problem/8878984>
Comment 3 Geoffrey Garen 2011-01-18 10:23:02 PST
> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:60
> +static RetainPtr<CFStringRef> cookieDomain(CFHTTPCookieRef cookie)

Can these wrappers be inlined?
Comment 4 Adam Roben (:aroben) 2011-01-18 10:26:28 PST
(In reply to comment #3)
> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:60
> > +static RetainPtr<CFStringRef> cookieDomain(CFHTTPCookieRef cookie)
> 
> Can these wrappers be inlined?

If you mean "Can you add the 'inline' keyword to these wrappers?", the answer is of course "yes". If you mean "Is it possible for the compiler to inline these wrappers, regardless of whether you specified the 'inline' keyword?", the answer is "I don't know, but I'd hope so."

Are you suggesting I add the keyword? Do you think it would make a difference?
Comment 5 Geoffrey Garen 2011-01-18 10:35:27 PST
Heh -- I'm suggesting adding the keyword.

All of the RetainPtr stuff inside these functions is inlined, so inlining these functions should produce good results.

That said, I doubt this is performance-critical code.
Comment 6 Adam Roben (:aroben) 2011-01-18 10:38:38 PST
(In reply to comment #5)
> Heh -- I'm suggesting adding the keyword.
> 
> All of the RetainPtr stuff inside these functions is inlined, so inlining these functions should produce good results.
> 
> That said, I doubt this is performance-critical code.

Thanks for the clarification. Do you think the compiler is unlikely to inline these functions on its own?
Comment 7 Geoffrey Garen 2011-01-18 11:00:55 PST
> Do you think the compiler is unlikely to inline these functions on its own?

I'm always surprised by what the compiler chooses to inline automatically. In theory, "inline" is a hint to consider something for inlining, but compilers may ignore the hint, or may inline without the hint. Some compilers require you to set an explicit "inline even functions not marked inline" flag.

I'm told that LLVM ignores the inline keyword altogether, and makes its own decisions about each function. I'm not sure what other compilers like MSVC and GCC do.

Given all this uncertainty, I usually add the inline keyword to good-looking functions.
Comment 8 Adam Roben (:aroben) 2011-01-18 11:09:19 PST
(In reply to comment #7)
> > Do you think the compiler is unlikely to inline these functions on its own?
> 
> I'm always surprised by what the compiler chooses to inline automatically. In theory, "inline" is a hint to consider something for inlining, but compilers may ignore the hint, or may inline without the hint. Some compilers require you to set an explicit "inline even functions not marked inline" flag.
> 
> I'm told that LLVM ignores the inline keyword altogether, and makes its own decisions about each function. I'm not sure what other compilers like MSVC and GCC do.

MSDN says that MSVC treats "inline" as a hint. I also know that MSVC will sometimes inline functions that don't have the "inline" keyword. MSVC also has a __forceinline keyword, which lets you give a much stronger hint to the compiler.

> Given all this uncertainty, I usually add the inline keyword to good-looking functions.

OK, I'll add them. Thanks!
Comment 9 Adam Roben (:aroben) 2011-01-18 11:15:32 PST
Committed r76041: <http://trac.webkit.org/changeset/76041>
Comment 10 Geoffrey Garen 2011-01-21 20:30:20 PST
Following up, here's an example where I just had to mark a one line forwarding function inline in order to get GCC to do the right thing: http://trac.webkit.org/changeset/76425.