Closed Bug 965347 Opened 6 years ago Closed 6 years ago

get rid of HyperTextAccessible::FrameSelection()

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: surkov, Assigned: badescunicu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

nowadays it's got to be a trivial wrapper around nsIFrame::GetFrameSelection so we don't need it anymore.
Hello, I want to take this bug as my first contribution to Mozilla, can someone assign me to it?

Regarding the bug, should I delete HyperTextAccessible::FrameSelection() and wherever this method is called, use nsIFrame::GetFrameSelection instead?

Thank you!
(In reply to Nicu Bădescu [:badescunicu] from comment #1)

> Regarding the bug, should I delete HyperTextAccessible::FrameSelection() and
> wherever this method is called, use nsIFrame::GetFrameSelection instead?

yes
Assignee: nobody → badescunicu
Attachment #8369437 - Flags: review?(surkov.alexander)
Comment on attachment 8369437 [details] [diff] [review]
rev1 - Remove HyperTextAccessible::FrameSelection

Review of attachment 8369437 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1182,5 @@
>  HyperTextAccessible::CaretLineNumber()
>  {
>    // Provide the line number for the caret, relative to the
>    // currently focused node. Use a 1-based index
> +  nsRefPtr<nsFrameSelection> frameSelection = GetFrame() ? GetFrame()->GetFrameSelection() : nullptr;

GetFrame() is a virtual call, so it should be worth to have local variable for it or consider having inlined FrameSelection() method (the second approach is less changes which is good)
You mean to leave HyperTextAccessible::FrameSelection as it is right now, move it to HyperTextAccessible-inl.h and make it an inline function?
(In reply to Nicu Bădescu [:badescunicu] from comment #5)
> You mean to leave HyperTextAccessible::FrameSelection as it is right now,
> move it to HyperTextAccessible-inl.h and make it an inline function?

correct
Attachment #8369437 - Attachment is obsolete: true
Attachment #8369437 - Flags: review?(surkov.alexander)
Attachment #8369657 - Flags: review?(surkov.alexander)
Comment on attachment 8369657 [details] [diff] [review]
rev2 - Make FrameSelection an inline function and move it to HyperTextAccessible-inl.h

Review of attachment 8369657 [details] [diff] [review]:
-----------------------------------------------------------------

don't you need to remove virtual keyword from method declaration?
Attachment #8369657 - Attachment is obsolete: true
Attachment #8369657 - Flags: review?(surkov.alexander)
Attachment #8369664 - Flags: review?(surkov.alexander)
Comment on attachment 8369664 [details] [diff] [review]
rev3 - remove virtual keyword

Review of attachment 8369664 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!
Attachment #8369664 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/727d19193bec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.