762841 - Make mozilla::Selection easier to obtain/use

Status: NEW

(Core :: DOM: Selection, enhancement, P5)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Currently you need a whole rigmarole to wrestle an nsTypedSelection out of an nsISelection.  This could be made a lot easier.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Use NS_DECL_NSISELECTIONCONTROLLER

This is just a prefatory cleanup patch.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Allow getting an nsTypedSelection from nsISelectionController

(smaug was the one who suggested I ask you to review these, BTW.)

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v1 -- Get rid of nsIPresShell::GetCurrentSelection

It does the same thing as nsISelectionController::GetSelection, and its name is longer.  I still have to declare a GetSelection method in nsIPresShell, but now its implementation is the same as nsISelectionController::GetSelection.  The include changes from nsISelection.h -> nsTypedSelection.h are so that the compiler can convert the nsTypedSelection* to an nsISelection*.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v1 -- Use new nsISelectionController::GetSelection() variant

Yay, minus LOC.  Try: https://tbpl.mozilla.org/?tree=Try&rev=e2557cde97f9

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 631346 [details] [diff] [review]
Patch part 1, v1 -- Use NS_DECL_NSISELECTIONCONTROLLER

Something went wrong when you uploaded this patch?

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 631347 [details] [diff] [review]
Patch part 2, v1 -- Allow getting an nsTypedSelection from nsISelectionController

> +[ptr] native nsTypedSelection(nsTypedSelection);

I think we should enforce consumers to use a nsRefPtr.
Does this work:
native nsTypedSelection(already_AddRefed<nsTypedSelection>);

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v2 -- Use NS_DECL_NSISELECTIONCONTROLLER

Whoops -- this one should work.  Dunno what happened there.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #6)
> I think we should enforce consumers to use a nsRefPtr.
> Does this work:
> native nsTypedSelection(already_AddRefed<nsTypedSelection>);

That produces

  virtual already_AddRefed<mozilla::Selection> * GetSelection(PRInt16 type) = 0;

in the header file, which we don't want -- it has an extra *.  So no, that doesn't seem to work.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So part two of this patch series causes a whole bunch of mysterious test failures:

https://tbpl.mozilla.org/?tree=Try&rev=e09da415f774

Any idea why?  They're Windows only -- cf. <https://tbpl.mozilla.org/?tree=Try&rev=e2557cde97f9>.  The failures all seem to have nsISelectionController::GetSelection return null on Windows only, in various tests.  The failing tests all *seem* to be using some type of special powers, not any APIs exposed to webpages, although I didn't look at every one (there are a lot).

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 631347 [details] [diff] [review]
Patch part 2, v1 -- Allow getting an nsTypedSelection from nsISelectionController

> > Does this work:
> > native nsTypedSelection(already_AddRefed<nsTypedSelection>);
> 
> That produces
> 
>   virtual already_AddRefed<mozilla::Selection> * GetSelection(PRInt16 type)
> = 0;

It produces the desired result for me:
  virtual already_AddRefed<nsTypedSelection> GetSelection(PRInt16 type) = 0;

Did you forget to remove the [ptr] perhaps?

Please bump the UUID for nsISelectionController.

Please don't add more use of NS_ENSURE_ macros, this is better:
+  return mSelection ? mSelection->GetSelection(aType) : nsnull;

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 631349 [details] [diff] [review]
Patch part 3, v1 -- Get rid of nsIPresShell::GetCurrentSelection

I'll wait for the already_AddRefed change before reviewing this part.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v2 -- Allow getting an nsTypedSelection from nsISelectionController

Oops, you were right, I left in the [ptr].  This works, thanks!

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v2 -- Get rid of nsIPresShell::GetCurrentSelection

Please be sure to scrutinize my nsRefPtr/forget() use closely here, because I'm not totally confident I'm not leaking/double-freeing/etc.

Assigning already_AddRefed<Selection> to nsCOMPtr<nsISelection> doesn't currently work, so I changed CloneSelection() to use nsRefPtr<Selection> while I was at it.  Of course, then I could use the nicer versions of GetRangeCount() and GetRangeAt(), so why not!

Ehsan, I changed nsEditor::GetSelection to return already_AddRefed<Selection> instead of Selection* so it matches the new patch part 2.  Is that okay with you, or should I use an nsRefPtr so it still returns Selection*?

Try: https://tbpl.mozilla.org/?tree=Try&rev=0c04e5d29cd0

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

