Fix svg contentrect location

Fix svg contentrect location

I was not computing entry.contetRect location correctly for <svg> elements.
It should be 0,0

BUG=641364
Committed: https://crrev.com/879fbd0230e2bf90bd19a6b03c22f20fd31f46e4
Cr-Commit-Position: refs/heads/master@{#417450}

[atotic1, 2016-09-01 22:30:51]

Description was changed from

==========
Fix svg contentrect location

BUG=641364
==========

to

==========
Fix svg contentrect location

I was not computing entry.contetRect location correctly for <svg> elements.
It should be 0,0

BUG=641364
==========

[atotic1, 2016-09-01 22:30:51]

atotic@google.com changed reviewers:
+ eae@chromium.org, esprehn@chromium.org

[atotic1, 2016-09-01 22:31:30]

Minor fix for ResizeObserver, computing SVG location

[esprehn, 2016-09-02 00:31:50]

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right):

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.cpp:57: LayoutPoint
ResizeObservation::getTargetLocation(Element* target) // static
static in the file, or anonymous namespace, no need to have it public on
ResizeObservation

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/observer/ResizeObservation.h (right):

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.h:29: static
LayoutSize getTargetSize(Element* target);
ditto to this method, no need for the statics over here

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp (right):

https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:20:
FloatPoint(ResizeObservation::getTargetLocation(m_target)),
just put a static function in this file, or use an anonymous namespace here.

[atotic1, 2016-09-02 18:22:28]

On 2016/09/02 at 00:31:50, esprehn wrote:
>
https://codereview.chromium.org/2305893002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/observer/ResizeObservation.cpp:57: LayoutPoint
ResizeObservation::getTargetLocation(Element* target) // static
> static in the file, or anonymous namespace, no need to have it public on
ResizeObservation
>.......
> third_party/WebKit/Source/core/observer/ResizeObserverEntry.cpp:20:
FloatPoint(ResizeObservation::getTargetLocation(m_target)),
> just put a static function in this file, or use an anonymous namespace here.

getTargetSize cannot be local static. It is being used in 
ResizeObservation::observationSizeOutOfSync and ResizeObserverEntry::constructor

getTargetLocation is only being used by ResizeObserverEntry.

getTargetSize used to be a method on ResizeObservation. When ResizeObserverEntry
needed it, it became a static.

I can make them both static on ResizeObserverEntry. Would that work for you?

--------
I do not like sticking them on ResizeObserverEntry. In my mental model,
ResizeObserverEntry is 'dumb'. All it does it transport data.
ResizeObservation is 'smart'. It knows about sizes, etc.
Moving stuff to Entry kills this mental model, there is no way to 
intuit where function should go.

What I wanted to do is have ResizeObserverEntry::constructor take
ResizeObservation, and get all data from the observation, no statics.
But, ResizeObserverEntry is exposed to JS with IDL, and has a
ResizeObserverEntry(Element*) constructor.
What should I do with that constructor?

I can go further down the rabbit hole here, but I'll stop.
Bottom line is, there is no way to structure this code
to be intuitively readable.

[esprehn, 2016-09-08 19:50:01]

If the function is only used in one place it should be in that .cpp file not
static on the class.

It's strange to me that ResizeObserverEntry has a constructor that takes an
element, that would have to force a layout to compute the rects.

This code looks wrong because it doesn't update layout before using the
layoutBox in that constructor. We either need to make it update layout or change
the JS API. I'd prefer if the JS API took an element and a rect and didn't
compute anything for you. Having this cause a sync layout flush inside every
record constructor seems like bad ergonomics.

[esprehn, 2016-09-08 19:52:41]

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

There's no constructor exposed there, you cB make ResizeObserverEntry take
whatever you want. I'd prefer we made the entry a dumb container of data, it
should have a constructor for (Element&, IntRect) and fill in the fields. All
the computation should happen in the observation.

[atotic1, 2016-09-08 20:48:25]

On 2016/09/08 at 19:50:01, esprehn wrote:
> If the function is only used in one place it should be in that .cpp file not
static on the class.
> 
> It's strange to me that ResizeObserverEntry has a constructor that takes an
element, that would have to force a layout to compute the rects.
> 
> This code looks wrong because it doesn't update layout before using the
layoutBox in that constructor. We either need to make it update layout or change
the JS API. I'd prefer if the JS API took an element and a rect and didn't
compute anything for you. Having this cause a sync layout flush inside every
record constructor seems like bad ergonomics.

I've reworked the code. ResizeObserverEntry no longer computes anything,
ResizeObservation has all the smarts.

This was a good suggestion, there is less code, and it is cleaner.

[atotic1, 2016-09-08 20:49:42]

New code that refactors LayoutRect calculations is in, review at leisure.

[esprehn, 2016-09-08 21:40:29]

lgtm

https://codereview.chromium.org/2305893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right):

https://codereview.chromium.org/2305893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/observer/ResizeObservation.cpp:50: if (doSave) {
I'd split this into two methods. One called updateFoo() and one compute() update
calls compute and then stores the value. The boolean isn't very clear at the
call site what it does or what doSave means.

[atotic1, 2016-09-08 21:47:21]

On 2016/09/08 at 21:40:29, esprehn wrote:
> lgtm
> 
>
https://codereview.chromium.org/2305893002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right):
> 
>
https://codereview.chromium.org/2305893002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/observer/ResizeObservation.cpp:50: if (doSave)
{
> I'd split this into two methods. One called updateFoo() and one compute()
update calls compute and then stores the value. The boolean isn't very clear at
the call site what it does or what doSave means.

That's the way I had it initially. What tipped me over the other way was
that the old way is more brittle, enforcement of "you must call save after
you broadcast compute" is up to the caller.

If ResizeObservation was used anywhere else, I'd prefer splitting them up.

Just my preference. Off to implement the updateFoo().

[atotic1, 2016-09-08 21:58:10]

compute & save split up. Please review.

[atotic1, 2016-09-08 21:58:41]

The CQ bit was checked by atotic@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 21:59:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-09-08 21:59:18]

lgtm w/ nits

https://codereview.chromium.org/2305893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/observer/ResizeObservation.cpp (right):

https://codereview.chromium.org/2305893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/observer/ResizeObservation.cpp:57: LayoutPoint
ResizeObservation::computeTargetLocation()
const

[atotic1, 2016-09-08 22:03:25]

const fixed.

[atotic1, 2016-09-08 22:03:39]

The CQ bit was checked by atotic@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 22:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 22:18:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 22:18:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[atotic1, 2016-09-08 22:29:23]

The CQ bit was checked by atotic@google.com

[atotic1, 2016-09-08 22:29:23]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2305893002/#ps80001
(title: "forgot to fix c++ tests")

[commit-bot: I haz the power, 2016-09-08 22:29:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:35:07]

Description was changed from

==========
Fix svg contentrect location

I was not computing entry.contetRect location correctly for <svg> elements.
It should be 0,0

BUG=641364
==========

to

==========
Fix svg contentrect location

I was not computing entry.contetRect location correctly for <svg> elements.
It should be 0,0

BUG=641364

Committed: https://crrev.com/879fbd0230e2bf90bd19a6b03c22f20fd31f46e4
Cr-Commit-Position: refs/heads/master@{#417450}
==========

[commit-bot: I haz the power, 2016-09-09 00:35:09]

Patchset 5 (id:??) landed as
https://crrev.com/879fbd0230e2bf90bd19a6b03c22f20fd31f46e4
Cr-Commit-Position: refs/heads/master@{#417450}