Bug 63037 – Make instantiation of legacy editing position more explicit

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Instead of keeping old constructor, we should make the way to instantiate a legacy editing position more explicit so that people will pay more attention to the deprecated code.

[Ryosuke Niwa]

This step is also necessary to add a new Position constructor that takes Text* and offset.

[Ryosuke Niwa]

Created attachment 97924 [details]
cleanup; likely to fail build on various ports

[Martin Robinson]

Comment on attachment 97924 [details]
cleanup; likely to fail build on various ports

Attachment 97924 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8909844

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 97924 [details])
> Attachment 97924 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/8909844

http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:
Range::create(node->document(),  Position(node->parentNode(), 0), Position(node, 0));

This is terrible :(

[Eric Seidel (no email)]

Comment on attachment 97924 [details]
cleanup; likely to fail build on various ports

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

> Source/WebCore/dom/Position.cpp:272
> +            return createDeprecatedLegacyEditingPosition(n, uncheckedPreviousOffsetForBackwardDeletion(n, o));

Huh?  Why not just createLegacyEditingPosition?  The deprecated seems redundant here.

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 97924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97924&action=review
> 
> > Source/WebCore/dom/Position.cpp:272
> > +            return createDeprecatedLegacyEditingPosition(n, uncheckedPreviousOffsetForBackwardDeletion(n, o));
> 
> Huh?  Why not just createLegacyEditingPosition?  The deprecated seems redundant here.

Sure. I can do that.

[Darin Adler]

Comment on attachment 97924 [details]
cleanup; likely to fail build on various ports

Looks like the only failure EWS found is:

dom/Position.h: In member function ‘void WebCore::AXObjectCache::nodeTextChangePlatformNotification(WebCore::AccessibilityObject*, WebCore::AXObjectCache::AXTextChange, unsigned int, unsigned int)’:
dom/Position.h:73:9: error: ‘WebCore::Position::LegacyEditingOffset::LegacyEditingOffset(int)’ is private
accessibility/gtk/AXObjectCacheAtk.cpp:190:90: error: within this context
dom/Position.h:73:9: error: ‘WebCore::Position::LegacyEditingOffset::LegacyEditingOffset(int)’ is private
accessibility/gtk/AXObjectCacheAtk.cpp:190:109: error: within this context

[Ryosuke Niwa]

(In reply to comment #7)
> (From update of attachment 97924 [details])
> Looks like the only failure EWS found is:
> 
> dom/Position.h: In member function ‘void WebCore::AXObjectCache::nodeTextChangePlatformNotification(WebCore::AccessibilityObject*, WebCore::AXObjectCache::AXTextChange, unsigned int, unsigned int)’:
> dom/Position.h:73:9: error: ‘WebCore::Position::LegacyEditingOffset::LegacyEditingOffset(int)’ is private
> accessibility/gtk/AXObjectCacheAtk.cpp:190:90: error: within this context
> dom/Position.h:73:9: error: ‘WebCore::Position::LegacyEditingOffset::LegacyEditingOffset(int)’ is private
> accessibility/gtk/AXObjectCacheAtk.cpp:190:109: error: within this context

Right. See comment #4. AXObjectCacheAtk.cpp is instantiating a Range with two legacy editing positions.

[Ryosuke Niwa]

Created attachment 98002 [details]
createDeprecatedLegacyEditingPosition to createLegacyEditingPosition; fixed gtk build

[Ryosuke Niwa]

Ping reviewers

[Darin Adler]

Comment on attachment 98002 [details]
createDeprecatedLegacyEditingPosition to createLegacyEditingPosition; fixed gtk build

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

> Source/WebCore/dom/Position.h:70
> +        int value() { return m_offset; }

Maybe make this const?

> Source/WebCore/dom/Position.h:73
> +        LegacyEditingOffset(int offset) : m_offset(offset) { }

Maybe make this explicit too?

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #11)
> (From update of attachment 98002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98002&action=review
> 
> > Source/WebCore/dom/Position.h:70
> > +        int value() { return m_offset; }
> 
> Maybe make this const?
> 
> > Source/WebCore/dom/Position.h:73
> > +        LegacyEditingOffset(int offset) : m_offset(offset) { }
> 
> Maybe make this explicit too?

Will fix.

[Ryosuke Niwa]

Committed r89440: <http://trac.webkit.org/changeset/89440>