[webkit-reviews] review granted: [Bug 233265] [Model] add support for getting and setting the camera : [Attachment 444544] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 233265: [Model] add support for getting and setting the camera
https://bugs.webkit.org/show_bug.cgi?id=233265

Attachment 444544: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 444544
  --> https://bugs.webkit.org/attachment.cgi?id=444544
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:304
> +    m_modelPlayer->getCamera([protectedThis = Ref { *this }, promise =
WTFMove(promise)] (std::optional<HTMLModelElementCamera> camera) mutable {

I don’t fully understand why protectedThis is helpful. Why is extending the
lifetime of the HTMLModelElement valuable, given we do nothing with that
element pointer?

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:319
> +    m_modelPlayer->setCamera(camera, [protectedThis = Ref { *this }, promise
= WTFMove(promise)] (bool success) mutable {

Ditto.

> Source/WebCore/Modules/model-element/HTMLModelElementCamera.h:67
> +    return { {
> +	   WTFMove(*pitch),
> +	   WTFMove(*yaw),
> +	   WTFMove(*scale),
> +    } };

Moving a double isn’t valuable.

    return { { *pitch, *yaw, *scale } };

> Source/WebCore/Modules/model-element/HTMLModelElementCamera.idl:33
> +    double yaw;
> +    double pitch;
> +    double scale;

This says "yaw, pitch, scale", but the C++ says "pitch, yaw, scale". Does the
order matter? Could we be consistent?

> Source/WebKit/Shared/ModelIdentifier.h:69
> +    return { { WTFMove(*layerIdentifier) } };

No need to move a layer ID:

    return { { *layerIdentifier } };

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:58
> +WKModelView*
ModelElementController::modelViewForModelIdentifier(ModelIdentifier
modelIdentifier)

WKModelView * with a space before the *?

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:61
> +	   return nullptr;

nil is preferred over nullptr for Objective-C object pointers, I think

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:64
> +	   return nullptr;

Ditto.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:68
> +	   return nullptr;

Ditto.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:77
>      auto *view = node->uiView();
>      if (!view)
> -	   return;
> +	   return nullptr;
>  
> -    if (![view isKindOfClass:[WKModelView class]])
> -	   return;
> +    if ([view isKindOfClass:[WKModelView class]])
> +	   return (WKModelView *)view;
> +
> +    return nullptr;

WebKit has a function template for this that collapses these 8 lines into one:

    return dynamic_objc_cast<WKModelView>(node->uiView());

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:84
> +    if (auto *modelView = modelViewForModelIdentifier(modelIdentifier))
> +	   return [modelView preview];
> +    return nullptr;

Objective-C has the nil handling that simplifies this. Can write one of these
instead:

    return modelViewForModelIdentifier(modelIdentifier).preview;

    return [modelViewForModelIdentifier(modelIdentifier) preview];

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:93
> +    auto *modelView = modelViewForModelIdentifier(modelIdentifier);

I suggest auto here. I don’t think "auto *" is helpful. Or you could use
"auto*" or "WKModelView *".

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:152
> +    if (auto preview = m_inlinePreviews.get(modelIdentifier.uuid))
> +	   return preview.get();
> +
> +    return nullptr;

No need for an if here:

    return m_inlinePreviews.get(modelIdentifier.uuid).get();

But this makes me realize we have an unsafe-looking thing here. Normally it
would not be safe to call get() on a smart pointer and then just return the raw
pointer. The ".get()" here looks unsafe.

What makes it safe is the whole "peek" situation where the map itself holds a
RetainPtr, but it seems we didn’t implement PeekType for RetainPtr the way we
did for RefPtr. If we had, then HashMap::get would return a raw pointer and we
would not need "get()".