Bug 120340 - EditorInternalCommand should use Frame& where possible
Summary: EditorInternalCommand should use Frame& where possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-26 20:38 PDT by Sam Weinig
Modified: 2013-08-26 21:08 PDT (History)
0 users

See Also:


Attachments
Patch (85.77 KB, patch)
2013-08-26 20:39 PDT, Sam Weinig
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-08-26 20:38:08 PDT
EditorInternalCommand should use Frame& where possible
Comment 1 Sam Weinig 2013-08-26 20:39:58 PDT
Created attachment 209712 [details]
Patch
Comment 2 Andreas Kling 2013-08-26 20:58:42 PDT
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.
Comment 3 Andreas Kling 2013-08-26 21:04:12 PDT
Comment on attachment 209712 [details]
Patch

r=me
Comment 4 Sam Weinig 2013-08-26 21:07:35 PDT
(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.
Comment 5 Sam Weinig 2013-08-26 21:08:07 PDT
Committed r154663: <http://trac.webkit.org/changeset/154663>