[webkit-reviews] review granted: [Bug 34564] Copying can result in span around block elements on the clipboard : [Attachment 103568] Renamed enum values

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 10 19:04:56 PDT 2011


Tony Chang <tony at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 34564: Copying can result in span around block elements on the clipboard
https://bugs.webkit.org/show_bug.cgi?id=34564

Attachment 103568: Renamed enum values
https://bugs.webkit.org/attachment.cgi?id=103568&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103568&action=review


> Source/WebCore/ChangeLog:34
> +	   with shouldEmit set false first to compute the highest node to be
serialized. The second call to

Nit: Use enum name.

> Source/WebCore/ChangeLog:37
> +	   Outputs string only if shouldEmit is true.

Nit: Ditto.

> Source/WebCore/editing/markup.cpp:221
> +    bool parentIsTextarea = text->parentElement() &&
text->parentElement()->tagQName() == textareaTag;
> +    bool wrappingSpan = shouldApplyWrappingStyle(text) && !parentIsTextarea;


FWIW, the usage of const bool for local variables seems kind of arbitrary.  I
don't think there's a style guide rule one way or another (and I don't have a
strong preference, I find them not that useful but not that noisy either), but
it would be nice if we were at least consistent within a file.	E.g., you could
make these const.

> Source/WebCore/editing/markup.cpp:291
> +    bool shouldAnnotateOrForceInline = element->isHTMLElement() &&
(shouldAnnotate() || addDisplayInline);
> +    bool shouldOverrideStyleAttr = shouldAnnotateOrForceInline ||
shouldApplyWrappingStyle(element);

...these could be const too.


More information about the webkit-reviews mailing list