Bug 227530 – [Model] [macOS] Add support for rendering model resources

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Model] [macOS] Add support for rendering model resources

[Antoine Quint]

Created attachment 432598 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79968206>

[Antoine Quint]

Created attachment 432599 [details]
Patch

[Sam Weinig]

Comment on attachment 432599 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.mm:29
> +#if ENABLE(MODEL_ELEMENT)

We shouldn't use .mm for cross platform code like this. Please keep this a .cpp file and factor out the objective-c bits into their own classes.

[Antoine Quint]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 432599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432599&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElement.mm:29
> > +#if ENABLE(MODEL_ELEMENT)
> 
> We shouldn't use .mm for cross platform code like this. Please keep this a
> .cpp file and factor out the objective-c bits into their own classes.

Right, so add a Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm with these bits?

[Antoine Quint]

Created attachment 432623 [details]
Patch

[Dean Jackson]

Comment on attachment 432623 [details]
Patch

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

No test?

> Source/WebCore/ChangeLog:12
> +        <model> element is created and message the UIProcess through the ChromeClient provinding the UUID generated for it. When

Typo provinding

> Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:43
> +#include <wtf/UUID.h>
> +SOFT_LINK_PRIVATE_FRAMEWORK(AssetViewer);

Add a space between these lines.

> Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:66
> +    auto fileName = FileSystem::encodeForFileName(createCanonicalUUIDString()) + ".usdz";

What about if it is a .reality file?

> Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:74
> +    auto file = FileSystem::openFile(filePath, FileSystem::FileOpenMode::Write);
> +    if (file <= 0)
> +        return;
> +
> +    FileSystem::writeToFile(file, m_data->data(), m_data->size());
> +    FileSystem::closeFile(file);
> +    m_filePath = filePath;

Can you put a note in here that this is a temporary solution and we don't want to write to files?

> Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:92
> +    FloatSize size;
> +    if (auto* renderer = this->renderer())
> +        size = renderer->absoluteBoundingBoxRect(false).size();

If there isn't a renderer you probably just want to exit.

if (!this->renderer())
  return;

auto size = this->renderer()->aBB...

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:134
> +    else
> +        iterator->value = preview;

I didn't know you could do this! Very nice.

[Antoine Quint]

(In reply to Dean Jackson from comment #7)
> Comment on attachment 432623 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432623&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:66
> > +    auto fileName = FileSystem::encodeForFileName(createCanonicalUUIDString()) + ".usdz";
> 
> What about if it is a .reality file?

Filed bug 227568, will add a reference to it in the commit.

> > Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm:74
> > +    auto file = FileSystem::openFile(filePath, FileSystem::FileOpenMode::Write);
> > +    if (file <= 0)
> > +        return;
> > +
> > +    FileSystem::writeToFile(file, m_data->data(), m_data->size());
> > +    FileSystem::closeFile(file);
> > +    m_filePath = filePath;
> 
> Can you put a note in here that this is a temporary solution and we don't
> want to write to files?

Filed bug 227567 and adding a reference to it in the commit.

[Antoine Quint]

Committed r279451 (239307@main): <https://commits.webkit.org/239307@main>

[Sam Weinig]

(In reply to Antoine Quint from comment #5)
> (In reply to Sam Weinig from comment #4)
> > Comment on attachment 432599 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=432599&action=review
> > 
> > > Source/WebCore/Modules/model-element/HTMLModelElement.mm:29
> > > +#if ENABLE(MODEL_ELEMENT)
> > 
> > We shouldn't use .mm for cross platform code like this. Please keep this a
> > .cpp file and factor out the objective-c bits into their own classes.
> 
> Right, so add a
> Source/WebCore/Modules/model-element/HTMLModelElementCocoa.mm with these
> bits?

I usually try to add some c++ wrapper class in its own files instead to avoid having multiple implementation files for the same header which is not something that is very understandable.