[webkit-reviews] review requested: [Bug 27156] SplitElementCommand shouldn't be duplicating id attribute : [Attachment 63113] fixed per ojan's comments

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  for review:
Bug 27156: SplitElementCommand shouldn't be duplicating id attribute
https://bugs.webkit.org/show_bug.cgi?id=27156

Attachment 63113: fixed per ojan's comments
https://bugs.webkit.org/attachment.cgi?id=63113&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>

(In reply to comment #22)
> (From update of attachment 63014 [details])
> > +	 enum InlineStyleRemovalMode { RemoveAttributesAndElement, RemoveNone
};
> 
> s/RemoveAttributesAndElement/RemoveAttributesAndElements

Done.

> > +	 bool removeInlineStyleFromElement(CSSMutableStyleDeclaration*,
HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +	 bool removeHTMLFontStyle(CSSMutableStyleDeclaration*, HTMLElement*,
InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +	 bool removeHTMLBidiEmbeddingStyle(CSSMutableStyleDeclaration*,
HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +	 bool removeCSSStyle(CSSMutableStyleDeclaration*, HTMLElement*,
InlineStyleRemovalMode = RemoveAttributesAndElement);
> 
> Typically, returning a boolean value from a method indicates whether it
succeeded or not. In this case, it indicates whether we would have removed
something. Which isn't exactly the same a success. Can we return an enum here
as well?

I can't agree on this. Adding another enum will make the functions too verbose
IMHO. And there are plenty of member functions such as
mergeStartWithPreviousIfIdentical whose return value indicates whether or not
the operation was done or not. It's pretty standard that remove function always
succeed and therefore have a void type. So remove* being a boolean type pretty
much tells us that it's indicating whether or not attributes / elements have
been (or should have been removed) or not.

In general, I'm not a big fan of using enum for something that doesn't have
more than two states other than function argument. If we had boolean variable,
then I can tell immediately that it can be either true or false. But if I had
enum, there could be more than two states so I have to think twice when reading
the code. I agree that the code calling a method with enum is much easier to
read than the one calling with a boolean variable because I have to check the
function declaration to know what it is. However, when I read the code of these
methods as is, I have to wonder if there are such thing as RemoveElements or
RemoveAttributes and if conditions checking with == are right or not.  If we
made the method to return enum type, then caller will check it against Removed
or NotRemoved.	But since it's enum type, a person reading the code will wonder
whether there are some other states.