[webkit-reviews] review granted: [Bug 23431] Frame Refactor: Move methods from Frame to SelectionController : [Attachment 26861] Carry out the move.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Holger Freyther <zecke at selfish.org>'s
request for review:
Bug 23431: Frame Refactor: Move methods from Frame to SelectionController
https://bugs.webkit.org/show_bug.cgi?id=23431

Attachment 26861: Carry out the move.
https://bugs.webkit.org/attachment.cgi?id=26861&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   m_frame->clearTypingStyle();
> +	   this->clearTypingStyle();

This is one of a few places where the patch includes "this->" but we want to
omit it. We should only use that explicit "this" if there's a name conflict
that needs to be resolved.

> +TextGranularity SelectionController::selectionGranularity() const
> +{
> +    return m_selectionGranularity;
> +}
> +
> +void SelectionController::setSelectionGranularity(TextGranularity
granularity)
> +{
> +    m_selectionGranularity = granularity;
> +}

These functions are good candidates for inlining. That wasn't practical before
because of FramePrivate.

> +void SelectionController::invalidateSelection()
> +{
> +    setNeedsLayout();
> +    selectionLayoutChanged();
> +}
> +void SelectionController::setCaretVisible(bool flag)
> +{

Missing blank line here.

> +static bool isFrameElement(const Node *n)

When moving the code it would be nice to also move the "*".

> +    RenderObject *renderer = n->renderer();

Same comment here.

> +    bool shouldBlink = m_caretVisible
> +	   && isCaret() && isContentEditable();

Looks like this would fit nicely on one line.

> +#if !PLATFORM(MAC)
> +void SelectionController::setUseSecureKeyboardEntry(bool)
> +{}
> +#endif

The braces should be on separate lines here.

> +CSSMutableStyleDeclaration *SelectionController::typingStyle() const
> +{
> +    return m_typingStyle.get();
> +}
> +
> +void SelectionController::setTypingStyle(CSSMutableStyleDeclaration *style)
> +{
> +    m_typingStyle = style;
> +}
> +
> +void SelectionController::clearTypingStyle()
> +{
> +    m_typingStyle = 0;
> +}

These are all candidates for inlining.

> +#include "RenderLayer.h"

Really unfortunate to have to add an include here just because of
RenderLayer::ScrollAlignment. We should investigate moving that enum out along
with other enums in either its own header or one like ScrollTypes.h.

> +    TextGranularity selectionGranularity() const;
> +    void setSelectionGranularity(TextGranularity);

I could imagine leaving the word "selection" out of this function name now that
it's in the selection controller.

> +    bool shouldChangeSelection(const Selection&) const;
> +    bool shouldDeleteSelection(const Selection&) const;

Maybe even from these.

> +const short enableRomanKeyboardsOnly = -23;

This should have "static" in front of it so it gets internal linkage. Maybe it
should also go inside the function or at the top of the file. I don't think
it's so great to have it before the function in the middle of the file.

> +#ifdef BUILDING_ON_TIGER
> +	   KeyScript(smKeyEnableKybds);
> +#else
> +	   TSMRemoveDocumentProperty(TSMGetActiveDocument(),
kTSMDocumentEnabledInputSourcesPropertyTag);
> +#endif
> +    }
> +}
> +
>  
>  } // namespace WebCore

Extra unwanted blank line here.

Can any more of the SelectionController functions be private?

> -    // FIXME: Granularity is stored separately on the frame, but also in the
selection controller.
> -    // The granularity in the selection controller should be used, and then
this line of code would not be needed.
> -    if (Frame* frame = document()->frame())
> -	   frame->setSelectionGranularity(CharacterGranularity);
> +	   frame->selection()->setSelectionGranularity(CharacterGranularity);
> +    }
>  }

Even though the situation has changed, the FIXME still applies. I would write
it like this.

    // FIXME: The selection controller already stores the selection
granularity.
    // But the code relies on this separate copy being set explicitly. The
    // granularity already known by the controller should be used, and then
    // this line of code would not be needed.

Or something along those lines. I also think that the selectionGranularity
functions in the SelectionController.h header would be a good place for this
same comment. Perhaps instead of having it here.

r=me

Thanks for taking this on.