Consider removing nsISelection

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: bzbarsky)

Tracking

(Blocks 4 bugs)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(29 attachments)

27.68 KB, patch
mats
: review+
Details | Diff | Splinter Review
26.29 KB, patch
mats
: review+
Details | Diff | Splinter Review
51.01 KB, patch
mats
: review+
Details | Diff | Splinter Review
12.04 KB, patch
mats
: review+
Details | Diff | Splinter Review
8.80 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.99 KB, patch
mats
: review+
Details | Diff | Splinter Review
45.88 KB, patch
mats
: review+
Details | Diff | Splinter Review
15.28 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.35 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.34 KB, patch
mats
: review+
Details | Diff | Splinter Review
9.12 KB, patch
mats
: review+
Details | Diff | Splinter Review
13.51 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.67 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.84 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.05 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.05 KB, patch
mats
: review+
Details | Diff | Splinter Review
7.95 KB, patch
mats
: review+
Details | Diff | Splinter Review
9.11 KB, patch
mats
: review+
Details | Diff | Splinter Review
7.91 KB, patch
mats
: review+
Details | Diff | Splinter Review
14.21 KB, patch
mats
: review+
Details | Diff | Splinter Review
23.60 KB, patch
mats
: review+
Details | Diff | Splinter Review
41.55 KB, patch
mats
: review+
Details | Diff | Splinter Review
15.78 KB, patch
mats
: review+
Details | Diff | Splinter Review
16.04 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.42 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.85 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.84 KB, patch
mats
: review+
Details | Diff | Splinter Review
25.73 KB, patch
mats
: review+
Details | Diff | Splinter Review
27.62 KB, patch
mats
: review+
Details | Diff | Splinter Review
See bug 1386411 comment 12.
Summary: Consider removing API changes → Consider removing nsISelection
Priority: -- → P3
Depends on: 1391978
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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...
Attachment #8973093 - Flags: review?(mats)
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.
Attachment #8973094 - Flags: review?(mats)
Instead of copying spec-duplicating comments from nsISelection.idl to
Selection.webidl, this just points the latter to the right spec.
Attachment #8973103 - Flags: review?(mats)
Comment on attachment 8973083 [details] [diff] [review]
part 1.  Stop using nsISelection in nsISelectionListener

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

typo
Attachment #8973083 - Flags: review?(mats) → review+
Attachment #8973084 - Flags: review?(mats) → review+
Attachment #8973085 - Flags: review?(mats) → review+
Attachment #8973086 - Flags: review?(mats) → review+
Attachment #8973087 - Flags: review?(mats) → review+
Attachment #8973088 - Flags: review?(mats) → review+
Attachment #8973089 - Flags: review?(mats) → review+
Attachment #8973090 - Flags: review?(mats) → review+
Attachment #8973091 - Flags: review?(mats) → review+
Attachment #8973092 - Flags: review?(mats) → review+
Attachment #8973093 - Flags: review?(mats) → review+
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?
Attachment #8973094 - Flags: review?(mats) → review+
Attachment #8973095 - Flags: review?(mats) → review+
Attachment #8973096 - Flags: review?(mats) → review+
Attachment #8973097 - Flags: review?(mats) → review+
Attachment #8973098 - Flags: review?(mats) → review+
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).
Attachment #8973099 - Flags: review?(mats) → review+
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.
Attachment #8973100 - Flags: review?(mats) → review+
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?
Attachment #8973101 - Flags: review?(mats) → review+
Attachment #8973102 - Flags: review?(mats) → review+
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.
Attachment #8973103 - Flags: review?(mats) → review+
Attachment #8973104 - Flags: review?(mats) → review+
Attachment #8973105 - Flags: review?(mats) → review+
Attachment #8973106 - Flags: review?(mats) → review+
Attachment #8973107 - Flags: review?(mats) → review+
Attachment #8973108 - Flags: review?(mats) → review+
Attachment #8973109 - Flags: review?(mats) → review+
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
Attachment #8973110 - Flags: review?(mats) → review+
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
Attachment #8973111 - Flags: review?(mats) → review+
(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.
> 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.
Blocks: 1459269, 1459271
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
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
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.
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
I'm guessing it's an *existing* problem though...
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
> I'm guessing it's an *existing* problem though...

Yes, it is.  I filed bug 1460094 on it.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.