My new try run has all mochitests and crashtests orange on Windows.  All mochitests seem to be crashing in the cycle collector, so smaug says that it looks like I messed up AddRef/Release somewhere, which is unsurprising.  But I have no idea where it might be.  Ms2ger also didn't spot it.  So let me know if you can see where it's happening.  Crashtests are also crashing with the same exit code, but in a different place, but other reftests and xpcshell tests seem fine.  All of this is Windows only.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I started two new try runs to narrow down which patch is at fault:

https://tbpl.mozilla.org/?tree=Try&rev=03c5c71d4524 (first two patches only)
https://tbpl.mozilla.org/?tree=Try&rev=16578458920b (first three patches only)

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 631846 [details] [diff] [review]
Patch part 2, v2 -- Allow getting an nsTypedSelection from nsISelectionController

So it turns out that overloading isn't allowed here -- I have to pick a different name for my new GetSelection, since it's in a scriptable interface.  That's what was causing the orange on Windows.  Canceling review pending a new version (although it looks like I didn't set the review flag right to start with).

Comment 17

[(no longer active)]

Can you please file another bug as well to make the idl parser do something more useful than just letting things through silently the next time that someone hits this problem?

Comment 18

[(no longer active)]

(In reply to Aryeh Gregor from comment #13)
> Ehsan, I changed nsEditor::GetSelection to return
> already_AddRefed<Selection> instead of Selection* so it matches the new
> patch part 2.  Is that okay with you, or should I use an nsRefPtr so it
> still returns Selection*?

That's fine by me.

Comment 19

[Mats Palmgren (inactive)]

I think the majority of our internal consumers use SELECTION_NORMAL,
so we could add a convenience method for that.  For example:

  [noscript,notxpcom,nostdcall] Selection GetTypedSelection(in short type);

%{C++
  already_AddRefed<mozilla::Selection> GetNormalSelection() {
    return GetTypedSelection(SELECTION_NORMAL);
  }
}

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Can you please file another bug as well to make the idl parser do something
> more useful than just letting things through silently the next time that
> someone hits this problem?

Bug 763860.

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v3 -- Allow getting an nsTypedSelection from nsISelectionController

This fixes the problem by renaming the existing GetSelection to ScriptableGetSelection.  I preferred this to picking a different name for the new method, because GetSelection is the nicer name, and callers should really be using the new method -- it doesn't return an nsresult or use an out param, and Selection is *much* nicer to use from C++ than nsISelection.  So I wanted the shorter and more natural name for the method we should actually be using.  Hopefully the "Scriptable" will deter people from using the old method going forward.

I initially tried to switch all callers to use the new method, but it required too many logic changes.  For one thing,
  nsCOMPtr<nsISelection> sel = selCon->GetSelection(...);
doesn't work, because the conversion from already_AddRefed<Selection> to nsCOMPtr<nsISelection> doesn't happen for some reason (Ms2ger seemed to think this was an nsCOMPtr bug).  So I had to do
  nsRefPtr<Selection> sel = selCon->GetSelection(...);
but then this required changes to headers, elimination of QI to nsISelectionPrivate, etc.  So it was more than I wanted to try doing in one patch.  I just switched them all to ScriptableGetSelection instead.

Try run for the new patch series: https://tbpl.mozilla.org/?tree=Try&rev=62378c017731

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 631848 [details] [diff] [review]
Patch part 3, v2 -- Get rid of nsIPresShell::GetCurrentSelection

(This patch still applies fine, with some context changes.)

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Mats, do you have time to review this, or should I ask someone else?

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 632183 [details] [diff] [review]
Patch part 2, v3 -- Allow getting an nsTypedSelection from nsISelectionController

r- because this breaks source compatibility for binary extensions using
nsISelectionController::GetSelection.  I think you need approval from
bz/bsmedberg/jst for such a breaking API change.

The code simplification in part 4 is relatively minor and although the
new GetSelection method is an improvement for writing new code, this
patch also makes existing code uglier and it seems from comment 21 that
updating it to use the new method might have some issues, so I'm not
convinced the current changes are an improvement as is.  I'd like to see
a patch that updates our code to use the new GetSelection method.

(In reply to Aryeh Gregor from comment #21)
> conversion from already_AddRefed<Selection> to nsCOMPtr<nsISelection>
> doesn't happen for some reason (Ms2ger seemed to
> think this was an nsCOMPtr bug)

Please file a bug on XPCOM to get that sorted out.

Also, I'd like to see the outcome of the discussion on overloading in
bug 763860 first.

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #24)
> r- because this breaks source compatibility for binary extensions using
> nsISelectionController::GetSelection.  I think you need approval from
> bz/bsmedberg/jst for such a breaking API change.

What APIs does this apply to?

> The code simplification in part 4 is relatively minor

Given an nsISelectionController, it reduces the code to get a Selection from

  nsCOMPtr<nsISelection> selection;
  rv = selCon->GetSelection(nsISelectionController::SELECTION_NORMAL,
                            getter_AddRefs(selection));
  NS_ENSURE_SUCCESS(rv, rv);
  NS_ENSURE_TRUE(selection, NS_ERROR_FAILURE);
  nsCOMPtr<nsISelectionPrivate> selPriv = do_QueryInterface(selection);
  NS_ENSURE_TRUE(selPriv, NS_ERROR_FAILURE);
  nsRefPtr<nsFrameSelection> frameSel;
  rv = selPriv->GetFrameSelection(getter_AddRefs(frameSel));
  NS_ENSURE_SUCCESS(rv, rv);
  NS_ENSURE_TRUE(frameSel, NS_ERROR_FAILURE);
  nsRefPtr<Selection> typedSel =
    frameSel->GetSelection(nsISelectionController::SELECTION_NORMAL);
  NS_ENSURE_TRUE(typedSel, NS_ERROR_FAILURE);

to

  nsRefPtr<Selection> selection =
    selCon->GetSelection(nsISelectionController::SELECTION_NORMAL);
  NS_ENSURE_TRUE(selection, NS_ERROR_FAILURE);

I think that's a *big* savings.  The specific patch is minor because there were only two call sites, but ideally we'd want everywhere to use the actual Selection instead of nsISelection, for the same reason we want to de-COMtaminate the whole codebase.  There's no reason any method for internal use should return an nsISelection.

> and although the
> new GetSelection method is an improvement for writing new code, this
> patch also makes existing code uglier and it seems from comment 21 that
> updating it to use the new method might have some issues, so I'm not
> convinced the current changes are an improvement as is.  I'd like to see
> a patch that updates our code to use the new GetSelection method.

I can do that.  I'll make it a separate patch.

> (In reply to Aryeh Gregor from comment #21)
> > conversion from already_AddRefed<Selection> to nsCOMPtr<nsISelection>
> > doesn't happen for some reason (Ms2ger seemed to
> > think this was an nsCOMPtr bug)
> 
> Please file a bug on XPCOM to get that sorted out.

Bug 765662.

Comment 26

[Mats Palmgren (inactive)]

(In reply to Aryeh Gregor from comment #25)
> (In reply to Mats Palmgren [:mats] from comment #24)
> > r- because this breaks source compatibility for binary extensions using
> > nsISelectionController::GetSelection.  I think you need approval from
> > bz/bsmedberg/jst for such a breaking API change.
> 
> What APIs does this apply to?

The nsISelectionController XPCOM API.  A binary extension using this API
will fail to compile and require changing its calls to GetSelection
to ScriptableGetSelection.  I don't think the saved lines of code for us
is worth this breakage for extensions.

Hopefully we can fix that though - I have attached a WIP patch in bug
763860 that may allow us to overload GetSelection so we can maintain
source compatibility.

Comment 27

[Nathan Froyd [:froydnj]]

Since we no longer have to maintain binary compatibility, could we see about reviving these patches?  It's not clear to me whether this overlaps with your work in bug 1391978, but it certain seems like it's in the same general area?

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Nathan Froyd [:froydnj] from comment #27)
> Since we no longer have to maintain binary compatibility, could we see about
> reviving these patches?  It's not clear to me whether this overlaps with
> your work in bug 1391978, but it certain seems like it's in the same general
> area?

We already have nsISelectionContoller::GetDOMSelection to get mozilla::dom::Selection easily (bug 1386411).  So most issue of this bug is already out of date.

We need another work that it can replace nsISelection / nsISelectionPrivate with mozilla::dom::Selection to reduce XPCOM/virtual call.  Bug 1391978 is for editor are only because I want to replace old nsIDOMNode usages with nsINode.

So we can close this bug, or change to general issue to replace nsISelection with Selection.

Comment 29

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.