WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
214215
Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
https://bugs.webkit.org/show_bug.cgi?id=214215
Summary
Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h...
Darin Adler
Reported
2020-07-10 21:42:59 PDT
Remove live ranges from AccessibilityObject.h, AccessibilityObjectInterface.h, AccessibilityRenderObject.h, AXIsolatedObject.h
Attachments
Patch
(60.50 KB, patch)
2020-07-10 21:55 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(63.75 KB, patch)
2020-07-10 22:25 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(63.91 KB, patch)
2020-07-11 09:47 PDT
,
Darin Adler
sam
: review+
Details
Formatted Diff
Diff
Patch
(76.95 KB, patch)
2020-07-11 13:48 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2020-07-10 21:55:46 PDT
Comment hidden (obsolete)
Created
attachment 404044
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2020-07-10 21:56:16 PDT
<
rdar://problem/65382291
>
Darin Adler
Comment 3
2020-07-10 22:25:42 PDT
Comment hidden (obsolete)
Created
attachment 404046
[details]
Patch
Darin Adler
Comment 4
2020-07-11 09:47:33 PDT
Created
attachment 404056
[details]
Patch
Sam Weinig
Comment 5
2020-07-11 10:27:23 PDT
Comment on
attachment 404056
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=404056&action=review
> Source/WebCore/accessibility/AccessibilityObject.cpp:309 > + if (misspellingRange->compareBoundaryPoints(Range::END_TO_END, createLiveRange(start)).releaseReturnValue() > 0)
It's a bit unfortunate that this live range needs to get created each time through the loop. Might be worth caching it until we have a version of compareBoundaryPoints that can take a SimpleRange.
> Source/WebCore/accessibility/AccessibilityObject.cpp:1278 > + auto start = makeBoundaryPoint(visiblePositionRange.start); > + auto end = makeBoundaryPoint(visiblePositionRange.end); > + if (!start || !end) > return String();
I'd probably break this up into: auto start = makeBoundaryPoint(visiblePositionRange.start); if (!start) return { }; auto end = makeBoundaryPoint(visiblePositionRange.end); if (!end) return { }; to skip the unnecessary construction of end if start is null.
> Source/WebCore/accessibility/AccessibilityObject.cpp:1281 > + for (TextIterator it({ *start, *end }); !it.atEnd(); it.advance()) {
One day, I will quietly turn this into a proper c++ style iterator.
> Source/WebCore/accessibility/AccessibilityObject.cpp:1303 > + auto start = makeBoundaryPoint(visiblePositionRange.start); > + auto end = makeBoundaryPoint(visiblePositionRange.end); > + if (!start || !end) > + return -1; // FIXME: Why not return 0?
Same comment about splitting this up (and in a few more places below.
> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:917 > + auto start = VisiblePosition { createLegacyEditingPosition(range->start) };
This could be moved within the if-statement below.
> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2170 > + CharacterOffset start = cache->startOrEndCharacterOffsetForRange(range, true); > + CharacterOffset end = cache->startOrEndCharacterOffsetForRange(range, false);
Could use auto here. function name makes it very clear what the return type is.
> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:698 > + operation.textRanges.reserveInitialCapacity(markerRanges.count); > + for (id markerRange : markerRanges) { > + if (auto range = [obj rangeForTextMarkerRange:markerRange]) > + operation.textRanges.append(*range); > + }
This could be slightly more optimal by using uncheckedAppend. I don't think we have a map() for this case, but it would be nice if you could write something like: operation.textRanges = WTF::mapIgnoringNullopts(markerRanges, [] (const auto& v) -> Optional<SimpleRange> { if (auto range = [obj rangeForTextMarkerRange:markerRange]) return *range; return WTF::nullopt; });
Darin Adler
Comment 6
2020-07-11 11:50:47 PDT
Comment on
attachment 404056
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=404056&action=review
I guess I still have a crash to fix.
>> Source/WebCore/accessibility/AccessibilityObject.cpp:309 >> + if (misspellingRange->compareBoundaryPoints(Range::END_TO_END, createLiveRange(start)).releaseReturnValue() > 0) > > It's a bit unfortunate that this live range needs to get created each time through the loop. Might be worth caching it until we have a version of compareBoundaryPoints that can take a SimpleRange.
I’ve been pretty cavalier about such things, often valuing clarity of future refactoring over efficiency. I’m fine hosting it out of the loop in this case, though.
>> Source/WebCore/accessibility/AccessibilityObject.cpp:1278 >> return String(); > > I'd probably break this up into: > > auto start = makeBoundaryPoint(visiblePositionRange.start); > if (!start) > return { }; > auto end = makeBoundaryPoint(visiblePositionRange.end); > if (!end) > return { }; > > to skip the unnecessary construction of end if start is null.
I’d be willing to do this, but I intentionally used the more economical style, even though it’s less efficient for the null case.
>> Source/WebCore/accessibility/AccessibilityObject.cpp:1281 >> + for (TextIterator it({ *start, *end }); !it.atEnd(); it.advance()) { > > One day, I will quietly turn this into a proper c++ style iterator.
Yes, making some style of TextIterator work in a range-based for loop would not even be all that hard!
>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:917 >> + auto start = VisiblePosition { createLegacyEditingPosition(range->start) }; > > This could be moved within the if-statement below.
It can’t, because end is computed based on start.
>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2170 >> + CharacterOffset end = cache->startOrEndCharacterOffsetForRange(range, false); > > Could use auto here. function name makes it very clear what the return type is.
OK.
>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:698 >> + } > > This could be slightly more optimal by using uncheckedAppend. I don't think we have a map() for this case, but it would be nice if you could write something like: > > operation.textRanges = WTF::mapIgnoringNullopts(markerRanges, [] (const auto& v) -> Optional<SimpleRange> { > if (auto range = [obj rangeForTextMarkerRange:markerRange]) > return *range; > return WTF::nullopt; > });
I think we are *very* close to having a map() for this case. We have makeVector in VectorCocoa.h that does things like this. However, unlike the createNSArray function in the same file, it does yet not have a version that takes a mapping function, so I would have to overload makeVectorElement to use it.
Darin Adler
Comment 7
2020-07-11 13:48:38 PDT
Created
attachment 404068
[details]
Patch
Darin Adler
Comment 8
2020-07-11 13:49:12 PDT
Did everything you suggested, and it changed the patch a bit.
Darin Adler
Comment 9
2020-07-11 15:38:51 PDT
Committed
r264271
: <
https://trac.webkit.org/changeset/264271
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug