Closed Bug 1136065 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(1 file, 2 obsolete files)

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
Also, the WebIDL bindings call RangeCount(), if I'm not mistaken.
We replace GetRangeCount() with the identical function RangeCount() to
avoid any confusion.
Attachment #8568926 - Flags: review?(ehsan)
Assignee: nobody → tlin
Status: NEW → ASSIGNED
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
Attachment #8568926 - Flags: review?(ehsan)
Comment on attachment 8569204 [details] [diff] [review]
Remove GetRangeCount() in Selection.h. (v2)

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

Nice!
Attachment #8569204 - Flags: review?(ehsan) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d9ee5b5c3aac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.