Bug 23431 – Frame Refactor: Move methods from Frame to SelectionController

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[Holger Freyther]

Move these to the SelectionController as requested by comments in Frame.h

[Holger Freyther]

Created attachment 26860 [details]
Carry out the move.

It compiles, tests pass on the mac. I was hit hard by Weinig killing of FramePrivate and need to verify the result of my git rebase actions. But comments would be appreciated right now.

[Holger Freyther]

Created attachment 26861 [details]
Carry out the move.

It compiles, tests pass on the mac. I was hit hard by Weinig killing of FramePrivate and need to verify the result of my git rebase actions. But comments would be appreciated right now.

SelectionController has now two (overloaded) paintCaret implementations. This looks fishy and I should consider merging them or making one private. I will think about it.

[Holger Freyther]

Comment on attachment 26860 [details]
Carry out the move.

Wrong bug :)

[Darin Adler]

Comment on attachment 26861 [details]
Carry out the move.

> -        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.

[Brent Fulgham]

Unfortunately, this patch no longer applies cleanly.

[Eric Seidel (no email)]

Comment on attachment 26861 [details]
Carry out the move.

Marking r- for lack of ChangeLog and since it no longer applies.

[Ryosuke Niwa]

Fixed in http://trac.webkit.org/changeset/67238.

*** This bug has been marked as a duplicate of bug 45508 ***