[webkit-reviews] review denied: [Bug 233057] Add ModelDocument for directly showing content that can be handled by <model> : [Attachment 444148] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Dean Jackson <dino at apple.com>'s request
for review:
Bug 233057: Add ModelDocument for directly showing content that can be handled
by <model>
https://bugs.webkit.org/show_bug.cgi?id=233057

Attachment 444148: Patch

https://bugs.webkit.org/attachment.cgi?id=444148&action=review




--- Comment #8 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 444148
  --> https://bugs.webkit.org/attachment.cgi?id=444148
Patch

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

> Source/WTF/wtf/PlatformHave.h:1096
> +#if PLATFORM(COCOA)
> +#define HAVE_MODEL_DOCUMENT 1
> +#endif

I'd remove this and replace it with a Setting.

> Source/WebCore/dom/Document.h:309
> -using DocumentClassFlags = unsigned char;
> +using DocumentClassFlags = uint32_t;

Seems like uint16_t would be more appropriate for the size.

> Source/WebCore/html/ModelDocument.cpp:98
> +    auto styleContent = makeString("body { background-color: white;
text-align: center; }\n",

maybe I am missing it, but I think this has any values being concatenated, so
this shouldn't use makeString(). Instead, it can just use a " .. "_s.

> LayoutTests/platform/mac-wk1/TestExpectations:50
> +http/tests/model/model-document.html [ Skip ]

I would skip all of 50http/tests/model. The feature isn't supported or enabled
there so no reason to test it.

> LayoutTests/platform/mac/TestExpectations:1716
> +[ Catalina Mojave BigSur ] http/tests/model/model-document.html [ Skip ]

Why are these being skipped?

> LayoutTests/platform/win/TestExpectations:136
> +# Apple Cocoa only for the moment.
> +http/tests/model/model-document.html [ Skip ]

Again, I would just skip the whole directory.