1136065 - GetRangeCount() and RangeCount() do the same thing in Selection.h

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (UTC-7)]

We have two identical functions GetRangeCount() [1] and RangeCount() [2] in Selection.h.  Besides, we already have another GetRangeCount(int32_t* aRangeCount) [3] which implements nsISelection.idl.

To avoid confusion, I think we should keep RangeCount() and eliminate the zero argument GetRangeCount().

[1] http://hg.mozilla.org/mozilla-central/file/368c62292249/layout/generic/Selection.h#l118
[2] http://hg.mozilla.org/mozilla-central/file/368c62292249/layout/generic/Selection.h#l163
[3] http://hg.mozilla.org/mozilla-central/file/368c62292249/layout/generic/nsSelection.cpp#l4966

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Also, the WebIDL bindings call RangeCount(), if I'm not mistaken.

Comment 2

[Ting-Yu Lin [:TYLin] (UTC-7)]

Attachment: Remove GetRangeCount() in Selection.h

We replace GetRangeCount() with the identical function RangeCount() to
avoid any confusion.

Comment 3

[Ting-Yu Lin [:TYLin] (UTC-7)]

Comment on attachment 8568926 [details] [diff] [review]
Remove GetRangeCount() in Selection.h

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

Oops ... uint32_t vs int32_t
https://treeherder.mozilla.org/#/jobs?repo=try&revision=605873342b21

Comment 4

[Ting-Yu Lin [:TYLin] (UTC-7)]

Attachment: Remove GetRangeCount() in Selection.h. (v2)

Fix build error.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d6ae2227e2e

Comment 5

[:ehsan akhgari]

Comment on attachment 8569204 [details] [diff] [review]
Remove GetRangeCount() in Selection.h. (v2)

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

Nice!

Comment 6

[Ting-Yu Lin [:TYLin] (UTC-7)]

Comment on attachment 8569204 [details] [diff] [review]
Remove GetRangeCount() in Selection.h. (v2)

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

::: accessible/generic/HyperTextAccessible.cpp
@@ +1501,5 @@
>    dom::Selection* domSel = DOMSelection();
>    if (!domSel)
>      return false;
>  
> +  if (aSelectionNum < 0 || aSelectionNum >= static_cast<int>(domSel->RangeCount()))

It is better to cast the RangeCount() to int32_t which is the same type as aSelectionNum.

Comment 7

[Ting-Yu Lin [:TYLin] (UTC-7)]

Attachment: Remove GetRangeCount() in Selection.h. r=ehsan (v3, carry r+)

Addressed comment 6.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7820d25d908f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ee5b5c3aac

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9ee5b5c3aac