1387143 - Consider removing nsISelection

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[(no longer active)]

See bug 1386411 comment 12.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop using nsISelection in nsISelectionListener

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop using nsISelection in nsIDocumentEncoder

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop using nsISelection in nsISelectionController

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using nsISelection in remaining xpidl

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove JS use of nsISelectionPrivate

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove some unused constants from nsISelectionPrivate and Selection

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Move the TABLESELECTION_* constants from nsISelectionPrivate to a TableSelection enum

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsISelectionPrivate::Get/SetInterlinePosition

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove nsISelectionPrivate::Get/SetAncestorLimiter

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove toString bits from nsISelection and nsISelectionPrivate

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Support non-XPCOM weakreference on Selection

This way we don't have to deal with QI to get a Selection out of a weakref.

mfbt weakrefs don't have a SizeOfOnlyThis.  In any case, the memory used by the
weakref itself is pretty minor...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove nsISelectionPrivate::Add/RemoveSelectionListener

None of the C++ callers of RemoveSelectionListener care about whether the
listener was already-added, and the only JS caller is in a test and knows the
listener was added.  So the behavior change to no-op instead of throwing when
trying to remove a nonexistent listener is OK.  Furthermore, the removal is
null-safe, so there's no point to explicitly failing if null is passed (which
it never is).

Also, no one passes null to AddSelectionListener.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove nsISelectionPrivate::Get/SetCanCacheFrameOffset

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove nsISelectionPrivate::GetCachedFrameOffset

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Remove nsISelectionPrivate::Get/SetSelectionDirection

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove nsISelectionPrivate::GetType

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Remove GetRangesForInterval bits from nsISelectionPrivate

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Remove ScrollIntoView bits from nsISelectionPrivate

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Remove remaining methods from nsISelectionPrivate

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 20. Remove nsISelectionPrivate

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 21. Remove nsISelection getters for anchor and focus points

Instead of copying spec-duplicating comments from nsISelection.idl to
Selection.webidl, this just points the latter to the right spec.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 22. Remove nsISelection isCollapsed/collapsed bits

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 23. Remove nsISelection collapse* methods

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 24. Remove nsISelection::ContainsNode

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 25. Remove nsISelection::DeleteFromDocument

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 26. Remove nsISelection::Modify

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 27. Remove some unused nsISelection methods

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: part 28. Remove nsISelection::AsSelection()

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: part 29. Remove nsISelection

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 8973083 [details] [diff] [review]
part 1.  Stop using nsISelection in nsISelectionListener

