[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