Closed Bug 1266782 Opened 6 years ago Closed 6 years ago
Phone Number Selection() method in Accessible Caret Manager for strong refs to doc, sel
MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection.
58 bytes, text/x-review-board-request
See bug 1265750 comment 8 for background... I've verified that we do generate SelectionEvent notifications to selection listeners in AccessibleCaretManager::ExtendPhoneNumberSelection().
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/49189/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49189/
Attachment #8745938 - Flags: review?(mats)
Comment on attachment 8745938 [details] MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection. https://reviewboard.mozilla.org/r/49189/#review46077 Don't we also need an early return if IsTerminated() becomes true inside the loop? (immediatly after the Modify call probably makes most sense) I think we also need the same early return after the ExtendPhoneNumberSelection calls in SelectMoreIfPhoneNumber, at least the first one, because otherwise mPresShell might by null in the second call and crash when accessing it. r=mats with those nits addressed as needed
Attachment #8745938 - Flags: review?(mats) → review+
https://reviewboard.mozilla.org/r/49189/#review46077 Assuming Modify() will make mPresShell becomes nullptr, we'll need IsTerminated() check. I'll add that in next patch. How about we add early return for mPresShell in the beginning of ExtendPhoneNumberSelection()? Then both calls to ExtendPhoneNumberSelection() will be safe, and we don't need to worry about other statements accessing mPresShell added after the second ExtendPhoneNumberSelection() call in the future. SetSelectionDirection() in SelectMoreIfPhoneNumber() should be safe when mPresShell is nullptr. If mPresShell is nullptr, then GetSelection() is nullptr, too.
> How about we add early return for mPresShell in the beginning of ExtendPhoneNumberSelection()? Sure, that makes it even more robust.
Comment on attachment 8745938 [details] MozReview Request: Bug 1266782 - Use RefPtr to hold document and selection in ExtendPhoneNumberSelection. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49189/diff/1-2/
You need to log in before you can comment on or make changes to this bug.