Bug 227392 - [Model] [iOS] Add support for rendering model resources
Summary: [Model] [iOS] Add support for rendering model resources
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: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-25 03:04 PDT by Antoine Quint
Modified: 2021-06-27 01:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.91 KB, patch)
2021-06-25 03:12 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (21.70 KB, patch)
2021-06-26 08:05 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (21.66 KB, patch)
2021-06-27 00:59 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-06-25 03:04:28 PDT
[Model] [iOS] Add support for rendering model resources
Comment 1 Antoine Quint 2021-06-25 03:12:31 PDT
Created attachment 432252 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-06-25 03:23:08 PDT
<rdar://problem/79770136>
Comment 3 Antoine Quint 2021-06-25 03:53:08 PDT
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.
Comment 4 Tim Horton 2021-06-25 12:38:15 PDT
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
Comment 5 Tim Horton 2021-06-25 12:56:10 PDT
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
Comment 6 Antoine Quint 2021-06-26 08:03:06 PDT
(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.
Comment 7 Antoine Quint 2021-06-26 08:05:26 PDT
Created attachment 432329 [details]
Patch
Comment 8 Tim Horton 2021-06-26 15:24:31 PDT
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
Comment 9 Tim Horton 2021-06-26 15:26:01 PDT
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?
Comment 10 Antoine Quint 2021-06-27 00:39:45 PDT
(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.
Comment 11 Antoine Quint 2021-06-27 00:59:16 PDT
Created attachment 432348 [details]
Patch for landing
Comment 12 EWS 2021-06-27 01:56:01 PDT
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].