856962 - Webidl bindings for Touch

Status: RESOLVED FIXED

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

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Move Touch to its own file

Comment 2

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

Comment on attachment 732216 [details] [diff] [review]
Move Touch to its own file

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +849,5 @@
>    }
>    event.touches.SetCapacity(aCount);
>    for (uint32_t i = 0; i < aCount; ++i) {
>      nsIntPoint pt = ToWidgetPoint(aXs[i], aYs[i], offset, presContext);
> +    nsCOMPtr<nsIDOMTouch> t(new dom::Touch(aIdentifiers[i],

Add a using namespace mozilla::dom and remove the dom::

::: widget/xpwidgets/InputData.cpp
@@ +41,5 @@
>        break;
>    }
>  
>    for (size_t i = 0; i < aTouchEvent.touches.Length(); i++) {
> +    dom::Touch* domTouch = (dom::Touch*)(aTouchEvent.touches[i].get());

static_cast, while you're here

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: WebIDL Touch

Comment 4

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

Comment on attachment 732682 [details] [diff] [review]
WebIDL Touch

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

::: content/events/src/Touch.cpp
@@ +47,4 @@
>    if (content && content->ChromeOnlyAccess() &&
>        !nsContentUtils::CanAccessNativeAnon()) {
>      content = content->FindFirstNonChromeOnlyAccessContent();
> +    target = do_QueryInterface(content);

You should be able to 'return content.forget();' here.

@@ +48,5 @@
>        !nsContentUtils::CanAccessNativeAnon()) {
>      content = content->FindFirstNonChromeOnlyAccessContent();
> +    target = do_QueryInterface(content);
> +  } else {
> +    target = do_QueryInterface(mTarget);

You should try to make mTarget a dom::EventTarget, then this could return a raw pointer.

::: content/events/src/Touch.h
@@ +16,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +class Touch MOZ_FINAL : public nsWrapperCache,
> +                        public nsIDOMTouch

Per <https://vargajan.wordpress.com/2013/04/02/moving-stuff-to-new-dom-bindings/>, this will leak.

::: dom/webidl/Touch.webidl
@@ +11,5 @@
> + */
> +
> +interface Touch {
> +  readonly    attribute long        identifier;
> +  readonly    attribute EventTarget target;

Are you sure this won't return null? Some assertions would be good.

Comment 5

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f134e2c4bb2a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f134e2c4bb2a

Comment 7

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

David, is the second patch blocked on something here?

Comment 8

[David Zbarsky (:dzbarsky)]

It is failing tests and I haven't had a chance to debug it yet.

Comment 9

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

Attachment: WebIDL Touch v2

This seems to be happier: https://tbpl.mozilla.org/?tree=Try&rev=9268caea9dcf

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 737263 [details] [diff] [review]
WebIDL Touch v2

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

What about the PrefEnabled() part? It seems that this patch makes the feature no longer living behind a pref. Is that on purpose?

Comment 11

[David Zbarsky (:dzbarsky)]

Ms2ger, do you want to finish this or should I pick it up again?

Comment 12

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

Attachment: WebIDL Touch v2.1

Yeah, I guess I will, if that's fine with you.

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 739446 [details] [diff] [review]
WebIDL Touch v2.1

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

::: content/events/src/Touch.cpp
@@ +107,3 @@
>    return NS_OK;
>  }
>                                               

nit: could you remove those blank spaces.

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/047132a1517f