WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
233057
Add ModelDocument for directly showing content that can be handled by <model>
https://bugs.webkit.org/show_bug.cgi?id=233057
Summary
Add ModelDocument for directly showing content that can be handled by <model>
Dean Jackson
Reported
2021-11-12 11:49:40 PST
Add ModelDocument for directly showing content that can be handled by <model>
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Dean Jackson
Comment 1
2021-11-12 11:50:10 PST
rdar://85265880
Dean Jackson
Comment 2
2021-11-12 17:23:01 PST
Created
attachment 444128
[details]
Patch
Dean Jackson
Comment 3
2021-11-13 09:06:06 PST
Oh, I forgot <model> is only available in newer macOSes.
Sam Weinig
Comment 4
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.
Dean Jackson
Comment 5
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.
Dean Jackson
Comment 6
2021-11-13 13:52:06 PST
Created
attachment 444148
[details]
Patch
Sam Weinig
Comment 7
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.
Sam Weinig
Comment 8
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.
Dean Jackson
Comment 9
2021-11-14 12:37:39 PST
Created
attachment 444188
[details]
Patch
Dean Jackson
Comment 10
2021-11-14 12:38:54 PST
Oops. Didn't read Sam's comments before uploading. Will send a new patch.
Dean Jackson
Comment 11
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.
Dean Jackson
Comment 12
2021-11-16 16:41:52 PST
Created
attachment 444452
[details]
Patch
Darin Adler
Comment 13
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.
Dean Jackson
Comment 14
2021-11-17 10:46:31 PST
Committed
r285936
(
244345@main
): <
https://commits.webkit.org/244345@main
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug