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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 378001: Patch

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




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

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:548
> +	   if (auto color = inlineStyle.propertyAsColor(propertyID)) {
> +	       if (color.value().isVisible() && !color.value().isSemantic()) {

Can we use an early return instead of nested if's?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:563
> +	   next = NodeTraversal::next(*node);

Since we're not modifying code there is no need to compute next early.
Just do: for (RefPtr<Node> node = fragment.firstChild(); node; node =
NodeTraversal::next(*node))

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

I don't think this comment is necessary. It's pretty self-evident from the
code.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:575
> +	   if (textLightness && *textLightness < textLightnessThreshold)

textLightnessThreshold doesn't quite tell us what kind threshold it is.
How about lightnessDarkEnoughForText?

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

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:580
> +	   if (backgroundLightness && *backgroundLightness >
backgroundLightnessThreshold)

How about lightnessLightEnoughForBackground?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:592
> +    for (RefPtr<Node> node = insertedNodes.firstNodeInserted(); node && node
!= pastEndNode; node = next) {
> +	   next = NodeTraversal::next(*node);

Ditto about traversal. Updating the attribute doesn't necessitate storing the
next node.
If sync DOM mutation were to occur (I don't think it could due to event scope),
then such a script would be mutated a node further down in the traversal
so it's not really meaningful to get just the next immediate node for traversal
regardless.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:389
> +    [webView setAppearance:[NSAppearance
appearanceNamed:NSAppearanceNameDarkAqua]];

Maybe we can add a helper function instead of repeating it twice?