Bug 124720

Created attachment 217578 [details] proposed patch no r? now, because we are waiting for bug124150

Created attachment 217579 [details] proposed patch better diff with git diff -M

Created attachment 217580 [details] proposed patch oops, I forgot to git add the soup files

Additional info: I tested this patch on the top of https://bugs.webkit.org/show_bug.cgi?id=124150 on Mac, certificats still work in Safari, Layout tests still pass on WK1 and WK2 too.

Created attachment 217801 [details] proposed patch patch from bug124150 landed (rename PlatformCertificateInfo to CertificateInfo), so we can go forward and move CertificateInfo from WebKit2 to WebCore without any behavioural change. I ran WK1/WK2 layout tests on EFL/Mac and tried Safari with this patch too and it seems certificates still works fine.

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

Comment on attachment 217801 [details] proposed patch fixing GTK build

Created attachment 217803 [details] proposed patch with speculative GTK buildfix

Created attachment 217804 [details] proposed patch one more speculative GTK fix

Created attachment 217806 [details] proposed patch typo fix to make GTK build happy

Created attachment 217938 [details] Patch One more shot, maybe the Mac WK2 bot is a little bit healthier now.

Created attachment 218046 [details] proposed patch Same patch, but forgot that webkit-patch upload doesn't use git -M. Uploaded "git -M" style patch.

Comment on attachment 218046 [details] proposed patch Attachment 218046 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/39318107

Comment on attachment 218046 [details] proposed patch Attachment 218046 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/39348034

Comment on attachment 218046 [details] proposed patch Attachment 218046 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/39138126

Created attachment 218048 [details] proposed patch

Anders, could you review this trivial patch, please? Or any other WK2 owner?

ping?

WK2 owners, please review this trivial patch. Thanks.

Created attachment 218684 [details] Patch

Updated patch to apply cleanly. I am surprised it is so much larger; perhaps I mistakenly included other changes. Clearing review bit while I investigate.

Created attachment 218751 [details] proposed patch Same as attachment218684 [details], but with git diff HEAD -M. (It is smaller and we can easily see the changes after moving.)

Comment on attachment 218751 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=218751&action=review This patch does more than just move the CertificateInfo class into WebCore. It also moves the encoding and decoding functions for certificate info *out* of WebCore. And I don’t know why it does that. > Source/WebCore/platform/network/mac/CertificateInfoMac.mm:-51 > -void CertificateInfo::encode(CoreIPC::ArgumentEncoder& encoder) const What’s the rationale for moving the encode/decode functions out of WebCore? It seems like a bad design to have the serialization far away from the class itself. Easy for the two to get out of sync. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:110 > + CertificateInfo certificate; > + > + if (!decodePlatformData(decoder, certificate)) > + return false; > + > + certificateInfo = certificate; > + > + return true; This should say: return decodePlatformData(decoder, certificateInfo); No need for the rest of that code. Right? I don’t think we need this indirection at all. Instead the functions currently named encodePlatformData and decodePlatformData should just be named encode and decode for CertificateInfo and there should be nothing in this file.

Comment on attachment 218751 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=218751&action=review >> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:-51 >> -void CertificateInfo::encode(CoreIPC::ArgumentEncoder& encoder) const > > What’s the rationale for moving the encode/decode functions out of WebCore? It seems like a bad design to have the serialization far away from the class itself. Easy for the two to get out of sync. Now all WebCore classes are serialized in Shared/WebCoreArgumentCoders.cpp, I don't think if we can make it in a different way only for this class. I'm not responisible for this bad design, maybe Anders or Sam can explain why it is needed. >> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:110 >> + return true; > > This should say: > > return decodePlatformData(decoder, certificateInfo); > > No need for the rest of that code. Right? > > I don’t think we need this indirection at all. Instead the functions currently named encodePlatformData and decodePlatformData should just be named encode and decode for CertificateInfo and there should be nothing in this file. I think yes, we can do it simpler. Good point, ArgumentCoder<KeypressCommand>::encode/decode in Shared/mac/WebCoreArgumentCodersMac.mm exactly do this.

Created attachment 218793 [details] proposed patch

