Closed Bug 949445 Opened 6 years ago Closed 6 years ago

Convert Selection to WebIDL bindings

Categories

(Core :: DOM: Selection, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files)

Most of this API is spec'ed here <https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#concept-selection>, except for modify, which is a cross-browser API, and selectionLanguageChange which is an internal method which should have never been exposed to the web.  I'm willing to remove the second one as part of this bug.
Keywords: dev-doc-needed
(We should remove the docs for selectionLanguageChange)
Some people who embed Gecko or make addons or SeaMonkey or Thunderbird may use it so removing the doc entirely isn't a good idea. But for sure, we can add scary banners and add a section to transition code using it :-)
(In reply to David Bruant from comment #2)
> Some people who embed Gecko or make addons or SeaMonkey or Thunderbird may
> use it so removing the doc entirely isn't a good idea.

They should not be using that function.  That is intended only for internal use and I'm willing to break anybody who's using it.  :-)

And FWIW, no code in comm-central or our AMO addons are using this function, so removing that method should not affect most people.
Another note to self: sequence<Range> GetRangesForInterval(Node, long, Node, long, boolean, object);
OK, these patches should be ready except for one test failure.  It turns out that moving test_scroll_selection_into_view.html to the mochitest-chrome suite can blow everything out (see this try run for example: <https://tbpl.mozilla.org/?tree=Try&rev=08daedb8e7ea> which is without the rest of my changes here.)

I still don't have any idea what's happening with that test, but I will fix that and submit the fix in another patch (which will probably be squashed with part 3 before landing) but the rest of the patches here should be ready for review.
Attachment #8347481 - Flags: review?(bzbarsky)
Attachment #8347482 - Flags: review?(bzbarsky)
Attachment #8347483 - Flags: review?(bzbarsky)
Attachment #8347484 - Flags: review?(bzbarsky)
Comment on attachment 8347481 [details] [diff] [review]
Part 1: Move selectionLanguageChange to nsISelectionPrivate; r=bzbarsky

r=me
Attachment #8347481 - Flags: review?(bzbarsky) → review+
Comment on attachment 8347482 [details] [diff] [review]
Part 2: Move Selection to WebIDL; r=bzbarsky

Maybe a followup to move Selection to mozilla::dom?

Good catch on HTMLDocument.getSelection() needing to be nullable!

>+++ b/dom/imptests/failures/editing/selecttest/test_interfaces.html.json

This file can just go away altogether, no?  All the failures are fixed.

>+++ b/layout/generic/Selection.h
>      NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(Selection, nsISelectionPrivate)

Since you're wrappercached, you want NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS

>+++ b/layout/generic/nsSelection.cpp

Here you need to NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in the CC traverse bits and add a NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE call.  And in your unlink, NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER.

>Selection::RemoveAllRanges() 
>+  if (result.Failed()) {
>+    return result.ErrorCode();
>+  }
>+  return NS_OK;

That can all just be: return result.ErrorCode(); and same in various other places in this patch.

>+Selection::RemoveAllRanges(ErrorResult& aRv)
>+    return;//nothing to do

Throw in some spaces?  ;)

>+Selection::Extend(nsINode& aParentNode, uint32_t aOffset, ErrorResult& aRv)
>+    res = range->SetEnd(&aParentNode, aOffset);
>+    if (NS_FAILED(res)) {
>+      aRv.Throw(res);
>+      return;
>+    }

That could be:

  range->SetEnd(aParentNode, aOffset, aRv);
  if (aRv.Failed()) {
    return;
  }

and similar for some other operations here.   Maybe a followup?

> Selection::SelectAllChildren(nsIDOMNode* aParentNode)
>   nsCOMPtr<nsINode> node = do_QueryInterface(aParentNode);

This can in fact return null, because nsIDOMNode is not builtinclass (yeah, I know, we should fix that).  Please null-check here.  And might as well drop the useless null-check of aParentNode.

> Selection::ContainsNode(nsIDOMNode* aNode, bool aAllowPartial, bool* aYes)
>+  bool contains = ContainsNode(*node, aAllowPartial, result);
>+  if (result.Failed()) {
>+    return result.ErrorCode();
>+  }
>+  *aYes = contains;
>+  return NS_OK;

