Bug 233057 - Add ModelDocument for directly showing content that can be handled by <model>
Summary: Add ModelDocument for directly showing content that can be handled by <model>
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: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-12 11:49 PST by Dean Jackson
Modified: 2021-11-17 10:46 PST (History)
12 users (show)

See Also:


Attachments
Patch (29.55 KB, patch)
2021-11-12 17:23 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (32.16 KB, patch)
2021-11-13 13:52 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (29.80 KB, patch)
2021-11-14 12:37 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (31.06 KB, patch)
2021-11-16 16:41 PST, Dean Jackson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-11-12 11:49:40 PST
Add ModelDocument for directly showing content that can be handled by <model>
Comment 1 Dean Jackson 2021-11-12 11:50:10 PST
rdar://85265880
Comment 2 Dean Jackson 2021-11-12 17:23:01 PST
Created attachment 444128 [details]
Patch
Comment 3 Dean Jackson 2021-11-13 09:06:06 PST
Oh, I forgot <model> is only available in newer macOSes.
Comment 4 Sam Weinig 2021-11-13 09:55:50 PST
Comment on attachment 444128 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:1095
> +#define HAVE_MODEL_DOCUMENT 1

What's the rationale for having a separate flag for this? Seems like using ENABLE(MODEL_ELEMENT) would be fine.
Comment 5 Dean Jackson 2021-11-13 13:44:14 PST
Comment on attachment 444128 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:1095
>> +#define HAVE_MODEL_DOCUMENT 1
> 
> What's the rationale for having a separate flag for this? Seems like using ENABLE(MODEL_ELEMENT) would be fine.

No real reason other than I just wanted to keep it separate for now. We still need to improve <model> before this could completely replace the WKUSDPreviewView, which is one of the goals.
Comment 6 Dean Jackson 2021-11-13 13:52:06 PST
Created attachment 444148 [details]
Patch
Comment 7 Sam Weinig 2021-11-13 17:45:37 PST
(In reply to Dean Jackson from comment #5)
> Comment on attachment 444128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444128&action=review
> 
> >> Source/WTF/wtf/PlatformHave.h:1095
> >> +#define HAVE_MODEL_DOCUMENT 1
> > 
> > What's the rationale for having a separate flag for this? Seems like using ENABLE(MODEL_ELEMENT) would be fine.
> 
> No real reason other than I just wanted to keep it separate for now. We
> still need to improve <model> before this could completely replace the
> WKUSDPreviewView, which is one of the goals.

I think a Setting would be better for this. Seems more useful to have it compiled in and testable, then compiled out.
Comment 8 Sam Weinig 2021-11-14 10:25:38 PST
Comment on attachment 444148 [details]
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.
Comment 9 Dean Jackson 2021-11-14 12:37:39 PST
Created attachment 444188 [details]
Patch
Comment 10 Dean Jackson 2021-11-14 12:38:54 PST
Oops. Didn't read Sam's comments before uploading. Will send a new patch.
Comment 11 Dean Jackson 2021-11-15 11:37:07 PST
Comment on attachment 444148 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:1096
>> +#endif
> 
> I'd remove this and replace it with a Setting.

OK.

>> Source/WebCore/dom/Document.h:309
>> +using DocumentClassFlags = uint32_t;
> 
> Seems like uint16_t would be more appropriate for the size.

OK.

>> 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.

OK.

>> 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.

OK.

>> LayoutTests/platform/mac/TestExpectations:1716
>> +[ Catalina Mojave BigSur ] http/tests/model/model-document.html [ Skip ]
> 
> Why are these being skipped?

I'll check.
Comment 12 Dean Jackson 2021-11-16 16:41:52 PST
Created attachment 444452 [details]
Patch
Comment 13 Darin Adler 2021-11-16 17:18:07 PST
Comment on attachment 444452 [details]
Patch

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

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

This should be changed to be an OptionSet at some point; would be a pretty small change, believe it or not. That does not require using "enum class" in case someone tries to convince you it does, but also this would be a great choice for an enum class since all the values have DocumentClass in their names. Also, the DocumentClass enum should use uint16_t as its underlying type. Also does not require "enum class".

> Source/WebCore/html/ModelDocument.cpp:98
> +    auto styleContent = "body { background-color: white; text-align: center; }\n@media (prefers-color-scheme: dark) { body { background-color: rgb(32, 32, 37); } }\nmodel { width: 80vw; height: 80vh; }\n"_s;

This does not have to be one large line. Can use string pasting to format however you like:

    auto styleContent =
        "body { background-color: white; text-align: center; }\n"
        "@media (prefers-color-scheme: dark) { body { background-color: rgb(32, 32, 37); } }\n"
    ;

Maybe not exactly like that, but something like that.

> Source/WebCore/html/ModelDocument.cpp:119
> +    RefPtr<Frame> frame = document.frame();

Just RefPtr is fine, no need to write <Frame>.

> Source/WebCore/html/ModelDocument.h:61
> +#endif // ENABLE(VIDEO)

Oops, MODEL_ELEMENT.
Comment 14 Dean Jackson 2021-11-17 10:46:31 PST
Committed r285936 (244345@main): <https://commits.webkit.org/244345@main>