[webkit-reviews] review denied: [Bug 201368] Mail appears to be double inverting code copied from Notes, Xcode, or Terminal : [Attachment 377773] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 201368: Mail appears to be double inverting code copied from Notes, Xcode,
or Terminal
https://bugs.webkit.org/show_bug.cgi?id=201368

Attachment 377773: Patch

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




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 377773
  --> https://bugs.webkit.org/attachment.cgi?id=377773
Patch

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

Are we trying to detect the dark mode content? That doesn't sound right.
The attributed string itself should contain that information.

r- because I don't think this patch should be landed as is.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:530
> +    ContainerNode* context =
insertedNodes.firstNodeInserted()->parentNode();

This is not safe. updateLayoutIgnorePendingStylesheets can delete this node.
Need to use RefPtr.
Also, why is it okay to not do this work when we're inserting into a list item?

This work probably needs to be done right before we start inserting contents
where hasBlankLinesBetweenParagraphs, etc... is computed.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:537
> +    auto* contextRenderer = context->renderer();
> +    if (!contextRenderer || !contextRenderer->style().hasAppleColorFilter())

Why don't we just check the style of the root editable element?
Realistically speaking, applying color filter to some part of the editable
region isn't gonna work anyway.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:541
> +    // Only the apple-invert-lightness() color filter is supported. It is
the only
> +    // FilterOperation that implements inverseTransformColor().

This is obvious from the code. I don't think we need this comment.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:564
> +    // Check the top-level inserted node's text and background colors. If
any node has
> +    // dark text or light background colors, abort before the transformation
step.

I don't think we need this comment. It's self-evident from the code.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:565
> +    RefPtr<Node> pastEndNode = insertedNodes.pastLastSibling();

Please use pastLastLeaf() as we've done elsewhere.
Introducing a new traversal code like this is very likely going to result in a
hang or a crash.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
> +	   auto* element = downcast<StyledElement>(node.get());
> +	   auto* inlineStyle = element->inlineStyle();

Please don't use raw pointers.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:578
> +	   // Abort if the text color is too dark.

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:583
> +	   // Abort if the background color is too light.

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:597
> +	   StyledElement* element = downcast<StyledElement>(node.get());

Please don't use raw pointers.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:599
> +	   auto* inlineStyle = element->inlineStyle();

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.h:85
> +	   Node* pastLastSibling() const

Why do we need to add this?

> LayoutTests/editing/pasteboard/paste-dark-mode-color-filtered.html:41
> +	   document.execCommand("SelectAll", false);
> +	   document.execCommand("Copy", false);
> +
> +	   copyNode.remove();

This will not test cross origin / cross app sanitization code path.
We should probably add an equivalent API test.