ResizeObserver implementation, part 2: lifetime

ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9PA/edit?usp=sharing

BUG=612962
ResizeObserver: conecting Objects
Committed: https://crrev.com/6c23d97a65d8568329a386bd1da1df27d8d0a893
Cr-Commit-Position: refs/heads/master@{#407088}

[atotic1, 2016-07-19 22:05:13]

Description was changed from

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
==========

to

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
==========

[atotic1, 2016-07-19 22:05:13]

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

[atotic1, 2016-07-19 22:10:33]

ResizeObserver implementation, part 2: object lifetime

This patch concerns object lifetime. It introduces all the 
classes, and links between them.

Object lifetime is described in the design doc at:
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

[eae, 2016-07-20 20:26:27]

OK, LGTM

[esprehn, 2016-07-21 17:52:29]

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.cpp:17:
m_target->ensureResizeObserverData().set(observer, this);
Can the caller do this instead? See IntersectionObserver

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.h:18: class
ResizeObservation : public GarbageCollected<ResizeObservation> {
final

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.h:29: // to the
ResizeObserver, to prevent it from being garbage-collected.
is this still true once you go to a push model?

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserver.cpp:23: m_controller =
document.resizeObserverController();
don't call it twice here, use the return value of ensureResizeObserverController

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserver.h:38: using
ObservationList = HeapLinkedHashSet<WeakMember<ResizeObservation>>;
remove this, you only use it in one spot

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:16: class
ResizeObserverController : public GarbageCollected<ResizeObserverController> {
final

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:17: 
remove blank line

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:19: 
you don't need all these blank lines around the public/private things

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:28: using
ObserverSet = HeapHashSet<WeakMember<ResizeObserver>>;
remove

[atotic1, 2016-07-21 19:15:00]

CR fixes are in. Please review, I've got 5 more in line.

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.cpp:17:
m_target->ensureResizeObserverData().set(observer, this);
On 2016/07/21 17:52:28, esprehn wrote:
> Can the caller do this instead? See IntersectionObserver

Done. Impressed you've caught this one. 

As I was working my way through the CR train, I've noticed this too,
it was already there in the next CR.

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.h:18: class
ResizeObservation : public GarbageCollected<ResizeObservation> {
On 2016/07/21 17:52:28, esprehn wrote:
> final

Done.

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObservation.h:29: // to the
ResizeObserver, to prevent it from being garbage-collected.
On 2016/07/21 17:52:28, esprehn wrote:
> is this still true once you go to a push model?

Good catch, no it is not. Done.

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserver.cpp:23: m_controller =
document.resizeObserverController();
On 2016/07/21 17:52:28, esprehn wrote:
> don't call it twice here, use the return value of
ensureResizeObserverController

Done.

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserver.h:38: using
ObservationList = HeapLinkedHashSet<WeakMember<ResizeObservation>>;
On 2016/07/21 17:52:29, esprehn wrote:
> remove this, you only use it in one spot

No. It is used in a later patch.

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

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:16: class
ResizeObserverController : public GarbageCollected<ResizeObserverController> {
On 2016/07/21 17:52:29, esprehn wrote:
> final

Done.

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:17: 
On 2016/07/21 17:52:29, esprehn wrote:
> remove blank line

Done.

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:19: 
On 2016/07/21 17:52:29, esprehn wrote:
> you don't need all these blank lines around the public/private things

Done.

https://codereview.chromium.org/2161313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/observer/ResizeObserverController.h:28: using
ObserverSet = HeapHashSet<WeakMember<ResizeObserver>>;
On 2016/07/21 17:52:29, esprehn wrote:
> remove

I prefer naming complex containers. It spares me from parsing the declaration
trying to figure out what it does. Is this against G coding guidelines?

Done.

[atotic1, 2016-07-21 19:42:19]

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

[commit-bot: I haz the power, 2016-07-21 19:43:08]

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-07-21 19:59:52]

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

[commit-bot: I haz the power, 2016-07-21 19:59:53]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[esprehn, 2016-07-22 05:52:24]

I prefer not aliasing types unless you're using it multiple times, then it cuts
down the verbosity and makes sense. If it's only used once it's just making the
code less clear. :)

lgtm

[atotic1, 2016-07-22 06:18:26]

The CQ bit was checked by atotic@google.com

[atotic1, 2016-07-22 06:18:27]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2161313002/#ps20001
(title: "CR fixes")

[commit-bot: I haz the power, 2016-07-22 06:18:56]

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-07-22 06:50:48]

Description was changed from

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
==========

to

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
==========

[commit-bot: I haz the power, 2016-07-22 06:50:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-22 06:52:10]

Description was changed from

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
==========

to

==========
ResizeObserver implementation, part 2: lifetime

The plot thickens:

This checkin introduces all the classes, and the links
between them. The links are important for object
lifetime management. Lifetime architecture is described
at
https://docs.google.com/document/d/1G4OmqqlFY3H3erQAUmteKES02-bIECikDjWBrDKH9...

BUG=612962

ResizeObserver: conecting Objects
Committed: https://crrev.com/6c23d97a65d8568329a386bd1da1df27d8d0a893
Cr-Commit-Position: refs/heads/master@{#407088}
==========

[commit-bot: I haz the power, 2016-07-22 06:52:11]

Patchset 2 (id:??) landed as
https://crrev.com/6c23d97a65d8568329a386bd1da1df27d8d0a893
Cr-Commit-Position: refs/heads/master@{#407088}