[webkit-reviews] review granted: [Bug 227392] [Model] [iOS] Add support for rendering model resources : [Attachment 432252] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 227392: [Model] [iOS] Add support for rendering model resources
https://bugs.webkit.org/show_bug.cgi?id=227392

Attachment 432252: Patch

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




--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 432252
  --> https://bugs.webkit.org/attachment.cgi?id=432252
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