Closed Bug 856962 Opened 7 years ago Closed 7 years ago

Webidl bindings for Touch

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dzbarsky, Assigned: Ms2ger)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attachment #732216 - Flags: review?(Ms2ger)
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+
Attached patch WebIDL Touch (obsolete) — Splinter Review
Attachment #732682 - Flags: review?(Ms2ger)
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)
David, is the second patch blocked on something here?
It is failing tests and I haven't had a chance to debug it yet.
Attached patch WebIDL Touch v2 (obsolete) — Splinter Review
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)
Ms2ger, do you want to finish this or should I pick it up again?
Flags: needinfo?(Ms2ger)
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+
https://hg.mozilla.org/mozilla-central/rev/047132a1517f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.