Last Comment Bug 850805 - Implement DOMPoint (aka WebKitPoint)
: Implement DOMPoint (aka WebKitPoint)
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla31
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 850806 850808 854490 917755
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-13 12:24 PDT by Lawrence Mandel [:lmandel] (use needinfo)
Modified: 2014-04-22 05:09 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implements WebKitPoint (5.92 KB, patch)
2013-03-14 03:08 PDT, Jet Villegas (:jet)
bzbarsky: feedback+
Details | Diff | Splinter Review
WIP patch implements DOMPoint. Still need to change this to a WebIDL dictionary (or have it return one.) (6.06 KB, patch)
2013-03-19 04:21 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
Proposed WebIDL for DOMPoint (434 bytes, text/plain)
2013-05-01 21:54 PDT, Jet Villegas (:jet)
no flags Details
Adds DOMPoint.webidl to Firefox (595 bytes, patch)
2013-05-01 21:58 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review

Description Lawrence Mandel [:lmandel] (use needinfo) 2013-03-13 12:24:47 PDT
WebKitPoint is a common feature in use on the mobile Web that provides functionality that is useful. We should add this feature to Gecko...albeit with a more generic/standard name.

Class reference: http://developer.apple.com/library/safari/#documentation/AudioVideo/Reference/WebKitPointClassReference/WebKitPoint/WebKitPoint.html
Comment 1 Jet Villegas (:jet) 2013-03-14 03:08:32 PDT
Created attachment 724846 [details] [diff] [review]
Implements WebKitPoint
Comment 2 Jet Villegas (:jet) 2013-03-14 03:12:48 PDT
Hacked this up as discussed in this www-style thread:
http://lists.w3.org/Archives/Public/public-pointer-events/2013JanMar/0099.html

I'm using the WebKitPoint name for now (not DOMPoint or CSSPoint yet) so I can test on existing sites. 

Need feedback on the general approach here, especially around the DOM bindings macros.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2013-03-14 03:26:03 PDT
Comment on attachment 724846 [details] [diff] [review]
Implements WebKitPoint

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

