[webkit-reviews] review granted: [Bug 47712] Merge isElementForFormatBlockCommand and FormatBlockCommand::isElementToApplyInFormatBlockCommand : [Attachment 70883] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47712: Merge isElementForFormatBlockCommand and
FormatBlockCommand::isElementToApplyInFormatBlockCommand
https://bugs.webkit.org/show_bug.cgi?id=47712

Attachment 70883: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=70883&action=review

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

If the behavior here is not changing, I suggest landing the new test first,
then the code change. But if the behavior is changing, then it’s great to land
them together.

I read this over and over again and could not figure out if the change log was
saying this was refactoring only or a bug fix. If it’s a bug fix, then the
wording should emphasize the behavior change and then go on to mention the code
structure, rather than describing it the other way around.

The test here includes covers the things that we do support, but what about
things we do not support? I would be just as interested in seeing at least some
of those test cases too.

> WebCore/editing/FormatBlockCommand.cpp:42
> +inline bool isElementForFormatBlock(Node* node) { return
node->isElementNode() &&
isElementForFormatBlock(static_cast<Element*>(node)->tagQName()); }

Since this function is inside a .cpp file it should be marked static, so it
gets internal linkage.

Also, I don’t think this deserves the “all on one line” format. lets use the
more typical format.

> WebCore/editing/FormatBlockCommand.cpp:107
>  // FIXME: We should consider merging this function with
isElementForFormatBlockCommand in Editor.cpp

You forgot to remove this FIXME.

> WebCore/editing/FormatBlockCommand.h:43
> +    bool didApply() const { return m_didApply; }
>  private:

Our usual style is to leave a blank line before private.