Bug 124720

Summary: Move CertificateInfo to WebCore
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, cgarcia, commit-queue, danw, darin, eflews.bot, gtk-ews, gustavo, gyuyoung.kim, ltilve+ews, mrobinson, ossy, rakuco, rego+ews, wingo, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 124150    
Bug Blocks: 118520, 124724    
Attachments:
Description Flags
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
Patch
none
proposed patch
eflews.bot: commit-queue-
proposed patch
none
Patch
none
proposed patch
none
proposed patch
none
patch for landing none

Description Csaba Osztrogonác 2013-11-21 08:24:45 PST
It is the 2nd step after renaming PlatformCertificateInfo to CertificateInfo (bug124150)
Comment 1 Csaba Osztrogonác 2013-11-21 10:01:58 PST
Created attachment 217578 [details]
proposed patch

no r? now, because we are waiting for bug124150
Comment 2 Csaba Osztrogonác 2013-11-21 10:08:04 PST
Created attachment 217579 [details]
proposed patch

better diff with git diff -M
Comment 3 Csaba Osztrogonác 2013-11-21 10:12:11 PST
Created attachment 217580 [details]
proposed patch

oops, I forgot to git add the soup files
Comment 4 Csaba Osztrogonác 2013-11-21 10:50:06 PST
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.
Comment 5 Csaba Osztrogonác 2013-11-25 06:08:11 PST
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.
Comment 6 WebKit Commit Bot 2013-11-25 06:09:19 PST
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 7 Csaba Osztrogonác 2013-11-25 06:20:39 PST
Comment on attachment 217801 [details]
proposed patch

fixing GTK build
Comment 8 Csaba Osztrogonác 2013-11-25 06:37:31 PST
Created attachment 217803 [details]
proposed patch

with speculative GTK buildfix
Comment 9 Csaba Osztrogonác 2013-11-25 06:47:52 PST
Created attachment 217804 [details]
proposed patch

one more speculative GTK fix
Comment 10 Csaba Osztrogonác 2013-11-25 07:24:03 PST
Created attachment 217806 [details]
proposed patch

typo fix to make GTK build happy
Comment 11 Csaba Osztrogonác 2013-11-27 03:44:31 PST
Created attachment 217938 [details]
Patch

One more shot, maybe the Mac WK2 bot is a little bit healthier now.
Comment 12 Csaba Osztrogonác 2013-11-29 02:29:34 PST
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 13 EFL EWS Bot 2013-11-29 02:32:53 PST
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 14 EFL EWS Bot 2013-11-29 02:33:49 PST
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 15 kov's GTK+ EWS bot 2013-11-29 02:36:15 PST
Comment on attachment 218046 [details]
proposed patch

Attachment 218046 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/39138126
Comment 16 Csaba Osztrogonác 2013-11-29 02:46:36 PST
Created attachment 218048 [details]
proposed patch
Comment 17 Csaba Osztrogonác 2013-12-02 08:49:03 PST
Anders, could you review this trivial patch, please? Or any other WK2 owner?
Comment 18 Csaba Osztrogonác 2013-12-02 23:51:54 PST
ping?
Comment 19 Csaba Osztrogonác 2013-12-04 09:47:09 PST
WK2 owners, please review this trivial patch. Thanks.
Comment 20 Andy Wingo 2013-12-08 05:56:58 PST
Created attachment 218684 [details]
Patch
Comment 21 Andy Wingo 2013-12-08 05:59:11 PST
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.
Comment 22 Csaba Osztrogonác 2013-12-09 05:37:00 PST
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 23 Darin Adler 2013-12-09 11:22:27 PST
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 24 Csaba Osztrogonác 2013-12-09 12:43:40 PST
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.
Comment 25 Csaba Osztrogonác 2013-12-09 13:05:34 PST
Created attachment 218793 [details]
proposed patch
Comment 26 Darin Adler 2013-12-09 13:56:50 PST
(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?
Comment 27 Csaba Osztrogonác 2013-12-10 07:12:32 PST
(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.
Comment 28 Csaba Osztrogonác 2013-12-10 07:21:08 PST
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. ;) )
Comment 29 Csaba Osztrogonác 2013-12-11 09:01:29 PST
WK2 owners, could you review it, please?
Comment 30 Darin Adler 2013-12-11 14:39:31 PST
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 31 WebKit Commit Bot 2013-12-11 14:45:32 PST
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
Comment 32 Csaba Osztrogonác 2013-12-12 03:20:44 PST
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
Comment 33 Csaba Osztrogonác 2013-12-12 04:28:12 PST
Landed in http://trac.webkit.org/changeset/160487
Comment 34 Csaba Osztrogonác 2013-12-17 05:00:57 PST
(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