828138 - clean up some selection related stuff

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsISelectionPrivate::SetPresShell()

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: outparamdel Selection::GetPresShell()

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: outparamdel Selection::GetPresContext()

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: nsIPresShell::GetCurrentSelection() should return Selection*

Comment 6

[(no longer active)]

Comment on attachment 699552 [details] [diff] [review]
remove nsISelectionPrivate::SetPresShell()

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

Nobody was using this?  Huh!

Comment 7

[(no longer active)]

Comment on attachment 699554 [details] [diff] [review]
outparamdel Selection::GetPresContext()

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

r=me with the nit below.

::: layout/generic/nsSelection.cpp
@@ +5065,1 @@
>    nsIPresShell *shell = mFrameSelection->GetShell();

You could simplify this even further by doing this instead of all of the above:

nsIPresShell *shell = GetPresShell();

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 699555 [details] [diff] [review]
nsIPresShell::GetCurrentSelection() should return Selection*

> 
>-  virtual nsISelection* GetCurrentSelection(SelectionType aType) = 0;
>+  virtual mozilla::Selection* GetCurrentSelection(SelectionType aType) = 0;

You're changing an interface, so update IID.
I hope no binary addons are using this stuff.



>+  int32_t rangeCount = origSelection->GetRangeCount();
>   for (int32_t i = 0; i < rangeCount; ++i) {
>-    nsCOMPtr<nsIDOMRange> range;
>-    origSelection->GetRangeAt(i, getter_AddRefs(range));
>-    if (range) {
>-      CloneRangeToSelection(range, aDoc, selection);
>-    }
>+      CloneRangeToSelection(origSelection->GetRangeAt(i), aDoc, selection);
So GetRangeAt(i) can't return null? I guess not

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 699555 [details] [diff] [review]
> nsIPresShell::GetCurrentSelection() should return Selection*
> 
> > 
> >-  virtual nsISelection* GetCurrentSelection(SelectionType aType) = 0;
> >+  virtual mozilla::Selection* GetCurrentSelection(SelectionType aType) = 0;
> 
> You're changing an interface, so update IID.
> I hope no binary addons are using this stuff.

sure, though I think this might actually be binary compatable.

> >+  int32_t rangeCount = origSelection->GetRangeCount();
> >   for (int32_t i = 0; i < rangeCount; ++i) {
> >-    nsCOMPtr<nsIDOMRange> range;
> >-    origSelection->GetRangeAt(i, getter_AddRefs(range));
> >-    if (range) {
> >-      CloneRangeToSelection(range, aDoc, selection);
> >-    }
> >+      CloneRangeToSelection(origSelection->GetRangeAt(i), aDoc, selection);
> So GetRangeAt(i) can't return null? I guess not

not unless we put null ranges into a selection.  Of course it will return null if the index is < 0 or >= # of ranges, but we know that isn't the case here.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Please check whether it is possible to have null in range array.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Please check whether it is possible to have null in range array.

it seems like everyone null checks before adding to the array.

Comment 12

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd618dbf069
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8edf6ffbf8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c112306e4b45
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5bf5d1a528d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5bf5d1a528d
https://hg.mozilla.org/mozilla-central/rev/c112306e4b45
https://hg.mozilla.org/mozilla-central/rev/c8edf6ffbf8a
https://hg.mozilla.org/mozilla-central/rev/fbd618dbf069