>accessible/base/SelectionManager.cpp
>-  if (document) {
>+  if (document){

typo

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 8973094 [details] [diff] [review]
part 12.  Remove nsISelectionPrivate::Add/RemoveSelectionListener

> Selection::AddSelectionListener(nsISelectionListener* aNewListener,
>                                 ErrorResult& aRv)
> {
>+  MOZ_ASSERT(aNewListener);
>   bool result = mSelectionListeners.AppendElement(aNewListener, fallible); // AddRefs
>   if (!result) {
>     aRv.Throw(NS_ERROR_FAILURE);

Since this is ChromeOnly, I assume there are only a handful of internal
consumers and no way for content script to indirectly add listeners.
If so, then does this really need to be fallible?
It seems OK to me to just abort here if we're OOM.

> Selection::RemoveSelectionListener(nsISelectionListener* aListenerToRemove)
> {
>+  mSelectionListeners.RemoveElement(aListenerToRemove); // Releases

Perhaps we should at least warn if the listener wasn't found?

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 8973099 [details] [diff] [review]
part 17.  Remove GetRangesForInterval bits from nsISelectionPrivate

A side note on GetRangesForIntervalArray: Maybe it could be optimized to take advantage of the bits we have on each node whether they are part of a selected range and get the potential ranges from there?  Also, some callers probably don't need this detailed information anyway and/or would be more efficient if they handled each range sequentially rather than demanding the full array upfront (e.g. mozInlineSpellChecker::IsPointInSelection).

Comment 33

[Mats Palmgren (inactive)]

Comment on attachment 8973100 [details] [diff] [review]
part 18.  Remove ScrollIntoView bits from nsISelectionPrivate

>+   * @param aRegion - the region inside the selection to scroll into view
>+   *                  (see selection region constants defined in
>+   *                   nsISelectionController).
>+   * @param aIsSynchronous - when true, scrolls the selection into view
>+   *                         before returning. If false, posts a request which
>+   *                         is processed at some point after the method returns.
>+   * @param aVPercent - how to align the frame vertically.
>+   * @param aHPercent - how to align the frame horizontally.

While we're here... would you mind removing those leading dashes please?
I don't think they will look good when formatted by tools that reads these doc strings.

Comment 34

[Mats Palmgren (inactive)]

Comment on attachment 8973101 [details] [diff] [review]
part 19.  Remove remaining methods from nsISelectionPrivate

>dom/base/Selection.h
>+   * Modifies the cursor Bidi level after a change in keyboard direction
>+   * @param langRTL is PR_TRUE if the new language is right-to-left or
>+   *                PR_FALSE if the new language is left-to-right.
>+   */
>+  nsresult SelectionLanguageChange(bool aLangRTL);

While we're here... would you mind fixing those obsolete PR_TRUE/PR_FALSE please?

Comment 35

[Mats Palmgren (inactive)]

Comment on attachment 8973103 [details] [diff] [review]
part 21.  Remove nsISelection getters for anchor and focus points

>dom/base/nsContentAreaDragDrop.cpp
>+DragDataProducer::GetDraggableSelectionData(Selection* inSelection,
> ...
>-        nsCOMPtr<nsIContent> selStartContent = do_QueryInterface(selectionStart);
>+        nsCOMPtr<nsIContent> selStartContent = nsIContent::FromNodeOrNull(selectionStart);
>         if (selStartContent && selStartContent->HasChildNodes()) {

Unrelated side comment: perhaps we shouldn't require a nsIContent node here?
HasChildNodes() works fine on nsINode and the rest of this function too, AFAICT.
It seems like the current code would fail on a selection:
{#document, 0, #document, 1} with an SVG image as the root element.

Not sure if it's worth bothering with though - definitely not in this bug anyway.

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 8973110 [details] [diff] [review]
part 28.  Remove nsISelection::AsSelection()

>editor/libeditor/TextEditRulesBidi.cpp
>   RefPtr<nsFrameSelection> frameSelection =
>-    aSelection->AsSelection()->GetFrameSelection();
>+    aSelection->GetFrameSelection();

nit: looks like it would fit on one line now

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 8973111 [details] [diff] [review]
part 29.  Remove nsISelection

>layout/forms/nsTextControlFrame.cpp
>-  nsISelection *caretSelection = caret->GetSelection();
>+  Selection *caretSelection = caret->GetSelection();

nit: move the * next to the type name while we're here please

Comment 38

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #31)
> >+  mSelectionListeners.RemoveElement(aListenerToRemove); // Releases
> 
> Perhaps we should at least warn if the listener wasn't found?

And by "warn" I mean NS_ASSERTION, since real warnings are useless.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> typo

Fixed.

> Since this is ChromeOnly, I assume there are only a handful of internal
> consumers and no way for content script to indirectly add listeners.

Hmm.  Definitely not in a lightweight manner.  I think content can maybe add a listener per contenteditable root on a page.  

I'll switch this to infallible for now and if it becomes a problem we can switch back.

> Perhaps we should at least warn if the listener wasn't found?

I can do that.  I'll do a MOZ_ASSERT, in fact.

> A side note on GetRangesForIntervalArray:

Filed bug 1459269.  Not sure whether I'll have a chance to look into it...

> While we're here... would you mind removing those leading dashes please?

Done.

> While we're here... would you mind fixing those obsolete PR_TRUE/PR_FALSE please?

Done.  I had caught some other cases of that, but missed this one; thank you for noticing it!

> Unrelated side comment: perhaps we shouldn't require a nsIContent node here?

Filed bug 1459271.

> nit: looks like it would fit on one line now

Fixed.

> nit: move the * next to the type name while we're here please

Fixed.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd373c71ec9
part 1.  Stop using nsISelection in nsISelectionListener.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec666bcb050b
part 2.  Stop using nsISelection in nsIDocumentEncoder.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42b86ea3a3b
part 3.  Stop using nsISelection in nsISelectionController.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0328bcd0039e
part 4.  Stop using nsISelection in remaining xpidl.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe33adcc3292
part 5.  Remove JS use of nsISelectionPrivate.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd98009e2644
part 6.  Remove some unused constants from nsISelectionPrivate and Selection.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9383d7bde7
part 7.  Move the TABLESELECTION_* constants from nsISelectionPrivate to a TableSelection enum.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f2f9869b1a
part 8.  Remove nsISelectionPrivate::Get/SetInterlinePosition.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a0de789a13
part 9.  Remove nsISelectionPrivate::Get/SetAncestorLimiter.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/560215a5ceda
part 10.  Remove toString bits from nsISelection and nsISelectionPrivate.  r=mats.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd568f984ca5
part 11.  Support non-XPCOM weakreference on Selection.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/70603ce0a11e
part 12.  Remove nsISelectionPrivate::Add/RemoveSelectionListener.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc853cc9de3
part 13.  Remove nsISelectionPrivate::Get/SetCanCacheFrameOffset.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd354b5a591
part 14.  Remove nsISelectionPrivate::GetCachedFrameOffset.  r=mats.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d6054f4b7e9
part 15.  Remove nsISelectionPrivate::Get/SetSelectionDirection.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/db8412ba88f2
part 16.  Remove nsISelectionPrivate::GetType.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/146396f37e7d
part 17.  Remove GetRangesForInterval bits from nsISelectionPrivate.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7dc2628f12
part 18.  Remove ScrollIntoView bits from nsISelectionPrivate.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a80dee62407a
part 19.  Remove remaining methods from nsISelectionPrivate.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc2c97cc372
part 20.  Remove nsISelectionPrivate.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/feaacb78a655
part 21.  Remove nsISelection getters for anchor and focus points.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/13aee20c6c4b
part 22.  Remove nsISelection isCollapsed/collapsed bits.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2fad352e01
part 23.  Remove nsISelection collapse* methods.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/1714b3b0dec5
part 24.  Remove nsISelection::ContainsNode.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/010c98256bf9
part 25.  Remove nsISelection::DeleteFromDocument.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d48d54b078
part 26.  Remove nsISelection::Modify.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3936f9ef030
part 27.  Remove some unused nsISelection methods.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e041cfea34
part 28.  Remove nsISelection::AsSelection().  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1082792475e
part 29.  Remove nsISelection.  r=mats

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c40353781d
followup.  Remove assertion that does not seem to hold and causes a CLOSED TREE.  r=bzbarsky

Comment 42

[Boris Zbarsky [:bzbarsky]]

Mats, I removed the assertion because it's being hit from the RemoveSelectionListener(this) call EditorBase::PreDestroy does.  That happens because the document's presshell changes between EditorBase::Init and EditorBase::PreDestroy, so we are removing from a different selection from the one we added to....

Is that expected?  If not, I can file a bug on it.

Comment 43

[Mats Palmgren (inactive)]

There are also non-fatal assertions in Editor leading up to
the one you added.  It looks like a bug to me so please file
a bug for it.

Comment 44

[Mats Palmgren (inactive)]

I'm guessing it's an *existing* problem though...

Comment 45

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4bd373c71ec9
https://hg.mozilla.org/mozilla-central/rev/ec666bcb050b
https://hg.mozilla.org/mozilla-central/rev/a42b86ea3a3b
https://hg.mozilla.org/mozilla-central/rev/0328bcd0039e
https://hg.mozilla.org/mozilla-central/rev/fe33adcc3292
https://hg.mozilla.org/mozilla-central/rev/dd98009e2644
https://hg.mozilla.org/mozilla-central/rev/bb9383d7bde7
https://hg.mozilla.org/mozilla-central/rev/89f2f9869b1a
https://hg.mozilla.org/mozilla-central/rev/26a0de789a13
https://hg.mozilla.org/mozilla-central/rev/560215a5ceda
https://hg.mozilla.org/mozilla-central/rev/dd568f984ca5
https://hg.mozilla.org/mozilla-central/rev/70603ce0a11e
https://hg.mozilla.org/mozilla-central/rev/0cc853cc9de3
https://hg.mozilla.org/mozilla-central/rev/8bd354b5a591
https://hg.mozilla.org/mozilla-central/rev/4d6054f4b7e9
https://hg.mozilla.org/mozilla-central/rev/db8412ba88f2
https://hg.mozilla.org/mozilla-central/rev/146396f37e7d
https://hg.mozilla.org/mozilla-central/rev/aa7dc2628f12
https://hg.mozilla.org/mozilla-central/rev/a80dee62407a
https://hg.mozilla.org/mozilla-central/rev/7fc2c97cc372
https://hg.mozilla.org/mozilla-central/rev/feaacb78a655
https://hg.mozilla.org/mozilla-central/rev/13aee20c6c4b
https://hg.mozilla.org/mozilla-central/rev/cb2fad352e01
https://hg.mozilla.org/mozilla-central/rev/1714b3b0dec5
https://hg.mozilla.org/mozilla-central/rev/010c98256bf9
https://hg.mozilla.org/mozilla-central/rev/51d48d54b078
https://hg.mozilla.org/mozilla-central/rev/d3936f9ef030
https://hg.mozilla.org/mozilla-central/rev/b6e041cfea34
https://hg.mozilla.org/mozilla-central/rev/c1082792475e
https://hg.mozilla.org/mozilla-central/rev/d8c40353781d

Comment 46

[Boris Zbarsky [:bzbarsky]]

> I'm guessing it's an *existing* problem though...

Yes, it is.  I filed bug 1460094 on it.