Summary: | EditorInternalCommand should use Frame& where possible | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Sam Weinig
2013-08-26 20:38:08 PDT
Created attachment 209712 [details]
Patch
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 on attachment 209712 [details]
Patch
r=me
(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. Committed r154663: <http://trac.webkit.org/changeset/154663> |