Bug 61255 - [Windows] Initial Cookie Accept Policy not set on Deserialized Storage Session
Summary: [Windows] Initial Cookie Accept Policy not set on Deserialized Storage Session
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 7
: P2 Normal
Assignee: Jessie Berlin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-22 16:46 PDT by Jessie Berlin
Modified: 2011-05-23 10:36 PDT (History)
3 users (show)

See Also:

Attachments
Patch (2.66 KB, patch)
2011-05-22 16:55 PDT, Jessie Berlin 		no flags Details | Formatted Diff | Diff
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 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