[webkit-reviews] review granted: [Bug 227530] [Model] [macOS] Add support for rendering model resources : [Attachment 432623] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 227530: [Model] [macOS] Add support for rendering model resources
https://bugs.webkit.org/show_bug.cgi?id=227530

Attachment 432623: Patch

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




--- Comment #7 from Dean Jackson <dino at apple.com> ---
Comment on attachment 432623
  --> https://bugs.webkit.org/attachment.cgi?id=432623
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.