965347 - get rid of HyperTextAccessible::FrameSelection()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

nowadays it's got to be a trivial wrapper around nsIFrame::GetFrameSelection so we don't need it anymore.

Comment 1

[Nicu Bădescu [:badescunicu]]

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!

Comment 2

[alexander :surkov (:asurkov)]

(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

Comment 3

[Nicu Bădescu [:badescunicu]]

Attachment: rev1 - Remove HyperTextAccessible::FrameSelection

Comment 4

[alexander :surkov (:asurkov)]

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)

Comment 5

[Nicu Bădescu [:badescunicu]]

You mean to leave HyperTextAccessible::FrameSelection as it is right now, move it to HyperTextAccessible-inl.h and make it an inline function?

Comment 6

[alexander :surkov (:asurkov)]

(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

Comment 7

[Nicu Bădescu [:badescunicu]]

Attachment: rev2 - Make FrameSelection an inline function and move it to HyperTextAccessible-inl.h

Comment 8

[alexander :surkov (:asurkov)]

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?

Comment 9

[Nicu Bădescu [:badescunicu]]

Attachment: rev3 - remove virtual keyword

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8369664 [details] [diff] [review]
rev3 - remove virtual keyword

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

r=me, thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/727d19193bec

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/727d19193bec