[webkit-reviews] review granted: [Bug 191352] Make it possible to edit images inline : [Attachment 354246] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 191352: Make it possible to edit images inline
https://bugs.webkit.org/show_bug.cgi?id=191352

Attachment 354246: Patch

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




--- Comment #32 from Dean Jackson <dino at apple.com> ---
Comment on attachment 354246
  --> https://bugs.webkit.org/attachment.cgi?id=354246
Patch

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

I feel like the EmbeddedView part should be a separate patch, and I'm not
really qualified to review, but whatever 🤗

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:745
> +    return ++nextEmbeddedViewID;

Shouldn't "next" mean nextEmbeddedViewID++? Or is 0 special?

(/me reads backwards and discovers that 0 is actually special in that it is
treated as no assigned ID)

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1170
> +	   contentsLayerChanged = true;

Why not just put noteSublayersChanged() here and avoid contentsLayerChanged
entirely?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:1031
> +    return { };

Why { } vs 0?

> Source/WebCore/rendering/RenderImage.cpp:227
> +    auto editableValue =
element()->attributeWithoutSynchronization(x_apple_editable_imageAttr);
> +    if (editableValue.isNull())
> +	   return false;
> +
> +    return editableValue.isEmpty() ||
equalLettersIgnoringASCIICase(editableValue, "true");

I think this should follow the controls attribute behaviour on
HTMLMediaElement. e.g.

return
element()->hasAttributeWithoutSynchronization(x_apple_editable_imageAttr);

and don't even look at the value.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm:44
> +    , embeddedViewID(0)

You use 0 here :)

Also, why isn't this in the .h file?

> Source/WebKit/Shared/WebPreferences.yaml:1187
> +EditableImagesEnabled:
> +  type: bool
> +  defaultValue: false

No webcoreBinding needed?

> LayoutTests/editing/images/basic-editable-image.html:13
> +    const numberOfStrokesInEditableImageAfterDrawing = (await
UIHelper.numberOfStrokesInEditableImage());

Please use WebML to detect if it was a square.

Nit: No need for the () around awaits.

> LayoutTests/editing/images/basic-editable-image.html:15
> +    testRunner.notifyDone();

No need to do this - I believe we wait until all promises are resolved by
default.