Bug 121423 – CookieStorageShim should be PLATFORM(MAC) guarded

• Component: WebKit2
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

CookieStorageShim is used by only Mac, so all of its use should be guarded.

[Csaba Osztrogonác]

Created attachment 211761 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 211761 [details]
Patch

Shouldn't this code be moved to platformInitializeWebProcess instead of adding ifdefs?

I'm not quite sure about iOS implications, but as long as the patch doesn't change behavior, it should be fine to land. The current one doesn't change behavior, and one that changes platformInitializeWebProcess shouldn't either.

[Csaba Osztrogonác]

I checked the git history and I found this CookieStorageShim was introduced
in http://trac.webkit.org/changeset/149074. The commit log mentioned this
change is related to AVFoundation and CFNetwork.

I have no idea if the Linux ports will need a similar implementation 
in the future or not. Now a buildable and more or less working 
NetworkProcess is my first priority. Bugfixes can be done after that.


(In reply to comment #2)
> (From update of attachment 211761 [details])
> Shouldn't this code be moved to platformInitializeWebProcess instead of adding ifdefs?
> 
> I'm not quite sure about iOS implications, but as long as the patch doesn't change behavior, it should be fine to land. The current one doesn't change behavior, and one that changes platformInitializeWebProcess shouldn't either.

I don't know anything about the iOS implications, because iOS port isn't
fully upstreamed yet. Otherwise I thought there isn't WebKit2 on iOS at
all - https://lists.webkit.org/pipermail/webkit-dev/2013-February/023735.html
Of course it might be changed in the last half year. :)

I'm not sure if it is safe to simple move this code to the
platformInitializeWebProcess. There is an ensureNetworkProcessConnection(); call before the CookieStorageShim::shared().initialize(); call now.
Is it really needed or not for CookieStorageShim initialization?

So I still think that the safest way for the fix is to leave 
this code here and simple adding the PLATFORM(MAC) ifdef guard.

[Csaba Osztrogonác]

It seems I wasn't up-to-date, WebKit2 on IOS already exists:
- http://trac.webkit.org/changeset/141025/trunk/Source/WebKit2/WebProcess/WebCoreSupport/mac/WebSystemInterface.mm
- http://trac.webkit.org/changeset/156350

But I don't think if I should now if you use CookieStorageShim in the secret
IOS branch or not. In the public WebKit trunk it is used by only PLATFORM(MAC),
so the proposed guard is correct.

[Darin Adler]

PLATFORM(MAC) is currently and has always been true for the iOS port.

[Csaba Osztrogonác]

(In reply to comment #5)
> PLATFORM(MAC) is currently and has always been true for the iOS port.

In this case it should be safe to land it. Can I have an r+ for it please?

[Darin Adler]

Anders, do you have any objection to doing this through a PLATFORM(MAC) conditional? Is this Mac-specific?

[WebKit Commit Bot]

Comment on attachment 211761 [details]
Patch

Clearing flags on attachment: 211761

Committed r156589: <http://trac.webkit.org/changeset/156589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.