Bug 119829

Summary: IAccessibleText/2 implementation for AppleWindows port
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cfleizach, commit-queue, jhoneycutt, jonlee, roger_fong, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 120119, 120121, 120122    
Bug Blocks:    
Attachments:
Description Flags
Draft Patch
none
patch
none
HookUp Interfaces to WebKit
none
Add IAccessible(Editable)Text implementation
none
Combined patch
none
Combined patch with style fixes
none
patch cfleizach: review+

Description Roger Fong 2013-08-14 17:54:52 PDT 
Implement IAccessibleText and IAccessibleText2.

The first patch is a bit of a rough draft.
First patch implements some of the more obvious core methods.
Comment 1 Radar WebKit Bug Importer 2013-08-14 17:55:04 PDT 
<rdar://problem/14742559>
Comment 2 Roger Fong 2013-08-14 18:00:57 PDT 
(In reply to comment #1)
> <rdar://problem/14742559>

Actually <rdar://problem/8476150>
Comment 3 Roger Fong 2013-08-15 19:43:39 PDT 
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
Comment 4 Brent Fulgham 2013-08-15 20:33:48 PDT 
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.
Comment 5 chris fleizach 2013-08-16 09:07:27 PDT 
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
Comment 6 Roger Fong 2013-08-16 17:42:34 PDT 
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?
Comment 7 Roger Fong 2013-08-23 15:46:26 PDT 
Created attachment 209519 [details]
patch

Not implemented methods are noted in ChangeLogs.
More implementations of API to come.
Comment 8 Roger Fong 2013-08-23 15:50:50 PDT 
Comment on attachment 209519 [details]
patch

I will split up the patch
Comment 9 Roger Fong 2013-08-23 15:53:16 PDT 
Created attachment 209521 [details]
HookUp Interfaces to WebKit
Comment 10 chris fleizach 2013-08-23 15:56:29 PDT 
Comment on attachment 209521 [details]
HookUp Interfaces to WebKit

do you need to add these files, or do they exist already?
Comment 11 Roger Fong 2013-08-23 15:56:59 PDT 
Created attachment 209522 [details]
Add IAccessible(Editable)Text implementation

Depends on previous patch to build
Comment 12 Roger Fong 2013-08-23 15:58:26 PDT 
(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.
Comment 13 WebKit Commit Bot 2013-08-23 15:58:50 PDT 
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.
Comment 14 chris fleizach 2013-08-23 15:59:53 PDT 
(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
Comment 15 Roger Fong 2013-08-23 16:03:55 PDT 
Created attachment 209523 [details]
Combined patch
Comment 16 WebKit Commit Bot 2013-08-23 16:05:59 PDT 
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.
Comment 17 Roger Fong 2013-08-23 16:06:25 PDT 
Created attachment 209524 [details]
Combined patch with style fixes

Ignoring some of the other style errors. I don't think they apply here.
Comment 18 WebKit Commit Bot 2013-08-23 16:08:51 PDT 
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.
Comment 19 chris fleizach 2013-08-23 16:10:27 PDT 
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?
Comment 20 Roger Fong 2013-08-23 16:14:38 PDT 
(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.
Comment 21 chris fleizach 2013-08-23 16:22:18 PDT 
(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.
Comment 22 Roger Fong 2013-08-23 16:37:24 PDT 
Created attachment 209525 [details]
patch

Ok I've replaced all calls to renderer->document with jsut document, including in the initialcheck method.
Comment 23 WebKit Commit Bot 2013-08-23 16:39:47 PDT 
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.
Comment 24 chris fleizach 2013-08-26 11:15:22 PDT 
Comment on attachment 209525 [details]
patch

Yes!
Comment 25 Roger Fong 2013-08-26 12:28:18 PDT 
Committed http://trac.webkit.org/changeset/154627