Bug 227392 – [Model] [iOS] Add support for rendering model resources

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Quint]

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

[Antoine Quint]

Created attachment 432252 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79770136>

[Antoine Quint]

I wonder if we should add the ability to write to a file on the Model class, which will allow us to write the file for the macOS code without any new code. On macOS, we will write the file in the WebProcess.

[Tim Horton]

Comment on attachment 432252 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Ensure the achor point is set correctly for model content layers, otherwise the preview layer shows

"anchor"

> Source/WebKit/UIProcess/ios/WKModelView.h:3
> +/*
> + * Copyright (C) 2021 Apple Inc. All rights reserved.
> + */

This is not the right copyright header

> Source/WebKit/UIProcess/ios/WKModelView.mm:3
> +/*
> + * Copyright (C) 2021 Apple Inc. All rights reserved.
> + */

Ditto

> Source/WebKit/UIProcess/ios/WKModelView.mm:56
> +    _preview = adoptNS([allocASVInlinePreviewInstance() initWithFrame:self.bounds]);

Do you need to propagate bounds changes on this layer downwards to the ASVInlinePreview?

> Source/WebKit/UIProcess/ios/WKModelView.mm:61
> +    [_preview setupRemoteConnectionWithCompletionHandler:^(NSError * _Nullable contextError) {

Get all these nullables out of this implementation file (because of infectious nullability)

> Source/WebKit/UIProcess/ios/WKModelView.mm:100
> +    _file = FileSystem::openFile(filePath, FileSystem::FileOpenMode::Write);

Definitely need a fixme about this going away here

> Source/WebKit/UIProcess/ios/WKModelView.mm:118
> +    _bounds = bounds;

_bounds is maybe "_previewBounds" or "_lastBounds" or something? Since this is a UIView, it's a bit weird to have _bounds that isn't equal to self.bounds

[Tim Horton]

Comment on attachment 432252 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKModelView.mm:69
> +                LOG(ModelElement, "Unable to load file, error: %s", [loadError localizedDescription]);

As smfr mentioned, this is not the right format string for an NSString

[Antoine Quint]

(In reply to Tim Horton from comment #4)
> Comment on attachment 432252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432252&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Ensure the achor point is set correctly for model content layers, otherwise the preview layer shows
> 
> "anchor"

Will fix in new patch.

> > Source/WebKit/UIProcess/ios/WKModelView.h:3
> > +/*
> > + * Copyright (C) 2021 Apple Inc. All rights reserved.
> > + */
> 
> This is not the right copyright header

Will fix in new patch.

> > Source/WebKit/UIProcess/ios/WKModelView.mm:3
> > +/*
> > + * Copyright (C) 2021 Apple Inc. All rights reserved.
> > + */
> 
> Ditto
> 
> > Source/WebKit/UIProcess/ios/WKModelView.mm:56
> > +    _preview = adoptNS([allocASVInlinePreviewInstance() initWithFrame:self.bounds]);
> 
> Do you need to propagate bounds changes on this layer downwards to the
> ASVInlinePreview?

Oops, forgot the layoutSubviews override from my main branch! Will add a new patch that you can review again just to check it's good.

> > Source/WebKit/UIProcess/ios/WKModelView.mm:61
> > +    [_preview setupRemoteConnectionWithCompletionHandler:^(NSError * _Nullable contextError) {
> 
> Get all these nullables out of this implementation file (because of
> infectious nullability)

Will fix in new patch.

> > Source/WebKit/UIProcess/ios/WKModelView.mm:100
> > +    _file = FileSystem::openFile(filePath, FileSystem::FileOpenMode::Write);
> 
> Definitely need a fixme about this going away here
> 
> > Source/WebKit/UIProcess/ios/WKModelView.mm:118
> > +    _bounds = bounds;
> 
> _bounds is maybe "_previewBounds" or "_lastBounds" or something? Since this
> is a UIView, it's a bit weird to have _bounds that isn't equal to self.bounds

Changing to _lastBounds in new patch.

[Antoine Quint]

Created attachment 432329 [details]
Patch

[Tim Horton]

Comment on attachment 432329 [details]
Patch

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

> Source/WTF/ChangeLog:11
> +        12 SDKs, but ultimately we will only use the iOS version check.

12?

> Source/WebKit/UIProcess/ios/WKModelView.mm:81
> +    [_preview setupRemoteConnectionWithCompletionHandler:^(NSError * _Nullable contextError) {

Your infectious nullables are still here

[Tim Horton]

Comment on attachment 432329 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKModelView.mm:124
> +    auto byteCount = static_cast<std::size_t>(FileSystem::writeToFile(file, model.data()->data(), model.data()->size()));

Nothing is cleaning up the file? Maybe because this is intended to go away?

[Antoine Quint]

(In reply to Tim Horton from comment #9)
> Comment on attachment 432329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432329&action=review
> 
> > Source/WebKit/UIProcess/ios/WKModelView.mm:124
> > +    auto byteCount = static_cast<std::size_t>(FileSystem::writeToFile(file, model.data()->data(), model.data()->size()));
> 
> Nothing is cleaning up the file? Maybe because this is intended to go away?

Yes, it's written to a temporary directory, and we don't intend to support this approach in the long run. Writing to the file system is only viable while we experiment with rendering 3D models using the <model> element.

[Antoine Quint]

Created attachment 432348 [details]
Patch for landing

[EWS]

Committed r279312 (239188@main): <https://commits.webkit.org/239188@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432348 [details].