[webkit-reviews] review granted: [Bug 195636] Add new NSAttributedString API for converting HTML : [Attachment 364476] Patch 2 / 4
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 12 19:08:50 PDT 2019
Ryosuke Niwa <rniwa at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 195636: Add new NSAttributedString API for converting HTML
https://bugs.webkit.org/show_bug.cgi?id=195636
Attachment 364476: Patch 2 / 4
https://bugs.webkit.org/attachment.cgi?id=364476&action=review
--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 364476
--> https://bugs.webkit.org/attachment.cgi?id=364476
Patch 2 / 4
View in context: https://bugs.webkit.org/attachment.cgi?id=364476&action=review
r=me assuming the issue that the encoder sometimes does not encode the right
number of items is fixed.
> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:89
> + // Secure coding check is after specific cases.
This just repeats what the code says. We should explain why we need to do this
instead.
> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:105
> + auto fontClass = [NSFont class];
Why don't we define PlatformFont somewhere above this line as we do in
HTMLConverter
so that we don't have to have if-defs everywhere.
> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:126
> + ASSERT(isSerializableValue(value));
> + if (!isSerializableValue(value))
> + continue;
This would make the length not match the number of the encoded items.
That would result in an encoding mismatch down the line and would result in a
mysterious crash, etc...
We should probably add a boolean or something to indicate when this happens.
> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:213
> + if (!isSerializableValue(key) || !isSerializableValue(value))
> + continue;
Ditto.
> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:504
> +namespace WTF {
> +template<> struct EnumTraits<IPC::NSType> {
Why not define this at the beginning of the file instead?
More information about the webkit-reviews
mailing list