[webkit-reviews] review denied: [Bug 21033] queryCommandValue('FontSize') returns pixel values instead of IE font numbers : [Attachment 66960] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 21033: queryCommandValue('FontSize') returns pixel values instead of IE
font numbers
https://bugs.webkit.org/show_bug.cgi?id=21033

Attachment 66960: Patch
https://bugs.webkit.org/attachment.cgi?id=66960&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    Node* node = m_node.get();
> +    if (!node)
> +	   return false;

This can just be:

    if (!m_node)

No need for get() or a local variable.

> +    RefPtr<RenderStyle> style =
m_node->computedStyle(m_pseudoElementSpecifier);
> +    if (!style)
> +	   return false;
> +
> +    return style->fontDescription().useFixedDefaultSize();

I don't think it makes sense to name this "isFontFixedSize" because it does not
answer that question. Instead it should use a name consistent with the name in
FontDescription such as "shouldUseFixedDefaultSize".

> +template<typename T>
> +static int findNearestLegacyFontSize(int size, const T* table, int
multiplier)
> +{
> +    // Ignore table[0] because xx-small does not correspond to any legacy
font size.
> +    for (int i = 1; i < totalKeywords - 1; i++) {
> +	   if (size * 2 < (table[i] + table[i + 1]) * multiplier)
> +	       return i;
> +    }
> +    return totalKeywords - 1;
> +}

Is this algorithm the copy of something done elsewhere? Why don't I see "-"
lines in the place where the old code was? Are we making a new copy of the code
without making the other copies share the code?

> +	   // Given a font size in pixel, this return will return legacy font
size between 1-7.

The phrase "between 1-7" is not good wording. You should say "between 1 and 7"
or just "1-7".

> +	   static int legacyFontSize(Document* document, int size, bool fixed);


The argument name "document" should be removed.

The argument name "size" is unclear. What kind of size? The function returns a
size, so the argument has to be distinguished. Maybe pixelSize would be the
right name.

The argument name "fixed" is not right. It should be
"shouldUseFixedDefaultSize" I think.

> +    // If the pos is at the end of a text node, then this node is not fully
selected.
> +    // Move it to the next deep quivalent position to avoid removing the
style from this node.

Typo: "quivalent".

> +    // e.g. if pos was at Position("hello", 5) in
<b>hello<div>world</div></b>, we want Position("world", 0) instead.
> +    // We only do this for range because caret at Position("hello", 5) in
<b>hello</b>world should give you font-weight: bold.
> +    Node* posNode = pos.containerNode();
> +    if (selection()->isRange() && posNode && posNode->isTextNode() &&
pos.computeOffsetInContainerNode() == posNode->maxCharacterOffset())
> +	   pos = nextVisuallyDistinctCandidate(pos);

Does the new test cover this case?

I worry that this special case is too specific.

I’ll say review- because I’m sure you’ll want to make at least one of the
changes mentioned above.