Implement DOMQuad of geometry interfaces.

Implement DOMQuad of geometry interfaces.

The DOMQuad can represent a quadrilateral.

The specification:
http://dev.w3.org/fxtf/geometry/

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V_bJNtOg0oM

BUG=388780

[zino, 2014-07-25 08:28:30]

Please take a look.

[Rik, 2014-07-26 05:50:51]

lgtm

[eseidel, 2014-07-26 17:06:28]

i don't see how this can work.

How does:

var p = quad.p1;
var bounds = qaud.bounds();
p.x = 10;
console.assert(bounds != quad.bounds())

It looks like currently that will fail.

https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad.h
File Source/core/dom/DOMQuad.h (right):

https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad....
Source/core/dom/DOMQuad.h:22: DOMPoint* p1() const { m_needsRecalcBounds = true;
return m_p1.get(); }
Why do these set needsRecalcBounds when accessing p1?

p1 hasn't changed yet...

[eseidel, 2014-07-26 17:07:04]

not lgtm based on above.

[zino, 2014-07-26 20:55:49]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2014-07-26 20:56:15]

The CQ bit was unchecked by jinho.bang@samsung.com

[haraken, 2014-07-27 06:58:37]

bashi@: FYI, this is another client of Dictionary.

[Rik, 2014-07-28 02:49:54]

On 2014/07/26 17:06:28, eseidel wrote:
> i don't see how this can work.
> 
> How does:
> 
> var p = quad.p1;
> var bounds = qaud.bounds();
> p.x = 10;
> console.assert(bounds != quad.bounds())
> 
> It looks like currently that will fail.

you're correct.  'bounds' must point to a live object that changes as the
attributes in DOMQuad change.

> https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad.h
> File Source/core/dom/DOMQuad.h (right):
> 
>
https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad....
> Source/core/dom/DOMQuad.h:22: DOMPoint* p1() const { m_needsRecalcBounds =
true;
> return m_p1.get(); }
> Why do these set needsRecalcBounds when accessing p1?

The code should be restructured so that width/height of DOMQuad always do the
calculation on the DOMQuad.

[zino, 2014-07-28 11:40:45]

https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad.h
File Source/core/dom/DOMQuad.h (right):

https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad....
Source/core/dom/DOMQuad.h:22: DOMPoint* p1() const { m_needsRecalcBounds = true;
return m_p1.get(); }
On 2014/07/26 17:06:28, eseidel wrote:
> Why do these set needsRecalcBounds when accessing p1?
> 
> p1 hasn't changed yet...

Done.

[zino, 2014-07-28 11:56:44]

On 2014/07/26 17:06:28, eseidel wrote:
> i don't see how this can work.
> 
> How does:
> 
> var p = quad.p1;
> var bounds = qaud.bounds();
> p.x = 10;
> console.assert(bounds != quad.bounds())
> 
> It looks like currently that will fail.

I'm not sure but the |bounds| property is [Same Object].
So, it should be console.assert(bounds == quad.bounds).

> 
> https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad.h
> File Source/core/dom/DOMQuad.h (right):
> 
>
https://codereview.chromium.org/412373003/diff/20001/Source/core/dom/DOMQuad....
> Source/core/dom/DOMQuad.h:22: DOMPoint* p1() const { m_needsRecalcBounds =
true;
> return m_p1.get(); }
> Why do these set needsRecalcBounds when accessing p1?
> 
> p1 hasn't changed yet...

I fixed it.

Could you please review again?

[Rik, 2014-07-28 17:01:35]

On 2014/07/28 11:56:44, zino wrote:
> 
> I fixed it.
> 
> Could you please review again?

not lgtm

The author is not required to call DOMQuad::bounds() to update the bounds. The
code only recalculates when you call this function which is incorrect.
for example:
var q = new DOMQuad();
...
var b = q.bounds;
q.p1.x = 1000; // <- b is updated automatically

[zino, 2014-07-29 00:54:49]

On 2014/07/28 17:01:35, Rik wrote:
> On 2014/07/28 11:56:44, zino wrote:
> > 
> > I fixed it.
> > 
> > Could you please review again?
> 
> not lgtm
> 
> The author is not required to call DOMQuad::bounds() to update the bounds. The
> code only recalculates when you call this function which is incorrect.
> for example:
> var q = new DOMQuad();
> ...
> var b = q.bounds;
> q.p1.x = 1000; // <- b is updated automatically

Oh.. good point.
I addressed it and added the test case to layout tests.
Please take a look.

Thank you.

[Rik, 2014-07-29 03:31:27]

On 2014/07/29 00:54:49, zino wrote:
> 
> Oh.. good point.
> I addressed it and added the test case to layout tests.
> Please take a look.
> 
> Thank you.

Behavior is better, but the code needs to be refactored. See comments.
not lgtm

[Rik, 2014-07-29 03:31:40]

https://codereview.chromium.org/412373003/diff/70001/Source/core/dom/DOMPoint.h
File Source/core/dom/DOMPoint.h (right):

