[webkit-reviews] review granted: [Bug 21305] queryCommandValue "formatBlock" always returns false : [Attachment 67864] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 21305: queryCommandValue "formatBlock" always returns false
https://bugs.webkit.org/show_bug.cgi?id=21305

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

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

I’m reluctantly saying review+ here. I am not that happy with this extremely
specific function being added to the Editor class with an unclear name. I’m not
sure the function belongs in the Editor class at all.

> WebCore/editing/Editor.cpp:986
> +static bool isFormatBlock(const Node* node)

The term “block” is normally a rendering term, so an “is block” function should
be answering a question about rendering, not DOM.

This function answers a much more specific question, and it would be better to
give it a specific name.

Generally speaking, putting this specific “FormatBlock” concept into Editor is
OK, but we need to give it a name that makes it clear this is “for the
FormatBlock” command, and not some more fundamental concept that is useful
elsewhere.

> WebCore/editing/Editor.cpp:1002
> +    if (!m_frame->selection())
> +	   return String("");

This can never be 0, so this check is not needed.

> WebCore/editing/Editor.cpp:1006
> +	   return String("");

I would prefer either

    return "";

Or:

    return emptyAtom;

Explicitly saying String("") doesn’t seem to add anything. I think that
returning emptyAtom might be slightly more efficient, which is why I mention
that option.

> WebCore/editing/Editor.cpp:1008
> +    ExceptionCode ec = 0;

There’s no need to set up and check the exception code here. The
commonAncestorContainer function is guaranteed to return 0 any time an
exception would be raised. The exception code is only there for the bindings’
benefit. We could even create an overloaded version without the exception code
for use inside the engine. It’s really only JavaScript and other such bindings
that need the exception code.

> WebCore/editing/Editor.cpp:1016
> +    return static_cast<Element*>(commonAncestor)->localName();

This cast is only safe if we have a guarantee that the common ancestor is not a
document. It would be best to have a comment explaining why that is so, or we
could add an “is element” check above and be a little safer. At the very least
I’d like to see an assertion.

> WebCore/editing/Editor.h:128
> +    String selectionFormatBlock();

I think this function name is very confusing. The FormatBlock thing is a
strange, quirky, execCommand name that is not based on normal web browser
terminology. Further, a block is not a string, and a function named “block”
should return a block, not a string. Editor should supply an operation with a
name that could be clear to someone who’s never heard of “FormatBlock”. Or it
could just have a name that explicitly refers to the FormatBlock command in its
name. Can you think of a better name? I can think and see if I can come up with
one, but I don’t have an immediate suggestion.