Separate property and context updates in PaintPropertyTreeBuilder

Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor because some variables are
shared between the two updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/95247e236ea9fcb826b8f50e3cecdb2082833aee
Cr-Commit-Position: refs/heads/master@{#428570}

[pdr., 2016-10-27 20:01:34]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 20:03:13]

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-10-27 22:04:14]

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

[commit-bot: I haz the power, 2016-10-27 22:04:15]

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

[pdr., 2016-10-27 22:14:32]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 22:15:49]

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

[pdr., 2016-10-27 22:16:10]

Description was changed from

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a completely trivial refactor.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-10-27 22:16:29]

Description was changed from

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a completely trivial refactor.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-10-27 22:17:36]

Description was changed from

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor because some variables are
shared between the two updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-10-27 22:37:55]

pdr@chromium.org changed reviewers:
+ trchen@chromium.org, wangxianzhu@chromium.org

[pdr., 2016-10-27 22:39:34]

Still waiting on cluster telemetry results for this patch.

[trchen, 2016-10-27 23:15:21]

On 2016/10/27 22:39:34, pdr. wrote:
> Still waiting on cluster telemetry results for this patch.

Is it a prerequisite for incremental update?
I'm guessing the follow-up would be: "If no dirty flag, jump over the updates to
context bookkeeping directly."

[commit-bot: I haz the power, 2016-10-27 23:45:35]

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

[commit-bot: I haz the power, 2016-10-27 23:45:36]

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

[pdr., 2016-10-28 00:02:13]

On 2016/10/27 at 23:15:21, trchen wrote:
> On 2016/10/27 22:39:34, pdr. wrote:
> > Still waiting on cluster telemetry results for this patch.
> 
> Is it a prerequisite for incremental update?
> I'm guessing the follow-up would be: "If no dirty flag, jump over the updates
to context bookkeeping directly."

Yeah, exactly. My goal here is just to make this easy on reviewers because
there's a lot of complex code moving around.

[trchen, 2016-10-28 00:37:13]

On 2016/10/28 00:02:13, pdr. wrote:
> On 2016/10/27 at 23:15:21, trchen wrote:
> > On 2016/10/27 22:39:34, pdr. wrote:
> > > Still waiting on cluster telemetry results for this patch.
> > 
> > Is it a prerequisite for incremental update?
> > I'm guessing the follow-up would be: "If no dirty flag, jump over the
updates
> to context bookkeeping directly."
> 
> Yeah, exactly. My goal here is just to make this easy on reviewers because
> there's a lot of complex code moving around.

My concern is that node update and context bookkeeping may still need to be
interleaved, because a latter node may be parented under an optional previous
node. For example, ideally we want the form:

if (dirty) {
  updateTransform();
  updateOverflowClip();
}
bookkeepingTransform();
bookkeepingOverflowClip();

But due to inter-dependencies between scroll and transform, we may end up
writing:

if (dirty)
  updateTransform()
bookkeepingTransform();
if (dirty)
  updateOverflowClip();
bookkeepingOverflowClip();

Is this intended?

[pdr., 2016-10-28 00:58:02]

On 2016/10/28 at 00:37:13, trchen wrote:
> On 2016/10/28 00:02:13, pdr. wrote:
> > On 2016/10/27 at 23:15:21, trchen wrote:
> > > On 2016/10/27 22:39:34, pdr. wrote:
> > > > Still waiting on cluster telemetry results for this patch.
> > > 
> > > Is it a prerequisite for incremental update?
> > > I'm guessing the follow-up would be: "If no dirty flag, jump over the
updates
> > to context bookkeeping directly."
> > 
> > Yeah, exactly. My goal here is just to make this easy on reviewers because
> > there's a lot of complex code moving around.
> 
> My concern is that node update and context bookkeeping may still need to be
interleaved, because a latter node may be parented under an optional previous
node. For example, ideally we want the form:
> 
> if (dirty) {
>   updateTransform();
>   updateOverflowClip();
> }
> bookkeepingTransform();
> bookkeepingOverflowClip();
> 
> But due to inter-dependencies between scroll and transform, we may end up
writing:
> 
> if (dirty)
>   updateTransform()
> bookkeepingTransform();
> if (dirty)
>   updateOverflowClip();
> bookkeepingOverflowClip();
> 
> Is this intended?

A great point but this ends up not being an issue in practice; I think
updateOverflowClip is the only example. You can see this in the proof-of-concept
patch where we need to use a pointer in updateOverflowClip (search for
currentClip): https://codereview.chromium.org/2404213004

The more common situation is that the inter-dependencies are not optional. For
example, in updateScrollAndScrollTranslation the Scroll node depends on a
ScrollTranslation node, but because it is always created we just have:
if (dirty) {
  properties.updateScrollTranslation();
  properties.updateScroll(properties.scrollTranslation());
}
bookkeepingScrollTranslation();
bookkeepingScroll();

[pdr., 2016-10-28 18:39:49]

Btw, this seems to have had no effect on cluster telemetry.

[Xianzhu, 2016-10-28 18:57:26]

lgtm

[pdr., 2016-10-28 20:37:27]

On 2016/10/28 at 18:57:26, wangxianzhu wrote:
> lgtm

thanks!


@trchen, I'd like to make sure your concerns are addressed before landing.

[trchen, 2016-10-28 21:28:27]

lgtm too. Just wanted to make sure you were aware of it. (clicking commit)

[trchen, 2016-10-28 21:28:34]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-10-28 21:29:25]

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-10-28 23:04:10]

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

[commit-bot: I haz the power, 2016-10-28 23:04:12]

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...)

[pdr., 2016-10-28 23:05:27]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-10-28 23:05: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-10-28 23:10:07]

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

[commit-bot: I haz the power, 2016-10-28 23:10:09]

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

[pdr., 2016-10-28 23:17:47]

On 2016/10/28 at 23:10:09, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I'm able to build locally so I think this is just an infra issue.

[pdr., 2016-10-28 23:19:25]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-10-28 23:19:38]

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-10-28 23:28:33]

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

[commit-bot: I haz the power, 2016-10-28 23:28:34]

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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2016-10-28 23:44:11]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-10-28 23:44:31]

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-10-29 00:54:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-29 01:04:36]

Description was changed from

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor because some variables are
shared between the two updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Separate property and context updates in PaintPropertyTreeBuilder

This patch is a refactoring to separate the context update from
property building/updating in PaintPropertyTreeBuilder. This
patch will make future changes to PaintPropertyTreeBuilder
simpler, such as skipping the property update step entirely if
possible. Because of the size of this change, I'd like to land
this as a pure refactoring before any changes are introduced.

The big idea in this patch is to refactor this pattern:
void updateProperty(object, context) {
  if (object needs property)
    context.property = object.properties.update(...);
  else
    object.properties.clearProperty();
}

To this new pattern:
void updateProperty(object, context) {
  // Update property.
  if (object needs property)
    object.properties.update(...);
  else
    object.properties.clearProperty();

  // Update context.
  if (object.properties.property())
    context.property = object.properties.property();
}

We may want to separate the property and context updates into two functions in
a followup but it is not a straightforward refactor because some variables are
shared between the two updates.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/95247e236ea9fcb826b8f50e3cecdb2082833aee
Cr-Commit-Position: refs/heads/master@{#428570}
==========

[commit-bot: I haz the power, 2016-10-29 01:04:37]

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