I would just do:

  *aYes = ContainsNode(*node, aAllowPartial, result);
  return result.ErrorCode();

But also, there is no spec for this method, and our current impl doesn't throw on null (just returns false).  Probably better to keep that behavior and make the first arg here nullable.

r=me with those fixed, but I'd like to see an interdiff, esp for the CC bits.
Attachment #8347482 - Flags: review?(bzbarsky) → review+
Comment on attachment 8347483 [details] [diff] [review]
Part 3: Expose some of the nsISelectionPrivate methods as ChromeOnly; r=bzbarsky

>@@ -3565,39 +3612,64 @@ Selection::GetRangesForInterval(nsIDOMNode* aBeginNode

Again, the QIs to nsINode here can return null.

>+++ b/layout/generic/test/test_bug348681.html

>+      SpecialPowers.pushPrefEnv({"set": [["dom.testing.selection.GetRangesForInterval", true]]}, doTest);

You want to still do that off the load event, no?  Otherwise doTest can run before we're done loading the page...

r=me with those nits, but it's worth folding this with part 2 so we don't have a broken intermediate state in the tree.
Attachment #8347483 - Flags: review?(bzbarsky) → review+
Comment on attachment 8347484 [details] [diff] [review]
Part 4: Remove the classinfo for Selection; r=bzbarsky

> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Selection)

Oh, this should have been changed in part 2.  You need NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY as the first thing after this line, right?

r=me
Attachment #8347484 - Flags: review?(bzbarsky) → review+
Comment on attachment 8347653 [details] [diff] [review]
Part 3.1: Move the test content into a separate window to prevent the main harness window from being scrolled; r=bzbarsky

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

Turns out that the reason why this test was affecting some of the tests after it was that it calls scrollIntoView which causes the main test harness window to be scrolled so that half of the test iframe would be off-screen, and therefore any future tests which relied on the entire test iframe being visible would fail this way.  The simplest fix is to move the DOM structure of the test into a separate top level window so that scrollIntoView would not affect the main harness window.  I chose to open the test in a 500x350 window which is the same size as the iframe that tests run inside.
Attachment #8347653 - Flags: review?(bzbarsky)
Attached patch Part 2 interdiffSplinter Review
Comment on attachment 8347653 [details] [diff] [review]
Part 3.1: Move the test content into a separate window to prevent the main harness window from being scrolled; r=bzbarsky

r=me
Attachment #8347653 - Flags: review?(bzbarsky) → review+
Comment on attachment 8347657 [details] [diff] [review]
Part 2 interdiff

>+    difRange->SetEnd(*range->GetEndParent(), range->EndOffset(), aRv);
>+    difRange->SetStart(*focusNode, focusOffset, aRv);

Are we guaranteed that GetEndParent() and focusNode are non-null?  Same for the other converted bits where we're ending up dereferencing explicitly.

Other than that, looks good.
(In reply to Boris Zbarsky [:bz] from comment #19)
> Comment on attachment 8347657 [details] [diff] [review]
> Part 2 interdiff
> 
> >+    difRange->SetEnd(*range->GetEndParent(), range->EndOffset(), aRv);
> >+    difRange->SetStart(*focusNode, focusOffset, aRv);
> 
> Are we guaranteed that GetEndParent() and focusNode are non-null?  Same for
> the other converted bits where we're ending up dereferencing explicitly.

Hmm, actually I don't know if we are.  Can I leave that stuff out of the patch please?  I don't know how to prove that these dereferences are correct to be honest.
Flags: needinfo?(bzbarsky)
Absolutely leave it out!  I was only saying to consider converting the the ones where we're passing a reference already, not all of them.  And even then, pushing that off to a followup is fine.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/bd02651de80b
https://hg.mozilla.org/mozilla-central/rev/12aeb83eaee7
https://hg.mozilla.org/mozilla-central/rev/8f9955d5f6b1
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 1025170
You need to log in before you can comment on or make changes to this bug.