get rid of HyperTextAccessible::FrameSelection()

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: badescunicu)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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.