Bug 118520

Sounds reasonable to me.

Created attachment 206453 [details] Patch

Created attachment 206454 [details] Patch

(In reply to comment #3) > Created an attachment (id=206454) [details] > Patch I am not sure why patch does not apply because I rebased the patch up to the HEAD before uploading. This is a preparation for https://bugs.webkit.org/show_bug.cgi?id=110141#c11

Created attachment 206500 [details] Patch

(In reply to comment #5) > Created an attachment (id=206500) [details] > Patch Qt build fix.

Created attachment 206507 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Attachment 206507 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/PlatformCertificateInfo.h', u'Source/WebCore/platform/network/ResourceErrorBase.h', u'Source/WebCore/platform/network/ResourceResponseBase.h', u'Source/WebCore/platform/network/cf/ResourceResponse.h', u'Source/WebCore/platform/network/mac/PlatformCertificateInfoMac.mm', u'Source/WebCore/platform/network/mac/ResourceResponseMac.mm', u'Source/WebCore/platform/network/qt/PlatformCertificateInfoQt.cpp', u'Source/WebCore/platform/network/soup/PlatformCertificateInfoSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceError.h', u'Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceResponse.h', u'Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/NetworkProcess/NetworkProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.messages.in', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/API/c/mac/WKCertificateInfoMac.mm', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.h', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.messages.in', u'Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm', u'Source/WebKit2/Shared/UserMessageCoders.h', u'Source/WebKit2/Shared/WebCertificateInfo.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/Shared/WebCoreArgumentCoders.h', u'Source/WebKit2/Shared/mac/PlatformCertificateInfo.h', u'Source/WebKit2/Shared/mac/PlatformCertificateInfo.mm', u'Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm', u'Source/WebKit2/Shared/qt/PlatformCertificateInfo.h', u'Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp', u'Source/WebKit2/Shared/soup/PlatformCertificateInfo.cpp', u'Source/WebKit2/Shared/soup/PlatformCertificateInfo.h', u'Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp', u'Source/WebKit2/Target.pri', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/Authentication/AuthenticationChallengeProxy.cpp', u'Source/WebKit2/UIProcess/WebFrameProxy.cpp', u'Source/WebKit2/UIProcess/WebFrameProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.h', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.messages.in', u'Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp']" exit_code: 1 Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:111: Use 0 instead of NULL. [readability/null] [5] Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:111: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build fix for Mac Debug.

The reason I haven't gotten to reviewing this yet is because it's very large and will require a block of time I don't have.

(In reply to comment #11) > The reason I haven't gotten to reviewing this yet is because it's very large and will require a block of time I don't have. Okay. Please take your time.

Created attachment 209637 [details] Patch Updated to ToT - r154595

(In reply to comment #13) > Created an attachment (id=209637) [details] > Patch > > Updated to ToT - r154595 Short summary: - trivial conflicts resolved in WebCore.exp.in, project.pbxproj, NetworkResourceLoader.cpp, WebPageProxy.h and WebFrameLoaderClient.cpp - trivial fix in the new AsynchronousNetworkLoaderClient.cpp - style fixed (NULL -> 0 and smaller indentation in WebCoreArgumentCodersSoup.cpp) - obsolete change removed from NetworkResourceLoader.cpp - unneeded typo removed from ResourceHandleSoup.cpp

Comment on attachment 209637 [details] Patch Attachment 209637 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1559859

Comment on attachment 209637 [details] Patch Attachment 209637 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1571082

Comment on attachment 209637 [details] Patch fixing the build

Created attachment 209640 [details] Patch Additional Mac and Soup buildfixes

Comment on attachment 209640 [details] Patch Attachment 209640 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1586234

Created attachment 209746 [details] Patch Fixed the if guard of including RetainPtr.h in PlatformCertificateInfo.h to make Windows build happy.

Comment on attachment 209746 [details] Patch Attachment 209746 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1583287

Created attachment 210347 [details] Patch patch for Win EWS, try to add PlatformCertificateInfoCFNet.cpp with empty constructor and destructor to make the build happy.

Comment on attachment 210347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210347&action=review > Source/WebCore/ChangeLog:1 > +2013-09-03 Kwang Yul Seo <skyul@company100.net> If this has been co-authored you should indicate it in the changelog header :) > Source/WebCore/platform/network/ResourceErrorBase.h:96 > + PlatformCertificateInfo m_platformCertificateInfo; I think you it should be added to ResourceError, not ResourceErrorBase. > Source/WebCore/platform/network/ResourceResponseBase.h:159 > + PlatformCertificateInfo m_platformCertificateInfo; Same thing for ResourceResponse.

Created attachment 212703 [details] Patch

(In reply to comment #23) > (From update of attachment 210347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210347&action=review > > > Source/WebCore/ChangeLog:1 > > +2013-09-03 Kwang Yul Seo <skyul@company100.net> > > If this has been co-authored you should indicate it in the changelog header :) It hasn't been really co-authored. Almost the full patch is the work of Kwang Yul Seo, I only fixed minor things. But you are right, I mentioned everything in the changelogs. > > Source/WebCore/platform/network/ResourceErrorBase.h:96 > > + PlatformCertificateInfo m_platformCertificateInfo; > > I think you it should be added to ResourceError, not ResourceErrorBase. I don't think if it is a good idea, m_platformCertificateInfo is used by platformCertificateInfo() and setPlatformCertificateInfo() in the same header. Why should we add this cross platform implementation to the 6 subclasses of ResourceErrorBase with copy/pasting instead of leaving it in the ResourceErrorBase? > > Source/WebCore/platform/network/ResourceResponseBase.h:159 > > + PlatformCertificateInfo m_platformCertificateInfo; > > Same thing for ResourceResponse. Ditto.

Could you review this 2.5 months old patch please? (Rebasing this huge patch to ToT again and again is a little bit burdensome.)

Created attachment 212804 [details] Patch updated to ToT: resolved a trivial conflict

Created attachment 212957 [details] Patch rename KURL to URL after r156550

Comment on attachment 212957 [details] Patch Attachment 212957 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/2613628

Created attachment 213036 [details] Patch

Comment on attachment 213036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213036&action=review soup changes look good to me, except for the few comments below. r- just because of the SoupMessageFlags initialization removal in ResourceResponse. > Source/WebCore/platform/network/PlatformCertificateInfo.h:35 > +#include <wtf/gobject/GRefPtr.h> We can probably include <gio/gio.h> since we are not using any libsoup API here. > Source/WebCore/platform/network/PlatformCertificateInfo.h:40 > +class ResourceError; > +class ResourceResponse; These look unused here. > Source/WebCore/platform/network/soup/PlatformCertificateInfoSoup.cpp:31 > +#include "ResourceError.h" > +#include "ResourceResponse.h" These are no longer needed either. > Source/WebCore/platform/network/soup/PlatformCertificateInfoSoup.cpp:32 > +#include <libsoup/soup.h> This is already included in PlatformCertificateInfo.h, I know this is also present in current code, though. > Source/WebCore/platform/network/soup/ResourceResponse.h:-40 > - , m_soupFlags(static_cast<SoupMessageFlags>(0)) You should keep the message flags initialization. > Source/WebCore/platform/network/soup/ResourceResponse.h:-47 > - , m_soupFlags(static_cast<SoupMessageFlags>(0)) Ditto. > Source/WebCore/platform/network/soup/ResourceResponse.h:-54 > - , m_soupFlags(static_cast<SoupMessageFlags>(0)) Ditto.

Created attachment 213359 [details] Patch Patch for EWS. Removed Qt changes, added back initializers for m_soupFlags. I'll check other things mentioned in the review later.

Created attachment 213595 [details] Patch

(In reply to comment #33) > Created an attachment (id=213595) [details] > Patch I fixed everything mentioned in https://bugs.webkit.org/show_bug.cgi?id=118520#c31 .

Comment on attachment 213595 [details] Patch soup changes look good to me, thanks ossy.

(In reply to comment #35) > (From update of attachment 213595 [details]) > soup changes look good to me, thanks ossy. Can we get a sign-off from a WK2 owner, please?

Can’t this just be called CertificateInfo? I don’t think we should be using the Platform prefix for classes that are in the platform layer.

(In reply to comment #37) > Can’t this just be called CertificateInfo? I don’t think we should be using the Platform prefix for classes that are in the platform layer. Of course I can rename PlatformCertificateInfo to CertificateInfo. Have you got any other remark for this patch? Apart from this rename thing, does it look good for you?

Created attachment 213867 [details] Patch patch for EWS with renaming

Created attachment 213868 [details] Patch patch for EWS with renaming

Comment on attachment 213868 [details] Patch Attachment 213868 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3777052

Comment on attachment 213868 [details] Patch Attachment 213868 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3777053

Comment on attachment 213868 [details] Patch Attachment 213868 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/3400074

Created attachment 213870 [details] Patch

Created attachment 213871 [details] Patch

Comment on attachment 213871 [details] Patch Attachment 213871 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3776053

Comment on attachment 213871 [details] Patch Attachment 213871 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3779039

Created attachment 213872 [details] Patch

Comment on attachment 213872 [details] Patch Attachment 213872 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/3721051

Created attachment 213874 [details] Patch

Comment on attachment 213874 [details] Patch EWS bots are green (except the non working GTK-WK2), so marking it as r?

Comment on attachment 213595 [details] Patch mark as obsolete based on comment #37

(In reply to comment #37) > Can’t this just be called CertificateInfo? I don’t think we should be using the Platform prefix for classes that are in the platform layer. (In reply to comment #50) > Created an attachment (id=213874) [details] > Patch I did the asked renaming. Is there anything else should I fix?

(In reply to comment #51) > (From update of attachment 213874 [details]) > EWS bots are green (except the non working GTK-WK2), so marking it as r? Applied locally, I confirm it builds fine in gtk-wk2

Anders or Brady, could you review it please?

I honestly don't have time to meaningfully review a 100k patch that touches so many sites right now.

Comment on attachment 213874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213874&action=review > Source/WebCore/platform/network/CertificateInfo.h:67 > + // Certificate chain is normally part of NS/CFURLResponse, but there is no way to re-add it to a deserialized response after IPC. I don't think this comment makes any sense. > Source/WebCore/platform/network/soup/ResourceError.h:60 > + unsigned tlsErrors() const { return m_certificateInfo.tlsErrors(); } > + void setTLSErrors(unsigned tlsErrors) { m_certificateInfo.setTLSErrors(static_cast<GTlsCertificateFlags>(tlsErrors)); } > + GTlsCertificate* certificate() const { return m_certificateInfo.certificate(); } > + void setCertificate(GTlsCertificate* certificate) { m_certificateInfo.setCertificate(certificate); } Can't we get rid of these and just use calls on m_certificateInfo directly? > Source/WebKit2/Shared/API/c/mac/WKCertificateInfoMac.mm:36 > + RefPtr<WebCertificateInfo> certificateInfo = WebCertificateInfo::create(WebCore::CertificateInfo(certificateChain)); Please add using directive for the WebCore namespace so you don't have to use the WebCore prefix here. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:564 > + Extra newline.

Comment on attachment 213874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213874&action=review >> Source/WebCore/platform/network/CertificateInfo.h:67 >> + // Certificate chain is normally part of NS/CFURLResponse, but there is no way to re-add it to a deserialized response after IPC. > > I don't think this comment makes any sense. What is the problem with it? We didn't touch this code/comment. It was originally added by http://trac.webkit.org/changeset/138206 >> Source/WebCore/platform/network/soup/ResourceError.h:60 >> + void setCertificate(GTlsCertificate* certificate) { m_certificateInfo.setCertificate(certificate); } > > Can't we get rid of these and just use calls on m_certificateInfo directly? Yes, we can. I'll do it before landing. >> Source/WebKit2/Shared/API/c/mac/WKCertificateInfoMac.mm:36 >> + RefPtr<WebCertificateInfo> certificateInfo = WebCertificateInfo::create(WebCore::CertificateInfo(certificateChain)); > > Please add using directive for the WebCore namespace so you don't have to use the WebCore prefix here. OK. >> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:564 >> + > > Extra newline. OK.

Created attachment 214157 [details] patch for attachment213874 [details]

Created attachment 214158 [details] Patch patch for landing

Comment on attachment 213874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213874&action=review >>> Source/WebCore/platform/network/soup/ResourceError.h:60 >>> + void setCertificate(GTlsCertificate* certificate) { m_certificateInfo.setCertificate(certificate); } >> >> Can't we get rid of these and just use calls on m_certificateInfo directly? > > Yes, we can. I'll do it before landing. Hmmmm, it seems we can't, because https://bugs.webkit.org/attachment.cgi?id=213881&action=review use these setters.

Created attachment 214160 [details] Patch patch for landing

(In reply to comment #61) > (From update of attachment 213874 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213874&action=review > > >>> Source/WebCore/platform/network/soup/ResourceError.h:60 > >>> + void setCertificate(GTlsCertificate* certificate) { m_certificateInfo.setCertificate(certificate); } > >> > >> Can't we get rid of these and just use calls on m_certificateInfo directly? > > > > Yes, we can. I'll do it before landing. > > Hmmmm, it seems we can't, because https://bugs.webkit.org/attachment.cgi?id=213881&action=review use these setters. That's not a problem, that patch will have to be reworked anyway since it also uses PlatformCertificateInfo instead of CertificateInfo.

Comment on attachment 214158 [details] Patch Clearing flags on attachment: 214158 Committed r157445: <http://trac.webkit.org/changeset/157445>

(In reply to comment #64) > (From update of attachment 214158 [details]) > Clearing flags on attachment: 214158 > > Committed r157445: <http://trac.webkit.org/changeset/157445> Thanks, I landed the original patch which removed the functions asked in https://bugs.webkit.org/show_bug.cgi?id=118520#c57.

Re-opened since this is blocked by bug 122825

(In reply to comment #64) > (From update of attachment 214158 [details]) > Clearing flags on attachment: 214158 > > Committed r157445: <http://trac.webkit.org/changeset/157445> just for note: the necessary buildfixes landed in - http://trac.webkit.org/changeset/157446 - http://trac.webkit.org/changeset/157447

Reopen, because Anders reverted it with the following commit message: http://trac.webkit.org/changeset/157842 Revert r157445 since it broke certificates on Mac. <rdar://problem/15246926&15254017&15269117> Sorry for the breakage. Anders, could you give us some hint about what happened? I haven't seen any failures on the Apple buildbots after landing. "it broke certificates on Mac." isn't enough for me to try to fix this regression.

Otherwise do we _really_ need this monumental change? Originally Balázs and Kwang suggested the following change in https://bugs.webkit.org/show_bug.cgi?id=110141 : diff --git a/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp b/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp index 5357bbcf44004cd3a52477e5636d5ab72cd24cb6..603895660f599f486502039b84f4e40ffe686e0f 100644 --- a/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp +++ b/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp @@ -106,7 +106,9 @@ void WebResourceLoader::didReceiveResponseWithCertificateInfo(const ResourceResp RefPtr<WebResourceLoader> protector(this); ResourceResponse responseCopy(response); +#if USE(CFNETWORK) responseCopy.setCertificateChain(certificateInfo.certificateChain()); +#endif m_coreLoader->didReceiveResponse(responseCopy); // If m_coreLoader becomes null as a result of the didReceiveResponse callback, we can't use the send function(). But Brady didn't want this oneliner and suggested this monunental one: "I understand this is (currently) necessary because of code in WebCore, which is too bad. We should push "PlatformCertificateInfo" to WebCore and make the ResourceRequest/Response primitives work in terms of that platform agnostic object instead of CF-specific types."

EFL buildfix after the imperfect rollout - http://trac.webkit.org/changeset/157850 .

and the GTK build is still broken because of the rollout, see https://bugs.webkit.org/show_bug.cgi?id=120160 for details.

Otherwise rolling out the patch without any notification on IRC and/or comment in this bug (with Apple only radar link) was a little bit unfriendly. :( Would you be so kind to notice the author before/after the rollout next time please? And please add more detailed infos how to reproduce the regression caused. Thanks.

(In reply to comment #69) > We should push "PlatformCertificateInfo" to WebCore and make the ResourceRequest/Response primitives work in terms of that platform agnostic object instead of CF-specific types." I still think this makes a lot of sense, we just need to know what's broken and try to fix it. Anders?

Anders, would you be so kind to share with us what was the problem with the patch you rolled out? It would be great to have any information to be able to fix the mistake we made. Thanks.

(In reply to comment #74) > Anders, would you be so kind to share with us what was the problem with the patch > you rolled out? It would be great to have any information to be able to fix the > mistake we made. Thanks. Oops, I thought I had done that. The reason was that on the Mac port we didn't get any certificates so a lot of Safari features that depend on having correct certificate info broke. I tried digging through the patch to fix the issue but couldn't due to its sheer size. I think a good next step here is to do the rename + move to WebCore as a separate step (maybe even two steps, first rename and then move). Once that's done it's easier to re-review the remaning bits.

I split up the original patch into 3 bugs: - Rename PlatformCertificateInfo to CertificateInfo (bug124150) - Move CertificateInfo to WebCore (bug124720) - Make the ResourceRequest/Response primitives work in terms of platform agnostic CertificateInfo object (bug124724)