Bug 120340 – EditorInternalCommand should use Frame& where possible

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

EditorInternalCommand should use Frame& where possible

[Sam Weinig]

Created attachment 209712 [details]
Patch

[Andreas Kling]

Comment on attachment 209712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209712&action=review

> Source/WebCore/editing/EditorCommand.cpp:100
> -    return node->document()->frame();
> +    return *node->document()->frame();

We should find a way to document why it's OK to dereference the Frame here, ideally through some compile-time mechanism.
Also, is 'frame != node->document()->frame()' ever true?

> Source/WebCore/editing/EditorCommand.cpp:1747
> -        return m_command->state(m_frame.get(), triggeringEvent) == TrueTriState ? "true" : "false";
> -    return m_command->value(m_frame.get(), triggeringEvent);
> +        return m_command->state(*m_frame, triggeringEvent) == TrueTriState ? "true" : "false";
> +    return m_command->value(*m_frame, triggeringEvent);

In cases like this, I think it's better to have a (possibly private) frame() getter that returns a reference, and using that instead of *m_frame.
It's not immediately obvious that *m_frame is safe.

[Andreas Kling]

Comment on attachment 209712 [details]
Patch

r=me

[Sam Weinig]

(In reply to comment #2)
> (From update of attachment 209712 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209712&action=review
> 
> > Source/WebCore/editing/EditorCommand.cpp:100
> > -    return node->document()->frame();
> > +    return *node->document()->frame();
> 
> We should find a way to document why it's OK to dereference the Frame here, ideally through some compile-time mechanism.
> Also, is 'frame != node->document()->frame()' ever true?

I was unable to convince myself that it is ok to dereference frame here, so I move this back to being a pointer.

> 
> > Source/WebCore/editing/EditorCommand.cpp:1747
> > -        return m_command->state(m_frame.get(), triggeringEvent) == TrueTriState ? "true" : "false";
> > -    return m_command->value(m_frame.get(), triggeringEvent);
> > +        return m_command->state(*m_frame, triggeringEvent) == TrueTriState ? "true" : "false";
> > +    return m_command->value(*m_frame, triggeringEvent);
> 
> In cases like this, I think it's better to have a (possibly private) frame() getter that returns a reference, and using that instead of *m_frame.
> It's not immediately obvious that *m_frame is safe.

There is a null check for m_frame right before this code.

[Sam Weinig]

Committed r154663: <http://trac.webkit.org/changeset/154663>