Bug 47712 - Match the elements supported by execCommand('formatBlock') and queryCommandValue('formatBlock')
Summary: Match the elements supported by execCommand('formatBlock') and queryCommandVa...
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
Depends on: 19795
  Show dependency treegraph
Reported: 2010-10-14 23:04 PDT by Ryosuke Niwa
Modified: 2010-10-15 13:53 PDT (History)
3 users (show)

See Also:

fixes the bug (18.49 KB, patch)
2010-10-15 10:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Darin's comments (24.89 KB, patch)
2010-10-15 11:59 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-10-14 23:04:27 PDT
isElementForFormatBlockCommand and FormatBlockCommand::isElementToApplyInFormatBlockCommand both determine elements used in FormatBlockCommand.  We should merge these two functions into one.
Comment 1 Ryosuke Niwa 2010-10-15 10:52:03 PDT
Created attachment 70883 [details]
fixes the bug
Comment 2 Darin Adler 2010-10-15 11:05:01 PDT
Comment on attachment 70883 [details]
fixes the bug

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.
Comment 3 Ryosuke Niwa 2010-10-15 11:59:40 PDT
Created attachment 70886 [details]
fixed per Darin's comments
Comment 4 Ryosuke Niwa 2010-10-15 13:53:59 PDT
Committed r69876: <http://trac.webkit.org/changeset/69876>