[webkit-reviews] review granted: [Bug 233105] [Model] [macOS] Add support for interaction on macOS : [Attachment 444520] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 233105: [Model] [macOS] Add support for interaction on macOS
https://bugs.webkit.org/show_bug.cgi?id=233105

Attachment 444520: Patch

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




--- Comment #12 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 444520
  --> https://bugs.webkit.org/attachment.cgi?id=444520
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:325
> +    auto frame = document().frame();

IIRC best practice™ is to wrap local variables in Ref or RefPtr unless it's
only passed into trivial functions
(https://www.mail-archive.com/webkit-dev@lists.webkit.org/msg29670.html). Not
precisely sure what constitutes "trivial" though :/

I think in this case it's technically safe because nothing below has the
potential to trigger layout, but it's probably still better to make this
`RefPtr frame`.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:334
> +	   m_modelPlayer->handleMouseDown(event.pageLocation(),
event.timeStamp());

I would double check that `pageLocation()` does the right thing in subframes.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:351
> +    auto frame = document().frame();

Ditto (`RefPtr frame`).

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:199
> +    auto iterator = m_inlinePreviews.find(uuid);
> +    if (iterator != m_inlinePreviews.end())
> +	   return iterator->value;
> +    return nullptr;

Nit - I think the body of this function can simply be `return
m_inlinePreviews.get(uuid);`

> Source/WebKit/UIProcess/WebPageProxy.h:598
> +    void handleMouseDownForModelElement(const String&, const
WebCore::LayoutPoint&, const MonotonicTime);
> +    void handleMouseMoveForModelElement(const String&, const
WebCore::LayoutPoint&, const MonotonicTime);
> +    void handleMouseUpForModelElement(const String&, const
WebCore::LayoutPoint&, const MonotonicTime);

Nit - I don't think we usually go out of our way to mark pass-by-value
arguments as const? Though it's fine as well, if you prefer to keep it.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:184
> +	  
page->send(Messages::WebPageProxy::HandleMouseDownForModelElement((String)[m_in
linePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Nit - I think you can omit the (String) here.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:190
> +	  
page->send(Messages::WebPageProxy::HandleMouseMoveForModelElement((String)[m_in
linePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Ditto.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:196
> +	  
page->send(Messages::WebPageProxy::HandleMouseUpForModelElement((String)[m_inli
nePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Ditto.