Bug 119829 – IAccessibleText/2 implementation for AppleWindows port

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Roger Fong]

Implement IAccessibleText and IAccessibleText2.

The first patch is a bit of a rough draft.
First patch implements some of the more obvious core methods.

[Radar WebKit Bug Importer]

<rdar://problem/14742559>

[Roger Fong]

(In reply to comment #1)
> <rdar://problem/14742559>

Actually <rdar://problem/8476150>

[Roger Fong]

Created attachment 208876 [details]
Draft Patch

This is a preliminary patch.
I've yet to test it using NVDA but most of it's straightforward.

This patch is fairly large, I will split it up into smaller patches once I
figure out a good way to divide things up (which will depend on the importance of the methods to NVDA)

If anyone is willing to brave taking a quick cursory glance of it just to point out any glaring mistakes that would be grand.
No pressure tho.

I'll upload some more tested, more split up patches tomorrow, but a preliminary review would definitely help expedite the process.
Thanks

[Brent Fulgham]

Comment on attachment 208876 [details]
Draft Patch

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

I think this looks great!  Sorry this turned into a larger project than I had originally envisioned. :(

> Source/WebKit/win/AccessibleText.cpp:63
> +HRESULT AccessibleText::attributes(long offset, long *startOffset, long *endOffset, BSTR *textAttributes)

The asterisk is part of the type! Please write long* startOffset, etc.

> Source/WebKit/win/AccessibleText.cpp:73
> +HRESULT AccessibleText::caretOffset(long *offset)

Ditto

> Source/WebKit/win/AccessibleText.cpp:76
> +        return E_FAIL;

We usually check that the pointer argument is non-null and return E_POINTER if we are called without a valid pointer. This applies to the other methods as well.

> Source/WebKit/win/AccessibleText.cpp:205
> +    int previous_pos = std::max(0, (int)(offset-1));

Need a C++ cast here...

> Source/WebKit/win/AccessibleText.cpp:528
> +    else

No need for this else case. Just return offset.

[chris fleizach]

Comment on attachment 208876 [details]
Draft Patch

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

> Source/WebKit/win/AccessibleText.cpp:78
> +    VisiblePosition caretPosition = m_object->renderer()->document()->frame()->page()->dragCaretController()->caretPosition();

there's a document() method on axObject that you can call that will check if renderer() exists. you should probably verify that that document exists after retrieving

> Source/WebKit/win/AccessibleText.cpp:123
> +    if (m_object->renderer()->document()->frame()->selection()->isNone())

ditto for document()

> Source/WebKit/win/AccessibleText.cpp:135
> +    Node* node = m_object->renderer()->node();

there's a node() method on axObject. you should prob. verify it exists after retrieving

> Source/WebKit/win/AccessibleText.cpp:260
> +    int textLength = m_object->text().length();

length() returns an unsigned here

> Source/WebKit/win/AccessibleText.cpp:324
> +    int textLength = m_object->text().length();

do you want to use m_object->textLength() here in case of password fields?

> Source/WebKit/win/AccessibleText.cpp:384
> +    m_object->renderer()->document()->frame()->selection()->clear();

ditto about document()

> Source/WebKit/win/AccessibleText.cpp:394
> +    Node* node = m_object->renderer()->node();

ditto about node()

> Source/WebKit/win/AccessibleText.cpp:413
> +HRESULT AccessibleText::nCharacters(long *characters)

* in wrong place

> Source/WebKit/win/AccessibleText.cpp:492
> +HRESULT AccessibleText::newText(IA2TextSegment *newText)

* in wrong place

> Source/WebKit/win/AccessibleText.cpp:500
> +HRESULT AccessibleText::oldText(IA2TextSegment *oldText)

* ditto

> Source/WebKit/win/AccessibleText.cpp:510
> +HRESULT AccessibleText::attributeRange(long offset, BSTR filter, long *startOffset, long *endOffset, BSTR *attributeValues)

* ditto

> Source/WebKit/win/AccessibleText.cpp:527
> +        return m_object->renderer()->document()->frame()->selection()->start().offsetInContainerNode();

ditto about document()

> Source/WebKit/win/AccessibleText.cpp:537
> +    Document* document = m_object->renderer()->document();

ditto about document()

> Source/WebKit/win/AccessibleText.h:40
> +	virtual HRESULT STDMETHODCALLTYPE attributes(long offset,	long *startOffset, long *endOffset,	BSTR *textAttributes);

tabs exist in this line

[Roger Fong]

I've made the suggested changes.

Two things I'm having issues with however.

1. When to create a new AccessibleText object.
This is controlled by the isTextControl() method is AccessibilityObject.cpp

Currently this returns true if the role is 
    case TextAreaRole:
    case TextFieldRole:
    case ComboBoxRole:
	case StaticTextRole:
	case ListItemRole:
(I've added the last 2)

I'm not sure that this is correct hwoever.

Another issue is that in the query interface method, I only ever seem to hit the IAccessibleText case. Never IAccessibleText2 or IAccessibleEditableText,

although the functionality of say copy paste using NVDA commands seems to work regardless.

This is a tad confusing because an IAccessibleText2 is an IAccessibleText.
The way things are organized right now, AccesibleBase implements IAccessibleText2 (which inherits from IAccessibleText) and from IAccessibleEditableText. Since NVDA supposedly uses IAccessible2 shouldn't it always be seeing the IAccessibleText2 uiid case?

[Roger Fong]

Created attachment 209519 [details]
patch

Not implemented methods are noted in ChangeLogs.
More implementations of API to come.

[Roger Fong]

Comment on attachment 209519 [details]
patch

I will split up the patch

[Roger Fong]

Created attachment 209521 [details]
HookUp Interfaces to WebKit

[chris fleizach]

Comment on attachment 209521 [details]
HookUp Interfaces to WebKit

do you need to add these files, or do they exist already?

[Roger Fong]

Created attachment 209522 [details]
Add IAccessible(Editable)Text implementation

Depends on previous patch to build

[Roger Fong]

(In reply to comment #10)
> (From update of attachment 209521 [details])
> do you need to add these files, or do they exist already?

My second patch adds those files...
Hmm I guess I'll need to commit both patches at the same time to minimize build breakage.
Or we can just go with the larger patch, which'll just do everything at once.

[WebKit Commit Bot]

Attachment 209522 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/AccessibleBase.cpp', u'Source/WebKit/win/AccessibleTextImpl.cpp', u'Source/WebKit/win/AccessibleTextImpl.h', u'Source/WebKit/win/ChangeLog']" exit_code: 1
Source/WebKit/win/AccessibleTextImpl.h:45:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:46:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:47:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:49:  The parameter name "offset" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:52:  The parameter name "scrollType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:53:  The parameter name "coordinateType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:67:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:77:  AccessibleText::get_caretOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:91:  AccessibleText::get_characterExtents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:125:  AccessibleText::get_nSelections is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:137:  AccessibleText::get_offsetAtPoint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:168:  AccessibleText::get_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:185:  AccessibleText::get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:201:  AccessibleText::get_textBeforeOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:209:  AccessibleText::get_textAfterOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:217:  AccessibleText::get_textAtOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:268:  AccessibleText::get_nCharacters is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:273:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/win/AccessibleTextImpl.cpp:345:  AccessibleText::get_Text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:353:  AccessibleText::get_oldText is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:363:  AccessibleText::get_attributeRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:498:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 23 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 209521 [details] [details])
> > do you need to add these files, or do they exist already?
> 
> My second patch adds those files...
> Hmm I guess I'll need to commit both patches at the same time to minimize build breakage.
> Or we can just go with the larger patch, which'll just do everything at once.

should probably do one patch here so it can be easily rolled out or rolled in as needed

[Roger Fong]

Created attachment 209523 [details]
Combined patch

[WebKit Commit Bot]

Attachment 209523 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj.filters', u'Source/WebKit/win/AccessibleBase.cpp', u'Source/WebKit/win/AccessibleBase.h', u'Source/WebKit/win/AccessibleTextImpl.cpp', u'Source/WebKit/win/AccessibleTextImpl.h', u'Source/WebKit/win/ChangeLog']" exit_code: 1
Source/WebKit/win/AccessibleTextImpl.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:67:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:77:  AccessibleText::get_caretOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:91:  AccessibleText::get_characterExtents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:125:  AccessibleText::get_nSelections is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:137:  AccessibleText::get_offsetAtPoint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:168:  AccessibleText::get_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:185:  AccessibleText::get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:201:  AccessibleText::get_textBeforeOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:209:  AccessibleText::get_textAfterOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:217:  AccessibleText::get_textAtOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:268:  AccessibleText::get_nCharacters is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:273:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit/win/AccessibleTextImpl.cpp:345:  AccessibleText::get_Text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:353:  AccessibleText::get_oldText is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:363:  AccessibleText::get_attributeRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:498:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.h:45:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:46:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:47:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:49:  The parameter name "offset" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:52:  The parameter name "scrollType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:53:  The parameter name "coordinateType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 23 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roger Fong]

Created attachment 209524 [details]
Combined patch with style fixes

Ignoring some of the other style errors. I don't think they apply here.

[WebKit Commit Bot]

Attachment 209524 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj.filters', u'Source/WebKit/win/AccessibleBase.cpp', u'Source/WebKit/win/AccessibleBase.h', u'Source/WebKit/win/AccessibleTextImpl.cpp', u'Source/WebKit/win/AccessibleTextImpl.h', u'Source/WebKit/win/ChangeLog']" exit_code: 1
Source/WebKit/win/AccessibleTextImpl.cpp:67:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:77:  AccessibleText::get_caretOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:91:  AccessibleText::get_characterExtents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:125:  AccessibleText::get_nSelections is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:137:  AccessibleText::get_offsetAtPoint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:168:  AccessibleText::get_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:185:  AccessibleText::get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:201:  AccessibleText::get_textBeforeOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:209:  AccessibleText::get_textAfterOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:217:  AccessibleText::get_textAtOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:268:  AccessibleText::get_nCharacters is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:345:  AccessibleText::get_Text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:353:  AccessibleText::get_oldText is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:363:  AccessibleText::get_attributeRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:498:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.h:45:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:46:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:47:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:49:  The parameter name "offset" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:52:  The parameter name "scrollType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:53:  The parameter name "coordinateType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 21 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 209523 [details]
Combined patch

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

> Source/WebKit/win/AccessibleTextImpl.cpp:130
> +    if (m_object->renderer()->document()->frame()->selection().isNone())

You should use m_object->document() and ensure existence of document()

> Source/WebKit/win/AccessibleTextImpl.cpp:235
> +    m_object->renderer()->document()->frame()->selection().clear();

ditto about document()

> Source/WebKit/win/AccessibleTextImpl.cpp:548
> +    ASSERT(m_object->renderer()->document()->frame());

why do you need to assert frame() here?

[Roger Fong]

(In reply to comment #19)
> (From update of attachment 209523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209523&action=review
> 
> > Source/WebKit/win/AccessibleTextImpl.cpp:130
> > +    if (m_object->renderer()->document()->frame()->selection().isNone())
> 
> You should use m_object->document() and ensure existence of document()
> 
> > Source/WebKit/win/AccessibleTextImpl.cpp:235
> > +    m_object->renderer()->document()->frame()->selection().clear();
> 
> ditto about document()

Is checking the existence of m_object->renderer()->document() not the same thing?
I do that in the initialCheck method.

> 
> > Source/WebKit/win/AccessibleTextImpl.cpp:548
> > +    ASSERT(m_object->renderer()->document()->frame());
> 
> why do you need to assert frame() here?

Oops I don't.

[chris fleizach]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 209523 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209523&action=review
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:130
> > > +    if (m_object->renderer()->document()->frame()->selection().isNone())
> > 
> > You should use m_object->document() and ensure existence of document()
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:235
> > > +    m_object->renderer()->document()->frame()->selection().clear();
> > 
> > ditto about document()
> 
> Is checking the existence of m_object->renderer()->document() not the same thing?
> I do that in the initialCheck method.

I think it's cleaner if we don't have to ask for the renderer() object. I think the platform implementations should try hard to work mainly through the WebCore AccessibilityObject interface rather than diving into objects it holds on to.

There's also a chance that renderer() disappears or doesn't exist for the object because maybe in a <canvas>

> 
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:548
> > > +    ASSERT(m_object->renderer()->document()->frame());
> > 
> > why do you need to assert frame() here?
> 
> Oops I don't.

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 209523 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209523&action=review
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:130
> > > +    if (m_object->renderer()->document()->frame()->selection().isNone())
> > 
> > You should use m_object->document() and ensure existence of document()
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:235
> > > +    m_object->renderer()->document()->frame()->selection().clear();
> > 
> > ditto about document()
> 
> Is checking the existence of m_object->renderer()->document() not the same thing?
> I do that in the initialCheck method.
> 
> > 
> > > Source/WebKit/win/AccessibleTextImpl.cpp:548
> > > +    ASSERT(m_object->renderer()->document()->frame());
> > 
> > why do you need to assert frame() here?
> 
> Oops I don't.

[Roger Fong]

Created attachment 209525 [details]
patch

Ok I've replaced all calls to renderer->document with jsut document, including in the initialcheck method.

[WebKit Commit Bot]

Attachment 209525 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj', u'Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKit/WebKit.vcxproj.filters', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj', u'Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj.filters', u'Source/WebKit/win/AccessibleBase.cpp', u'Source/WebKit/win/AccessibleBase.h', u'Source/WebKit/win/AccessibleTextImpl.cpp', u'Source/WebKit/win/AccessibleTextImpl.h', u'Source/WebKit/win/ChangeLog']" exit_code: 1
Source/WebKit/win/AccessibleTextImpl.cpp:67:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:77:  AccessibleText::get_caretOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:91:  AccessibleText::get_characterExtents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:125:  AccessibleText::get_nSelections is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:137:  AccessibleText::get_offsetAtPoint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:168:  AccessibleText::get_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:185:  AccessibleText::get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:201:  AccessibleText::get_textBeforeOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:209:  AccessibleText::get_textAfterOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:217:  AccessibleText::get_textAtOffset is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:268:  AccessibleText::get_nCharacters is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:345:  AccessibleText::get_Text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:353:  AccessibleText::get_oldText is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:363:  AccessibleText::get_attributeRange is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.cpp:498:  AccessibleText::get_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit/win/AccessibleTextImpl.h:45:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:46:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:47:  The parameter name "boundaryType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:49:  The parameter name "offset" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:52:  The parameter name "scrollType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/AccessibleTextImpl.h:53:  The parameter name "coordinateType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 21 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 209525 [details]
patch

Yes!

[Roger Fong]

Committed http://trac.webkit.org/changeset/154627