[Model] [iOS] Add support for rendering model resources
Created attachment 432252 [details] Patch
<rdar://problem/79770136>
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 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 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
(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.
Created attachment 432329 [details] Patch
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 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?
(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.
Created attachment 432348 [details] Patch for landing
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].