Bug 169486

Summary:

Cleanup "addCookie" and its sole user

Product:

WebKit

Reporter:

Brady Eidson <beidson>

Component:

WebKit2

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, bburg, berto, cdumez, cgarcia, commit-queue, danw, dbates, galpeter, gustavo, japhet, mcatanzaro, mrobinson, ossy

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

169546

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	bburg: review+
Patch for landing	none

Brady Eidson

Reported 2017-03-10 15:17:00 PST

Cleanup "addCookie" and its sole user addCookie doesn't cleanly fit in with the native cookie storage API, and a lot of dead code was added to support it as well. Refactoring this to be "setCookies" like NSHTTPCookieStorage supports will be better going forward.

Attachments
Patch (47.06 KB, patch) 2017-03-10 16:25 PST, Brady Eidson	no flags	Details Formatted Diff Diff
Patch (47.71 KB, patch) 2017-03-10 19:19 PST, Brady Eidson	no flags	Details Formatted Diff Diff
Patch (47.83 KB, patch) 2017-03-10 19:55 PST, Brady Eidson	bburg: review+	Details Formatted Diff Diff
Patch for landing (46.21 KB, patch) 2017-03-11 03:09 PST, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Brady Eidson

Comment 1 2017-03-10 16:25:11 PST

Created attachment 304098 [details] Patch

Brady Eidson

Comment 2 2017-03-10 16:26:00 PST

Already reviewed. Waiting for EWS.

Brady Eidson

Comment 3 2017-03-10 19:19:38 PST

Created attachment 304117 [details] Patch

Brady Eidson

Comment 4 2017-03-10 19:20:32 PST

Attempt to fix GTK, and hopefully the rest of the EWS bots will actually be responsive this time.

Brady Eidson

Comment 5 2017-03-10 19:55:29 PST

Created attachment 304120 [details] Patch

Blaze Burg

Comment 6 2017-03-10 22:56:37 PST

Comment on attachment 304120 [details] Patch r=me GTK is still sad.

Carlos Garcia Campos

Comment 7 2017-03-11 03:09:47 PST

Created attachment 304160 [details] Patch for landing

Carlos Garcia Campos

Comment 8 2017-03-11 03:15:32 PST

(In reply to comment #6) > Comment on attachment 304120 [details] > Patch > > r=me > > GTK is still sad. This is a known issue with our forwarding headers generator. It doesn't scan generated sources, so in this case #include <WebCoreCookie.h> is only added to the generated header DerivedSources/WebKit2/WebCookieManagerMessages.h. WebCookieManager.cpp needs that include, but it doesn't include it explicitly becase it already includes WebCookieManagerMessages.h. Our forwarding header generator doesn't generate the forwarding header in this case. To fix it we only need to make sure the header is included in a non-generated source file, in this case WebCookieManager.cpp. We could probably change the generator to also scan generated sources, ensuring the script is always run after all generated sources during the build. I'm not sure how easy that would be, though.

WebKit Commit Bot

Comment 9 2017-03-11 08:44:04 PST

Comment on attachment 304160 [details] Patch for landing Clearing flags on attachment: 304160 Committed r213759: <http://trac.webkit.org/changeset/213759>

WebKit Commit Bot

Comment 10 2017-03-11 08:44:10 PST

All reviewed patches have been landed. Closing bug.

Michael Catanzaro

Comment 11 2017-03-11 10:17:52 PST

(In reply to comment #8) > This is a known issue with our forwarding headers generator. It doesn't scan > generated sources, so in this case #include <WebCoreCookie.h> is only added > to the generated header DerivedSources/WebKit2/WebCookieManagerMessages.h. > WebCookieManager.cpp needs that include, but it doesn't include it > explicitly becase it already includes WebCookieManagerMessages.h. Our > forwarding header generator doesn't generate the forwarding header in this > case. To fix it we only need to make sure the header is included in a > non-generated source file, in this case WebCookieManager.cpp. We could > probably change the generator to also scan generated sources, ensuring the > script is always run after all generated sources during the build. I'm not > sure how easy that would be, though. Is there a bug report for this...?

Csaba Osztrogonác

Comment 12 2017-03-12 00:20:17 PST

(In reply to comment #9) > Comment on attachment 304160 [details] > Patch for landing > > Clearing flags on attachment: 304160 > > Committed r213759: <http://trac.webkit.org/changeset/213759> It broke the Apple Mac cmake build.

Csaba Osztrogonác

Comment 13 2017-03-13 10:29:20 PDT

Just to document, the Windows build was fixed in bug169546

Note You need to log in before you can comment on or make changes to this bug.