[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