[Webkit-unassigned] [Bug 23431] Frame Refactor: Move methods from Frame to SelectionController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23431


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26861|review?                     |review+
               Flag|                            |




------- Comment #4 from darin at apple.com  2009-01-23 14:16 PDT -------
(From update of attachment 26861)
> -        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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.