[Webkit-unassigned] [Bug 94510] Inspector Bar does not reflect selected font styles correctly, when multiple <div>s are selected.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94510


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159487|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2012-08-20 18:10:18 PST ---
(From update of attachment 159487)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.