[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
bugzilla-daemon at webkit.org
Fri Jan 23 14:16:47 PST 2009
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
Attachment 26861: Carry out the move.
------- 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
> + 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)
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);
> + TSMRemoveDocumentProperty(TSMGetActiveDocument(),
> + }
> } // 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
> - // 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
// 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.
Thanks for taking this on.
More information about the webkit-reviews