Bug 61255

Summary: [Windows] Initial Cookie Accept Policy not set on Deserialized Storage Session
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: WebKit2Assignee: Jessie Berlin <jberlin>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, jberlin, sfalken
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: Windows 7   
Attachments:
Description Flags
Patch none

Description Jessie Berlin 2011-05-22 16:46:48 PDT
<rdar://problem/9483885>
Comment 1 Jessie Berlin 2011-05-22 16:55:46 PDT
Created attachment 94357 [details]
Patch
Comment 2 Darin Adler 2011-05-22 22:08:09 PDT
Comment on attachment 94357 [details]
Patch

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

> Source/WebKit2/WebProcess/win/WebProcessWin.cpp:144
> +    WebCookieManager::shared().setHTTPCookieAcceptPolicy(parameters.initialHTTPCookieAcceptPolicy);

Moving this one line up seems like the bug fix. The other changes seem OK, but I don’t see why they have to go along with this bug fix in the same patch. They could be done separately before or after.

> Source/WebKit2/WebProcess/win/WebProcessWin.cpp:147
> +    // By using the default storage session that came from the UI Process, the Web Process
> +    // automatically use the same the URL Cache as UI Process.

I suggest not capitalizing terms such as “web process”. Also, the second line should read:

    // automatically uses the same URL cache as the UI process.
Comment 3 Darin Adler 2011-05-22 22:08:25 PDT
r=me whether you decide to break this up in pieces or not
Comment 4 Jessie Berlin 2011-05-23 10:14:57 PDT
(In reply to comment #2)
> (From update of attachment 94357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94357&action=review
> 
> > Source/WebKit2/WebProcess/win/WebProcessWin.cpp:144
> > +    WebCookieManager::shared().setHTTPCookieAcceptPolicy(parameters.initialHTTPCookieAcceptPolicy);
> 
> Moving this one line up seems like the bug fix. The other changes seem OK, but I don’t see why they have to go along with this bug fix in the same patch. They could be done separately before or after.

It is true they could be done separately, but I without them the code with my change is harder to read (which was my motivation for making those changes now). I think I am going to keep them (per your other comment) for that reason.

> 
> > Source/WebKit2/WebProcess/win/WebProcessWin.cpp:147
> > +    // By using the default storage session that came from the UI Process, the Web Process
> > +    // automatically use the same the URL Cache as UI Process.
> 
> I suggest not capitalizing terms such as “web process”. Also, the second line should read:
> 
>     // automatically uses the same URL cache as the UI process.

Fixed.

Thanks for the review!
Comment 5 Jessie Berlin 2011-05-23 10:36:24 PDT
Comment on attachment 94357 [details]
Patch

Committed in http://trac.webkit.org/changeset/87077