Webidl bindings for Touch

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: Ms2ger)

Tracking

unspecified
mozilla23
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 732216 [details] [diff] [review]
Move Touch to its own file
Attachment #732216 - Flags: review?(Ms2ger)
(Assignee)

Comment 2

5 years ago
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
Attachment #732216 - Flags: review?(Ms2ger) → review+
(Reporter)

Comment 3

5 years ago
Created attachment 732682 [details] [diff] [review]
WebIDL Touch
Attachment #732682 - Flags: review?(Ms2ger)
(Assignee)

Comment 4

5 years ago
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.
Attachment #732682 - Flags: review?(Ms2ger)
(Reporter)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f134e2c4bb2a
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/f134e2c4bb2a
(Assignee)

Comment 7

5 years ago
David, is the second patch blocked on something here?
(Reporter)

Comment 8

5 years ago
It is failing tests and I haven't had a chance to debug it yet.
(Assignee)

Comment 9

5 years ago
Created attachment 737263 [details] [diff] [review]
WebIDL Touch v2

This seems to be happier: https://tbpl.mozilla.org/?tree=Try&rev=9268caea9dcf
Attachment #732682 - Attachment is obsolete: true
Attachment #737263 - Flags: review?(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?
Attachment #737263 - Flags: review?(mounir)
(Reporter)

Comment 11

5 years ago
Ms2ger, do you want to finish this or should I pick it up again?
Flags: needinfo?(Ms2ger)
(Assignee)

Comment 12

5 years ago
Created attachment 739446 [details] [diff] [review]
WebIDL Touch v2.1

Yeah, I guess I will, if that's fine with you.
Assignee: dzbarsky → Ms2ger
Attachment #737263 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #739446 - Flags: review?(mounir)
Flags: needinfo?(Ms2ger)
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.
Attachment #739446 - Flags: review?(mounir) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/047132a1517f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.