[webkit-reviews] review denied: [Bug 39854] Refactor platform dependent editing behavior code out of Settings : [Attachment 60186] patch v3.1 - part II

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 39854: Refactor platform dependent editing behavior code out of Settings
https://bugs.webkit.org/show_bug.cgi?id=39854

Attachment 60186: patch v3.1 - part II
https://bugs.webkit.org/attachment.cgi?id=60186&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
>  static TriState stateStyle(Frame* frame, int propertyID, const char*
desiredValue)
>  {
>      RefPtr<CSSMutableStyleDeclaration> style =
CSSMutableStyleDeclaration::create();
> -    style->setProperty(propertyID, desiredValue);
> -    return frame->editor()->selectionHasStyle(style.get());
> +    style->setProperty(propertyID, desiredValue); // We need to add this
style to pass it to selectionStartHasStyle / selectionHasStyle
> +
> +    TriState styleIsPresent;
> +    if
(!frame->editor()->behavior().shouldConsiderStylePresentOnlyIfThroughoutTheSele
ction())
> +	   styleIsPresent =
frame->editor()->selectionStartHasStyle(style.get()) ? TrueTriState :
FalseTriState;
> +    else
> +	   styleIsPresent = frame->editor()->selectionHasStyle(style.get());
> +
> +    return styleIsPresent;

This is a change in behavior. If we keep it, it should have a layout test and
should maybe be a separate patch. In either case, I'm not sure this change is
correct. I think Darin was just asking that the name of the EditingBehavior
method be more specific. How about something like
shouldToggleStyleBasedOnStartOfSelection?