bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Make mozilla::Selection easier to obtain/use

NEW
Unassigned

Status

()

Core
Selection
--
enhancement
6 years ago
11 months ago

People

(Reporter: ayg, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

3.54 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.19 KB, patch
mats
: review+
Details | Diff | Splinter Review
10.73 KB, patch
Details | Diff | Splinter Review
47.10 KB, patch
mats
: review-
Details | Diff | Splinter Review
Currently you need a whole rigmarole to wrestle an nsTypedSelection out of an nsISelection.  This could be made a lot easier.
Flags: in-testsuite-
Created attachment 631346 [details] [diff] [review]
Patch part 1, v1 -- Use NS_DECL_NSISELECTIONCONTROLLER

This is just a prefatory cleanup patch.
Attachment #631346 - Flags: review?(matspal)
Created attachment 631347 [details] [diff] [review]
Patch part 2, v1 -- Allow getting an nsTypedSelection from nsISelectionController

(smaug was the one who suggested I ask you to review these, BTW.)
Attachment #631347 - Flags: review?(matspal)
Created attachment 631349 [details] [diff] [review]
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*.
Attachment #631349 - Flags: review?(matspal)
Created attachment 631350 [details] [diff] [review]
Patch part 4, v1 -- Use new nsISelectionController::GetSelection() variant

Yay, minus LOC.  Try: https://tbpl.mozilla.org/?tree=Try&rev=e2557cde97f9
Attachment #631350 - Flags: review?(ehsan)
Depends on: 693933

Updated

6 years ago
Attachment #631350 - Flags: review?(ehsan) → review+
Comment on attachment 631346 [details] [diff] [review]
Patch part 1, v1 -- Use NS_DECL_NSISELECTIONCONTROLLER

Something went wrong when you uploaded this patch?
Attachment #631346 - Flags: review?(matspal) → review-
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>);
Created attachment 631723 [details] [diff] [review]
Patch part 1, v2 -- Use NS_DECL_NSISELECTIONCONTROLLER

Whoops -- this one should work.  Dunno what happened there.
Attachment #631346 - Attachment is obsolete: true
Attachment #631723 - Flags: review?(matspal)
(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.
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).
Attachment #631723 - Flags: review?(matspal) → review+
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;
Attachment #631347 - Flags: review?(matspal) → review-
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.
Attachment #631349 - Flags: review?(matspal)
Created attachment 631846 [details] [diff] [review]
Patch part 2, v2 -- Allow getting an nsTypedSelection from nsISelectionController

Oops, you were right, I left in the [ptr].  This works, thanks!
Attachment #631347 - Attachment is obsolete: true
Attachment #631846 - Flags: review?
Created attachment 631848 [details] [diff] [review]
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
Attachment #631349 - Attachment is obsolete: true
Attachment #631848 - Flags: review?
Summary: Make nsTypedSelection easier to obtain/use → Make mozilla::Selection easier to obtain/use
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.
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 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).
Attachment #631846 - Flags: review?
Attachment #631848 - Flags: review?

Comment 17

6 years ago
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

6 years ago
(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.
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);
  }
}
Depends on: 763860
(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.
Created attachment 632183 [details] [diff] [review]
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
Attachment #631846 - Attachment is obsolete: true
Attachment #632183 - Flags: review?(matspal)
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.)
Attachment #631848 - Flags: review?(matspal)
Mats, do you have time to review this, or should I ask someone else?
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.
Attachment #632183 - Flags: review?(matspal) → review-
Attachment #631848 - Flags: review?(matspal)
(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.
(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.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
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?
Flags: needinfo?(m_kato)

Comment 28

11 months ago
(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.
Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.