Bug 175441 - Rename the source files for the WTF persistent coders
Summary: Rename the source files for the WTF persistent coders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-10 13:54 PDT by Brady Eidson
Modified: 2017-08-10 20:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (50.32 KB, patch)
2017-08-10 17:05 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff
Patch (19.70 KB, patch)
2017-08-10 19:15 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-08-10 13:54:02 PDT
Make Persistence::Encoder and Persistence::Decoder appropriate for in-memory encoding

I'll be needed an in-memory only encoder/decoder in WebCore soon and these classes *almost* hit the mark.
The reason they don't quite hit the mark is the SHA1 functionality with is expensive and unnecessary for bits we're not going to store on disk.

I plan to split out a base class that skips the SHA1'ing but keeps all of the same great template infrastructure.
Comment 1 Brady Eidson 2017-08-10 16:58:21 PDT
Retitle:

Make the WTF Encoder/Decoders appropriate for in-memory encoding (Don't always do the SHA1 checksumming)
Comment 2 Brady Eidson 2017-08-10 17:05:44 PDT
Created attachment 317889 [details]
Patch
Comment 3 Build Bot 2017-08-10 17:08:20 PDT
Attachment 317889 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/coding/Encoder.h:87:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:88:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:89:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:90:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:91:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:92:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:93:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:94:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/coding/Encoder.h:96:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 10 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Sam Weinig 2017-08-10 17:13:05 PDT
I am not sure combining these is a good idea. The Persistence coders need to maintain the same binary format, whereas the in-memory coders are free to change. 

We've seen in the past cases where people were not aware that binary format was not stable for the WebKit2 coders, and were using it to write to the filesystem, and then when it changed, bad things happened.

So, despite there being the desire to share code here, I think this is a rare instance where that might not be the right call.
Comment 5 Tim Horton 2017-08-10 17:13:59 PDT
Comment on attachment 317889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317889&action=review

> Tools/TestWebKitAPI/Tests/WTF/Coding.cpp:62
> +// This should reliable capture a mistake where both encoders start doing the extra work of checksumming

reliably
Comment 6 Brady Eidson 2017-08-10 18:57:12 PDT
(In reply to Sam Weinig from comment #4)
> I am not sure combining these is a good idea. The Persistence coders need to
> maintain the same binary format, whereas the in-memory coders are free to
> change. 
> 
> We've seen in the past cases where people were not aware that binary format
> was not stable for the WebKit2 coders, and were using it to write to the
> filesystem, and then when it changed, bad things happened.
> 
> So, despite there being the desire to share code here, I think this is a
> rare instance where that might not be the right call.

This is a good point.

I agree making it a general purpose coder increases the odds that somebody will unknowingly change the format. (And I noticed, sadly, that there are not any API tests to guarantee backwards compatibility...)

We clearly need some WTF API tests for this reason.

Additionally I wonder if there's some way in the code we could make it even less likely that somebody would unknowingly change the format?
(But if we did have API test coverage, that would still be something that wouldn't last for long)

I'll fork it.
Comment 7 Brady Eidson 2017-08-10 19:11:32 PDT
Retitling:

Rename the source files for the WTF persistent coders
This will avoid conflicts from the upcoming non-persistent coders.
Comment 8 Brady Eidson 2017-08-10 19:15:18 PDT
Created attachment 317900 [details]
Patch
Comment 9 Build Bot 2017-08-10 19:17:45 PDT
Attachment 317900 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:94:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:96:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:97:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:98:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:99:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:100:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:102:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:103:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 10 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Commit Bot 2017-08-10 19:59:10 PDT
Comment on attachment 317900 [details]
Patch

Clearing flags on attachment: 317900

Committed r220574: <http://trac.webkit.org/changeset/220574>
Comment 11 WebKit Commit Bot 2017-08-10 19:59:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-08-10 20:00:28 PDT
<rdar://problem/33841949>