::: dom/bindings/Bindings.conf
@@ +973,5 @@
>     'wrapperCache': False
>  },
>  
> +'WebKitPoint': {
> +    'nativeOwnership': 'refcounted',

Dunno if you actually need to refcount this; you could probably make it 'owned'.

@@ +974,5 @@
>  },
>  
> +'WebKitPoint': {
> +    'nativeOwnership': 'refcounted',
> +    'nativeType': 'mozilla::dom::WebKitPoint',

That's the default

::: dom/webidl/WebKitPoint.webidl
@@ +9,5 @@
> +interface WebKitPoint {
> +
> +  attribute float x;
> +  attribute float y;
> +  

Trailing whitespace

::: layout/style/WebKitPoint.cpp
@@ +7,5 @@
> +#include "WebKitPoint.h"
> +#include "nsContentUtils.h"
> +#include "mozilla/dom/WebKitPointBinding.h"
> +
> +using namespace mozilla::dom;

wrap the file in namespace {}

::: layout/style/WebKitPoint.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* DOM object representing color values in DOM computed style */

Oh really?

@@ +14,5 @@
> +
> +// 3900c55f-5a66-42e7-a8e2-0a6dca9b9dae
> +#define MOZILLA_WEBKITPOINT_IID \
> +{ 0x3900c55f, 0x5a66, 0x42e7, \
> +  { 0xa8, 0xe2, 0x0a, 0x6d, 0xca, 0x9b, 0x9d, 0xae } }

Why?

@@ +33,5 @@
> +  Constructor(const GlobalObject& aGlobal, float aX, float aY, ErrorResult& aError);
> +
> +  WebKitPoint(float aX, float aY);
> +
> +  virtual ~WebKitPoint(void);

No need for the void, or the virtual.

@@ +65,5 @@
> +  }
> +
> +  nsISupports* GetParentObject() const
> +  {
> +    return nullptr;

You need to return something here; the window from the constructor, say.
Comment 4 Boris Zbarsky [:bz] 2013-03-14 11:08:44 PDT
Comment on attachment 724846 [details] [diff] [review]
Implements WebKitPoint

If we don't expect to be handing out WebKitPoint objects from various places, then Ms2ger is right: we could just nuke the wrapper cache, and make them non-wrappercached and owned.

In fact, I agree with most of his nits, except returning null from GetParentObject() should in fact be OK here, I think.

I don't see why you need to check for a Window in the constructor.  Doesn't seem like you actually care whether your global is a Window or not.

You should probably just export the header to mozilla/dom and then you don't need the headerFile annotation in Bindings.conf either.

Apart from that, looks good.
Comment 5 :Ehsan Akhgari 2013-03-14 11:13:00 PDT
Were you also planning to implement Node.convertPointFromNode?

Also, I don't think it's a great idea to name the C++ class WebKitPoint.  DOMPoint seems like a much better name!
Comment 6 Jet Villegas (:jet) 2013-03-14 13:50:47 PDT
(In reply to :Ehsan Akhgari from comment #5)
> Were you also planning to implement Node.convertPointFromNode?

WebKitPoint isn't too useful without the coord-space converters, so Yes, I think we need those (see dependent bugs.)

> Also, I don't think it's a great idea to name the C++ class WebKitPoint. 
> DOMPoint seems like a much better name!

Agreed, mozilla::dom::DOMPoint sounds like a plan.
Comment 7 Jet Villegas (:jet) 2013-03-16 15:46:34 PDT
I ran into a pretty big snag when I nuked the wrappercache:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   XUL                           	0x0000000101c80169 ReleaseSliceNow(unsigned int, void*) + 89
1   XUL                           	0x0000000101c8047c XPCIncrementalReleaseRunnable::ReleaseNow(bool) + 156
2   XUL                           	0x0000000101c8056e XPCIncrementalReleaseRunnable::Run() + 30
3   XUL                           	0x00000001022c60b6 nsThread::ProcessNextEvent(bool, bool*) + 742 (nsThread.cpp:627)

I think it's because DOMPoint objects can be handed out from anywhere like so:

var pt = new DOMPoint();

I don't think I can change it to 'owned' because we'll want this to be prefable. My current strategy is to implement it like this binding:

http://dxr.mozilla.org/mozilla-central/content/svg/content/src/nsISVGPoint.h.html
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-03-17 22:10:19 PDT
In public-fx the consensus was to create a Point type that's actually a WebIDL dictionary. This allows authors to pass {x:3, y:2} and things like that.

Maybe we can just use that type here instead of Webkit/DOMPoint? The only thing is that for compatibility with WebKitPoint we should have a constructor, and WebIDL dictionaries don't have constructors. Could we add support (in the spec and our implementation) for [Constructor] on WebIDL dictionaries?
Comment 9 Jet Villegas (:jet) 2013-03-19 04:21:59 PDT
Created attachment 726575 [details] [diff] [review]
WIP patch implements DOMPoint. Still need to change this to a WebIDL dictionary (or have it return one.)
Comment 10 Boris Zbarsky [:bz] 2013-03-19 13:10:09 PDT
We should be able to hook up constructors for dictionaries, yeah.  As far as spec goes, should check with heycam.
Comment 11 Cameron McCormack (:heycam) 2013-03-25 15:37:56 PDT
Yeah I think it should be fine to have constructors on dictionaries.
Comment 13 Masatoshi Kimura [:emk] 2013-04-10 01:56:27 PDT
WebKitPoint should not be exposed on beta/release even if it is prefed off by default. See bug 837211. If we expose it anyway, we should spec it. (Silly name? yes, but navigator.product is already speced which just returns the hard-coded "Gecko").
Comment 14 Jet Villegas (:jet) 2013-05-01 21:54:02 PDT
Created attachment 744454 [details]
Proposed WebIDL for DOMPoint
Comment 15 Jet Villegas (:jet) 2013-05-01 21:55:29 PDT
Comment on attachment 744454 [details]
Proposed WebIDL for DOMPoint

>/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>/* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> * You can obtain one at http://mozilla.org/MPL/2.0/.
> */
>
>[Constructor(optional float x = 0, optional float y = 0),Pref="layout.css.dompoint.enabled"]
>dictionary DOMPoint {
>  float x = 0;
>  float y = 0;
>};
Comment 16 Jet Villegas (:jet) 2013-05-01 21:58:02 PDT
Created attachment 744455 [details] [diff] [review]
Adds DOMPoint.webidl to Firefox

As soon as bug 854490 lands, this patch will add DOMPoint to Firefox, hidden behind this pref:

layout.css.dompoint.enabled
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-09-16 03:38:57 PDT
Making DOMPoint a dictionary is a problem because it means other objects (like the proposed DOMQuad) can't have DOMPoints as attributes. I have posted to public-fx/www-style about this.
Comment 18 Jet Villegas (:jet) 2013-09-20 13:21:54 PDT
Turning this back into an interface from dictionary should just work without any codegen.py changes.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-04-22 05:03:01 PDT
DOMPoint is implemented now, in bug 917755.

Note You need to log in before you can comment on or make changes to this bug.