(In reply to comment #24) > Now all WebCore classes are serialized in Shared/WebCoreArgumentCoders.cpp, I don't think > if we can make it in a different way only for this class. I'm not responisible for this bad > design, maybe Anders or Sam can explain why it is needed. But can’t we do that in a different patch? The argument coders are already there in WebCore and this patch moves them into WebKit2. Or am I reading the diff wrong?

(In reply to comment #26) > But can’t we do that in a different patch? The argument coders are already there in WebCore and this patch moves them into WebKit2. Or am I reading the diff wrong? The pretty diff can be a little bit misleading here. Here is an example from the patch: ---------------------------------------------------------------------------- diff --git a/Source/WebKit2/Shared/mac/CertificateInfo.mm b/Source/WebCore/platform/network/mac/CertificateInfoMac.mm similarity index 75% rename from Source/WebKit2/Shared/mac/CertificateInfo.mm rename to Source/WebCore/platform/network/mac/CertificateInfoMac.mm ... ---------------------------------------------------------------------------- First I moved the file and then modified. The pretty diff shows only the changes after moving. git diff -M is a little bit tricky, but it shows better what we have done instead of removing a whole file and adding a whole file. I didn't remove encode/decode from Source/WebCore/platform/network/mac/CertificateInfoMac.mm, but didn't move this logic from WebKit2 to WebCore. Exactly I moved it from Source/WebKit2/Shared/mac/CertificateInfo.mm to Source/WebKit2/Shared/mac/WebCoreArgumentCodersMac.mm, because serialization is a WebKit2 only need that's why serializing WebCore objects stand in WebKit2's WebCoreArgumentCoders instead of WebCore.

I filed a bug against the misleading pretty diff: https://bugs.webkit.org/show_bug.cgi?id=125514 ( And I'm going to find somebody to fix it soon. ;) )

WK2 owners, could you review it, please?

Comment on attachment 218793 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=218793&action=review > Source/WebKit2/ChangeLog:38 > + * Shared/WebCoreArgumentCoders.cpp: > + (CoreIPC::::encode): > + (CoreIPC::::decode): > + * Shared/WebCoreArgumentCoders.h: > + * Shared/mac/WebCoreArgumentCodersMac.mm: > + (CoreIPC::::encodePlatformData): > + (CoreIPC::::decodePlatformData): > + * Shared/soup/CertificateInfo.cpp: > + * Shared/soup/WebCoreArgumentCodersSoup.cpp: > + (CoreIPC::::encodePlatformData): > + (CoreIPC::::decodePlatformData): prepare-ChangeLog did not do a good job with these function names; normally I fix those by hand when I see them in my patches’ change logs, but fixing the script would be even better > Source/WebKit2/WebProcess/WebProcess.h:56 > +#if !ENABLE(NETWORK_PROCESS) && USE(SOUP) > +class CertificateInfo; > +#endif I don’t think we need to wrap this forward declaration in #if. Generally speaking I think our style in the future should be to not waste time putting forward declarations inside #if statements since they are much uglier like that, and omitting them catches almost no errors that we wouldn’t catch other ways.

Comment on attachment 218793 [details] proposed patch Rejecting attachment 218793 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 218793, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: bCoreSupport/WebFrameLoaderClient.cpp patching file Source/WebKit2/WebProcess/WebProcess.h Hunk #1 succeeded at 55 (offset 4 lines). Hunk #2 succeeded at 79 (offset 4 lines). Hunk #3 succeeded at 177 (offset 4 lines). patching file Source/WebKit2/WebProcess/WebProcess.messages.in patching file Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/48148046

Created attachment 219068 [details] patch for landing changelog fixed, ifdef guard around forward declaration removed, conflict in Source/WebKit2/Shared/WebCertificateInfo.h after r160384 resolved

Landed in http://trac.webkit.org/changeset/160487

(In reply to comment #30) > > Source/WebKit2/ChangeLog:38 > > + * Shared/WebCoreArgumentCoders.cpp: > > + (CoreIPC::::encode): > > + (CoreIPC::::decode): > > + * Shared/WebCoreArgumentCoders.h: > > + * Shared/mac/WebCoreArgumentCodersMac.mm: > > + (CoreIPC::::encodePlatformData): > > + (CoreIPC::::decodePlatformData): > > + * Shared/soup/CertificateInfo.cpp: > > + * Shared/soup/WebCoreArgumentCodersSoup.cpp: > > + (CoreIPC::::encodePlatformData): > > + (CoreIPC::::decodePlatformData): > > prepare-ChangeLog did not do a good job with these function names; normally I fix those by hand when I see them in my patches’ change logs, but fixing the script would be even better I filed a new bug report about this prepare-ChangeLog bug: bug125853