825499 - Add caretPositionFromPoint to Document.webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ms2ger, thanks for catching this!

Scott, are you planning to patch, or should I do it?

Comment 2

[Scott Johnson (:jwir3)]

Sure, I can do it.

Comment 3

[Scott Johnson (:jwir3)]

Attachment: b825499

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 697502 [details] [diff] [review]
b825499

>+  *aCaretPos = nullptr;
>+  nsRefPtr<nsDOMCaretPosition> cp = nsIDocument::CaretPositionFromPoint(aX, aY, rv);
>+  *aCaretPos = cp.get();

  *aCaretPos = nsIDocument::CaretPositionFromPoint(aX, aY, rv).get();

(note that this is not equivalent to what you had; the refcounting is different).

Is the change to throw if !ps or !ptFrame (instead of returning null) purposeful?

Comment 5

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 697502 [details] [diff] [review]
> b825499
> 
> >+  *aCaretPos = nullptr;
> >+  nsRefPtr<nsDOMCaretPosition> cp = nsIDocument::CaretPositionFromPoint(aX, aY, rv);
> >+  *aCaretPos = cp.get();
> 
>   *aCaretPos = nsIDocument::CaretPositionFromPoint(aX, aY, rv).get();
> 
> (note that this is not equivalent to what you had; the refcounting is
> different).

Ok, I'll fix that.

> Is the change to throw if !ps or !ptFrame (instead of returning null)
> purposeful?

It was intentional. But, after thinking about it some more, I think perhaps the way I had it originally was a bit better. I think I'm going to change this back, since it's perfectly reasonable to simply return null if we're in a strange state like this... it doesn't seem to be an exception IMO.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 697502 [details] [diff] [review]
b825499

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

::: content/base/src/nsDocument.cpp
@@ +8846,5 @@
>    nsIFrame *ptFrame = nsLayoutUtils::GetFrameForPoint(rootFrame, pt, true,
>                                                        false);
>    if (!ptFrame) {
> +    rv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;

Isn't this what you'd hit for """If either argument is negative, x is greater than the viewport width excluding the size of a rendered scroll bar (if any), y is greather than the viewport height excluding the size of a rendered scroll bar (if any) return null."""?

@@ +8885,5 @@
> +  NS_ENSURE_ARG_POINTER(aCaretPos);
> +  ErrorResult rv;
> +  *aCaretPos = nullptr;
> +  nsRefPtr<nsDOMCaretPosition> cp = nsIDocument::CaretPositionFromPoint(aX, aY, rv);
> +  *aCaretPos = cp.get();

What bz said.

::: dom/webidl/Document.webidl
@@ +15,5 @@
>   * http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIDOMDocument.idl
>   */
>  
>  interface Attr;
> +interface CaretPosition;

I would expect this to break. Anyway, remove it.

Comment 7

[Scott Johnson (:jwir3)]

Attachment: b825499 (v2)

Second revision of the patch.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 697594 [details] [diff] [review]
b825499 (v2)

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

r=me

Comment 9

[Scott Johnson (:jwir3)]

Pushed to try: 
https://tbpl.mozilla.org/?tree=Try&rev=3788c55300e2

Comment 10

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ec18652409

Comment 11

[Ed Morley [:emorley]]

(Missed the merge)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30ec18652409