Bug 124720 – Move CertificateInfo to WebCore

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

It is the 2nd step after renaming PlatformCertificateInfo to CertificateInfo (bug124150)

[Csaba Osztrogonác]

Created attachment 217578 [details]
proposed patch

no r? now, because we are waiting for bug124150

[Csaba Osztrogonác]

Created attachment 217579 [details]
proposed patch

better diff with git diff -M

[Csaba Osztrogonác]

Created attachment 217580 [details]
proposed patch

oops, I forgot to git add the soup files

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[WebKit Commit Bot]

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

[Csaba Osztrogonác]

Comment on attachment 217801 [details]
proposed patch

fixing GTK build

[Csaba Osztrogonác]

Created attachment 217803 [details]
proposed patch

with speculative GTK buildfix

[Csaba Osztrogonác]

Created attachment 217804 [details]
proposed patch

one more speculative GTK fix

[Csaba Osztrogonác]

Created attachment 217806 [details]
proposed patch

typo fix to make GTK build happy

[Csaba Osztrogonác]

Created attachment 217938 [details]
Patch

One more shot, maybe the Mac WK2 bot is a little bit healthier now.

[Csaba Osztrogonác]

Created attachment 218046 [details]
proposed patch

Same patch, but forgot that webkit-patch upload doesn't use git -M.
Uploaded "git -M" style patch.

[EFL EWS Bot]

Comment on attachment 218046 [details]
proposed patch

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

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 218046 [details]
proposed patch

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

[Csaba Osztrogonác]

Created attachment 218048 [details]
proposed patch

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

WK2 owners, please review this trivial patch. Thanks.

[Andy Wingo]

Created attachment 218684 [details]
Patch

[Andy Wingo]

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.

[Csaba Osztrogonác]

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.)

[Darin Adler]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 218793 [details]
proposed patch

[Darin Adler]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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. ;) )

[Csaba Osztrogonác]

WK2 owners, could you review it, please?

[Darin Adler]

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.

[WebKit Commit Bot]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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