Bug 189216 - Add functionality to encode and decode a uint64_t in KeyedCoding
Summary: Add functionality to encode and decode a uint64_t in KeyedCoding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Woodrow Wang
URL:
Keywords: InRadar
Depends on:
Blocks: 187773
  Show dependency treegraph
 
Reported: 2018-08-31 16:59 PDT by Woodrow Wang
Modified: 2018-09-05 10:05 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.49 KB, patch)
2018-08-31 17:39 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2018-08-31 17:43 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (8.89 KB, patch)
2018-09-04 09:39 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (8.89 KB, patch)
2018-09-04 15:47 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Woodrow Wang 2018-08-31 16:59:44 PDT
Added functionality to encode and decode a uint64_t in KeyedDecoding
Comment 1 Woodrow Wang 2018-08-31 17:39:11 PDT
Created attachment 348689 [details]
Patch
Comment 2 Woodrow Wang 2018-08-31 17:43:01 PDT
Created attachment 348691 [details]
Patch
Comment 3 Daniel Bates 2018-08-31 18:57:04 PDT
Comment on attachment 348691 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Added functionality to encode and decode a uint64_t in KeyedCoding. 

This line should be a verbatim copy of the bug title. Please remove the period.

> Source/WebCore/ChangeLog:7
>  

Please explain why you are adding this functionality given that this patch just adds infrastructure and does not make use of it.

> Source/WebCore/ChangeLog:22
> +2018-08-31  Woodrow Wang  <woodrow_wang@apple.com>

Please fix up this file such that it only contains a new change log entry at the top.
Comment 4 Michael Catanzaro 2018-09-01 06:33:31 PDT
Comment on attachment 348691 [details]
Patch

The changes to KeyedEncoderGlib look fine to me.

I agree that, if this is going to land in its own patch, there should be an explanation of how this functionality will be imminently used in a follow-up patch.
Comment 5 Woodrow Wang 2018-09-01 10:25:19 PDT
I've added this functionality in order to be able to encode and decode the raw uint64_t representation of an OptionSet for my patch here https://bugs.webkit.org/show_bug.cgi?id=187773 

The changes in the KeyedEncoder/KeyedDecoder for Glib were made because they are derived classes of KeyedCoding which contains pure virtual functions that need to be implemented.
Comment 6 Daniel Bates 2018-09-01 11:38:57 PDT
(In reply to Woodrow Wang from comment #5)
> I've added this functionality in order to be able to encode and decode the
> raw uint64_t representation of an OptionSet for my patch here
> https://bugs.webkit.org/show_bug.cgi?id=187773 
> 
> The changes in the KeyedEncoder/KeyedDecoder for Glib were made because they
> are derived classes of KeyedCoding which contains pure virtual functions
> that need to be implemented.

Great! Please explain this in the description portion of the ChangeLog entry: under the Reviewed by line.
Comment 7 Daniel Bates 2018-09-01 11:42:39 PDT
More specifically, please upload a new patch that addresses all feedback, substitutes Daniel Bates for “NOBODY (OOPS!)” and mark the patch commit-queue ?.
Comment 8 Woodrow Wang 2018-09-04 09:39:01 PDT
Created attachment 348819 [details]
Patch
Comment 9 Daniel Bates 2018-09-04 13:30:59 PDT
Comment on attachment 348819 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        representation of an OptionSet for my patch here https://bugs.webkit.org/show_bug.cgi?id=187773 

I would put the URL in angle brackets and add a period at the end of this sentence.
Comment 10 Woodrow Wang 2018-09-04 15:47:19 PDT
Created attachment 348860 [details]
Patch
Comment 11 WebKit Commit Bot 2018-09-05 10:03:09 PDT
Comment on attachment 348860 [details]
Patch

Clearing flags on attachment: 348860

Committed r235673: <https://trac.webkit.org/changeset/235673>
Comment 12 WebKit Commit Bot 2018-09-05 10:03:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-09-05 10:05:01 PDT
<rdar://problem/44142988>