[webkit-reviews] review denied: [Bug 94510] Inspector Bar does not reflect selected font styles correctly, when multiple <div>s are selected. : [Attachment 159487] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alice Cheng <alice_cheng at apple.com>'s
request for review:
Bug 94510: Inspector Bar does not reflect selected font styles correctly, when
multiple <div>s are selected.
https://bugs.webkit.org/show_bug.cgi?id=94510

Attachment 159487: proposed patch
https://bugs.webkit.org/attachment.cgi?id=159487&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=159487&action=review


Why are we patching up the attributed string after the fact rather than making
the style correct when we originally construct the string? It seems strange to
“patch around” this bug instead of fixing it.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5724
> +static NSAttributedString * mergeNewLineCharAttributes(NSAttributedString
*string)

There should be no space after the * before the word merge.

The name of thus function is not good. This function doesn’t “merge” anything.
I believe what it does is to overwrite newline character attributes with the
attributes of the previous character. Also, a function that returns a value
should not be given a name that is a “verb”. It should be a noun. Maybe
stringWithNewlineAttributesFixed would be a good name, even though “fixed” is
not a clear concept.

This function needs a comment explaining why it does what it does. It’s good
that the change log explains, but the code needs to explain too, because it’s
non-obvious why this would be a good thing to do.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5726
> +    NSMutableAttributedString *attributedString =
[[NSMutableAttributedString alloc] init];

This algorithm looks quite inefficient. We're copying all sorts of substrings.
We’re not even changing the string, so building an entirely new attributed
string is not right. If we do need to do this, rather than making the style
correct when we originally construct the string, we should make a mutable copy
of the attributed string and then modify only the attributes, leaving the
textual part of the string alone.

Use of the name attributedString for the new one and the name string for the
original one is unnecessarily confusing. Those are two different type names,
and yet both are attributed strings. The names need to make clear the
difference between these two rather than being arbitrarily different in a way
that does not make clear what’s different.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5728
> +    NSUInteger firstNoneNewLineCharIndex = 0;

In this variable name it should be NonNewline rather than NoneNewLine.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5733
> +	   return string;

We leak attributedString here.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5752
> +    return attributedString;

This result will leak. We need to call autorelease on it.