Bug 61255 – [Windows] Initial Cookie Accept Policy not set on Deserialized Storage Session

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jessie Berlin]

<rdar://problem/9483885>

[Jessie Berlin]

Created attachment 94357 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

r=me whether you decide to break this up in pieces or not

[Jessie Berlin]

(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!

[Jessie Berlin]

Comment on attachment 94357 [details]
Patch

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