https://codereview.chromium.org/412373003/diff/70001/Source/core/dom/DOMPoint...
Source/core/dom/DOMPoint.h:18: virtual void setX(double x) { m_x = x; }
If you're adding virtual methods, shouldn't the destructor be virtual as well?

https://codereview.chromium.org/412373003/diff/70001/Source/core/dom/DOMQuad.h
File Source/core/dom/DOMQuad.h (right):

https://codereview.chromium.org/412373003/diff/70001/Source/core/dom/DOMQuad....
Source/core/dom/DOMQuad.h:59: Member<DOMRect> m_bounds;
I think this is wrong. m_bounds' lifetime should not be tied to the DOMQuad.
This needs to be a reference to a real JS object.

[zino, 2014-07-29 04:53:33]

On 2014/07/29 03:31:40, Rik wrote:
> Source/core/dom/DOMQuad.h:59: Member<DOMRect> m_bounds;
> I think this is wrong. m_bounds' lifetime should not be tied to the DOMQuad.
> This needs to be a reference to a real JS object.

Dear Rik,

Sorry, I'm not sure I follow you.
Could you explain in a bit more detail?
(If it is possible, please share some examples.)

[zino, 2014-07-29 06:08:11]

On 2014/07/29 04:53:33, zino wrote:
> On 2014/07/29 03:31:40, Rik wrote:
> > Source/core/dom/DOMQuad.h:59: Member<DOMRect> m_bounds;
> > I think this is wrong. m_bounds' lifetime should not be tied to the DOMQuad.
> > This needs to be a reference to a real JS object.
> 
> Dear Rik,
> 
> Sorry, I'm not sure I follow you.
> Could you explain in a bit more detail?
> (If it is possible, please share some examples.)

Do you mean we should create |bound| object(DOMRectReadOnly) lazily?
(if the object is actually referenced, then we should create it for the first
time.)

If so, I've uploaded a new patch set 6.

[zino, 2014-07-29 06:08:50]

Sorry, patch set 5.

[Rik, 2014-07-29 06:27:34]

On 2014/07/29 04:53:33, zino wrote:
> On 2014/07/29 03:31:40, Rik wrote:
> > Source/core/dom/DOMQuad.h:59: Member<DOMRect> m_bounds;
> > I think this is wrong. m_bounds' lifetime should not be tied to the DOMQuad.
> > This needs to be a reference to a real JS object.
> 
> Dear Rik,
> 
> Sorry, I'm not sure I follow you.
> Could you explain in a bit more detail?
> (If it is possible, please share some examples.)

I'm not that familiar with how blink wraps JS objects so maybe this is not an
issue.
What does "Member<DOMRect>" do?
Is it possible that DOMQuadPoint and DOMQuad are holding references to each
other which results in a memory leak?

[zino, 2014-07-29 07:15:58]

On 2014/07/29 06:27:34, Rik wrote:
> On 2014/07/29 04:53:33, zino wrote:
> > On 2014/07/29 03:31:40, Rik wrote:
> > > Source/core/dom/DOMQuad.h:59: Member<DOMRect> m_bounds;
> > > I think this is wrong. m_bounds' lifetime should not be tied to the
DOMQuad.
> > > This needs to be a reference to a real JS object.
> > 
> > Dear Rik,
> > 
> > Sorry, I'm not sure I follow you.
> > Could you explain in a bit more detail?
> > (If it is possible, please share some examples.)
> 
> I'm not that familiar with how blink wraps JS objects so maybe this is not an
> issue.
> What does "Member<DOMRect>" do?
> Is it possible that DOMQuadPoint and DOMQuad are holding references to each
> other which results in a memory leak?

I think @haraken is able to answer to you. (He is Oilpan's owner)
http://www.chromium.org/blink/blink-gc#TOC-Introduction

According to oilpan basic rule 4, Member<T> means that..
"T is a data member of another garbage-collected class U, use Member<T> and
trace it in U::trace(Visitor*)"

For example..

// global context
var g_bounds;

function other_context()
{
    // function context
    var quad = new DOMQuad(...);
    g_bounds = quad.bounds;
    quad = null;

    // Perform garbage collection

    // You seem to expect that the |quad| object is destroyed.
    // But I don't think it will be garbage collected because |g_bounds| still
points to its bounds.
}

other_context();

// I think we can still use |g_bounds| here. The anonymous DOMQuad object still
lives.
console.log(g_bounds);

g_bounds = null;

// Perform garbage collection
// The anonymous DOMQuad object will finally be destroyed.

[Rik, 2014-07-29 16:17:13]

On 2014/07/29 07:15:58, zino wrote:
> 
> // Perform garbage collection
> // The anonymous DOMQuad object will finally be destroyed.

I think you're right. The two objects will still point to each other, but the
garbage collector will catch that you can't reach them and destroy them.
Thanks for explaining!

lgtm

[zino, 2014-07-29 22:41:57]

Ping owners.

Please take a look.

[zino, 2014-08-04 08:09:22]

Dear Eric,

Could you review again?
(I addressed your comments.)

Thank you.

[rwlbuis, 2017-09-21 19:40:27]

zino can we close this? This seems to have landed in other patches anyway.