Bug 120820 - [Windows] Implement text offset methods of IAccessibleText interface
Summary: [Windows] Implement text offset methods of IAccessibleText interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-05 18:35 PDT by Roger Fong
Modified: 2013-09-06 14:33 PDT (History)
7 users (show)

See Also:


Attachments
patch (9.56 KB, patch)
2013-09-05 18:52 PDT, Roger Fong
bfulgham: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (840.28 KB, application/zip)
2013-09-05 19:26 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2013-09-05 18:35:10 PDT
Left these out previously.
They've now been implemented and function properly.
Comment 1 Roger Fong 2013-09-05 18:36:56 PDT
<rdar://problem/14925242>
Comment 2 Roger Fong 2013-09-05 18:52:54 PDT
Created attachment 210692 [details]
patch
Comment 3 Build Bot 2013-09-05 19:26:25 PDT
Comment on attachment 210692 [details]
patch

Attachment 210692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1707194

New failing tests:
media/video-object-fit.html
Comment 4 Build Bot 2013-09-05 19:26:27 PDT
Created attachment 210696 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 5 Roger Fong 2013-09-05 19:38:01 PDT
(In reply to comment #4)
> Created an attachment (id=210696) [details]
> Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

This patch does not affect mac in the slightest. Flaky test.
Comment 6 Brent Fulgham 2013-09-05 20:47:10 PDT
Comment on attachment 210692 [details]
patch

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

A couple of minor typos. Please fix when landing.

> Source/WebKit/win/AccessibleTextImpl.cpp:-206
> -    return E_NOTIMPL;

Don't we need to make sure startOffset and endOffset are non-null, and return E_POINTER if they are?

> Source/WebKit/win/AccessibleTextImpl.cpp:209
> +    if (offset < 0 && offset > m_object->text().length())

Shouldn't this be an ||? I don't know if too man things that are negative and also larger than the text length! :)

> Source/WebKit/win/AccessibleTextImpl.cpp:215
> +    int previousPos = std::max(0, (int)(offset-1));

Please use a C++ cast.

> Source/WebKit/win/AccessibleTextImpl.cpp:256
> +    *text = SysAllocStringLen(substringText.characters(), substringText.length());

On - we should check that text is also a valid pointer passed to the function?

> Source/WebKit/win/AccessibleTextImpl.cpp:271
> +    if (offset < 0 && offset > textLength)

These two cases are exclusive; use ||

> Source/WebKit/win/AccessibleTextImpl.cpp:311
> +    *endOffset = textRange.end.deepEquivalent().offsetInContainerNode();

Check locations and return E_POINTER if they are not valid.

> Source/WebKit/win/AccessibleTextImpl.cpp:317
> +    *text = SysAllocStringLen(substringText.characters(), substringText.length());

Ditto for text.

> Source/WebKit/win/AccessibleTextImpl.cpp:333
> +    if (offset < 0 && offset > textLength)

|| please.
Comment 7 Roger Fong 2013-09-06 14:33:07 PDT
Committed with changes above:
http://trac.webkit.org/changeset/155212