[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





--- Comment #4 from Alice Cheng <alice_cheng at apple.com>  2012-08-21 17:04:49 PST ---
(From update of attachment 159487)
View in context: https://bugs.webkit.org/attachment.cgi?id=159487&action=review

Thanks for your review =) I did not modify HTMLConverter directly, because I thought it might be too risky. Let me know if it would not be too risky to just not produce new line characters in HTMLConverter::editingAttributedStringFromRange. Or maybe do the merge over there?

>> 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.

fixed. Changed the name for the function. Added proper comments.

>> 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.

fixed. Left alone its textual part. Changed naming for two strings

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5728
>> +    NSUInteger firstNoneNewLineCharIndex = 0;
> 
> In this variable name it should be NonNewline rather than NoneNewLine.

fixed

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5733
>> +        return string;
> 
> We leak attributedString here.

fixed

>> Source/WebKit/mac/WebView/WebHTMLView.mm:5752
>> +    return attributedString;
> 
> This result will leak. We need to call autorelease on it.

fixed

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