Refactor to centralize code which decides whether ObjectPaintProperties are needed.

Refactor to centralize code which decides whether ObjectPaintProperties are needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

Review-Url: https://codereview.chromium.org/2831683003
Cr-Commit-Position: refs/heads/master@{#466916}
Committed: https://chromium.googlesource.com/chromium/src/+/7c90da71a8f435ae99bb7f90e0cc1cfcc96cc1fd

[chrishtr, 2017-04-19 21:52:21]

Description was changed from

==========
none


none


none


none

BUG=
==========

to

==========
none


none


none


none

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

[chrishtr, 2017-04-19 21:53:17]

Description was changed from

==========
none


none


none


none

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.



This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-19 21:53:25]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.



This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-19 21:57:58]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* Paint offset translation is always present if there is transform, not just
if the inherited PaintOffset is non-zero.
* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* Scroll transforms are always added when scrolling is possible, but not
when scrolling is not possible but a main thread scrolling reason changed.

This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-19 22:01:14]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* Paint offset translation is always present if there is transform, not just
if the inherited PaintOffset is non-zero.
* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* Scroll transforms are always added when scrolling is possible, but not
when scrolling is not possible but a main thread scrolling reason changed.

This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* Paint offset translation is always present if there is transform, not just
if the inherited PaintOffset is non-zero.
* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* Scroll transforms are always added when scrolling is possible, but not
when scrolling is not possible but a main thread scrolling reason changed.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-19 22:41:21]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[chrishtr, 2017-04-19 22:41:36]

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

[commit-bot: I haz the power, 2017-04-19 22:42:00]

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, 2017-04-19 23:06:00]

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

[commit-bot: I haz the power, 2017-04-19 23:06:00]

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

[chrishtr, 2017-04-19 23:39:32]

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

[commit-bot: I haz the power, 2017-04-19 23:40:03]

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, 2017-04-20 00:27:10]

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

[commit-bot: I haz the power, 2017-04-20 00:27:11]

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

[pdr., 2017-04-20 05:48:00]

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:882: bool
scroll_node_needed_for_main_thread_reasons =
Can you split out the ancestor reasons change into a separate patch? I think we
can land that pretty easily with no test changes.

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:292: return
object.IsSVGChild() &&
We can just dcheck the svg child check:
DCHECK(object.IsSVGChild());
// TODO(pdr): Check for the presence of a transform instead of the value.
// [rest of comment]
return !object.LocalToSVGParentTransform().IsIdentity();

The TODO(pdr) comment about checking IsIdentity got separated from the code. Can
you reunite them?

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:391: if
(NeedsPaintPropertiesForTransform(object)) {
At a high level the current code looks like this:
void UpdateProperty(obj, context) {
  if (needs update) {
    if (needs property)
      // update property, creating new storage as needed
    else
      // clear property
  }
  if (property)
    // update context with property
}
void UpdateProperties(obj, context) {
  UpdatePropertyA(obj, context)
  UpdatePropertyB(obj, context)
  ...
}

At a high level, this patch changes the code to look like:
bool NeedsProperty(obj) { ... }
void UpdateProperty(obj, context) {
  if (needs update) {
    if (NeedsProperty(obj))
      // update property using existing storage
    else
      // clear property
  }
  if (property)
    // update context with property
}
void UpdateProperties(obj, context) {
  if (NeedsPropertyA || NeedsPropertyB || ...)
    // create property storage

  UpdatePropertyA(obj, context)
  UpdatePropertyB(obj, context)
  ...
}

I thought we were going to do something like this:
void UpdateProperty(obj, context) {
  if (needs update) {
    if (needs property)
      // update property, creating new storage as needed
    else
      // clear property
  }
  if (property)
    // update context with property
}
Contexts GetPerFragmentContexts(object, ancestor_fragment_contexts) {
  // something resembling PaintLayer::CollectFragments
}
DescendantFragmentContexts UpdateProperties(obj, ancestor_fragment_contexts) {
  descendant_contexts = [];
  for (context : GetPerFragmentContexts(obj, ancestor_fragment_contexts) {
    UpdatePropertyA(obj, context)
    UpdatePropertyB(obj, context)
    ...
    descendant_contexts.add(context)
  }
  return descendant_contexts
}

Using pseudocode like above, can you sketch out future design? I don't see why
we need to split out the code for whether a property is needed.

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:422: }
Instead of tracking has_transform you can just make this an else:

if (NeedsPaintPropertiesForTransform(object)) {
  ...
} else {
  !has_transform code here
}

[chrishtr, 2017-04-20 17:13:15]

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:882: bool
scroll_node_needed_for_main_thread_reasons =
On 2017/04/20 at 05:48:00, pdr. wrote:
> Can you split out the ancestor reasons change into a separate patch? I think
we can land that pretty easily with no test changes.

Ok doing that now.

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:292: return
object.IsSVGChild() &&
On 2017/04/20 at 05:48:00, pdr. wrote:
> We can just dcheck the svg child check:
> DCHECK(object.IsSVGChild());

I can't do that, because this method is called blindly from
updatePaintProperties().

> // TODO(pdr): Check for the presence of a transform instead of the value.
> // [rest of comment]
> return !object.LocalToSVGParentTransform().IsIdentity();
> 
> The TODO(pdr) comment about checking IsIdentity got separated from the code.
Can you reunite them?

Done.

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:391: if
(NeedsPaintPropertiesForTransform(object)) {
On 2017/04/20 at 05:48:00, pdr. wrote:
> At a high level the current code looks like this:
> void UpdateProperty(obj, context) {
>   if (needs update) {
>     if (needs property)
>       // update property, creating new storage as needed
>     else
>       // clear property
>   }
>   if (property)
>     // update context with property
> }
> void UpdateProperties(obj, context) {
>   UpdatePropertyA(obj, context)
>   UpdatePropertyB(obj, context)
>   ...
> }
> 
> At a high level, this patch changes the code to look like:
> bool NeedsProperty(obj) { ... }
> void UpdateProperty(obj, context) {
>   if (needs update) {
>     if (NeedsProperty(obj))
>       // update property using existing storage
>     else
>       // clear property
>   }
>   if (property)
>     // update context with property
> }
> void UpdateProperties(obj, context) {
>   if (NeedsPropertyA || NeedsPropertyB || ...)
>     // create property storage
> 
>   UpdatePropertyA(obj, context)
>   UpdatePropertyB(obj, context)
>   ...
> }
> 
> I thought we were going to do something like this:
> void UpdateProperty(obj, context) {
>   if (needs update) {
>     if (needs property)
>       // update property, creating new storage as needed
>     else
>       // clear property
>   }
>   if (property)
>     // update context with property
> }
> Contexts GetPerFragmentContexts(object, ancestor_fragment_contexts) {
>   // something resembling PaintLayer::CollectFragments
> }
> DescendantFragmentContexts UpdateProperties(obj, ancestor_fragment_contexts) {
>   descendant_contexts = [];
>   for (context : GetPerFragmentContexts(obj, ancestor_fragment_contexts) {
>     UpdatePropertyA(obj, context)
>     UpdatePropertyB(obj, context)
>     ...
>     descendant_contexts.add(context)
>   }
>   return descendant_contexts
> }
> 
> Using pseudocode like above, can you sketch out future design? I don't see why
we need to split out the code for whether a property is needed.

The pseudo-code I have in mind in a future step is:

// Repeat for B, C, etc.
bool NeedsPropertyA() { ...}
bool UpdatePropertyAForFragment(obj, fragment, context) {
  if (needs_update)) {
    if (NeedsPropertyA(obj))
      // update property using fragment
    else
      // clear property
  }
}

bool NeedsFragments(obj) {
  return needsPropertyA(obj) || needsPropertyB(obj) || ...;
}

void UpdateProperties(obj, context) {
  if (NeedsFragments(obj)) {
    // Not really a Fragments object, just a linked list of
ObjectPaintProperties.
    // Also computes ancestor fragment to connect pointers to in property tree.
    Fragments fragments = GetFragments(obj);
    // Usually just one fragment.
    for (fragment: fragments) {
      UpdatePropertyAForFragment(obj, fragment, context);
      UpdatePropertyAForFragment(obj, fragment, context);
      ...
   }
  } else {
    // Clear all fragments; update context with force states and
under-invalidation checking
  }
}

https://codereview.chromium.org/2831683003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:422: }
On 2017/04/20 at 05:48:00, pdr. wrote:
> Instead of tracking has_transform you can just make this an else:
> 
> if (NeedsPaintPropertiesForTransform(object)) {
>   ...
> } else {
>   !has_transform code here
> }

Done.

[pdr., 2017-04-20 17:36:59]

> On 2017/04/20 at 05:48:00, pdr. wrote:
> > Using pseudocode like above, can you sketch out future design? I don't see
why we need to split out the code for whether a property is needed.
> 
> The pseudo-code I have in mind in a future step is:
> 
> // Repeat for B, C, etc.
> bool NeedsPropertyA() { ...}
> bool UpdatePropertyAForFragment(obj, fragment, context) {
>   if (needs_update)) {
>     if (NeedsPropertyA(obj))
>       // update property using fragment
>     else
>       // clear property
>   }
> }
> 
> bool NeedsFragments(obj) {
>   return needsPropertyA(obj) || needsPropertyB(obj) || ...;
> }
> 
> void UpdateProperties(obj, context) {
>   if (NeedsFragments(obj)) {
>     // Not really a Fragments object, just a linked list of
ObjectPaintProperties.
>     // Also computes ancestor fragment to connect pointers to in property
tree.
>     Fragments fragments = GetFragments(obj);
>     // Usually just one fragment.
>     for (fragment: fragments) {
>       UpdatePropertyAForFragment(obj, fragment, context);
>       UpdatePropertyAForFragment(obj, fragment, context);
>       ...
>    }
>   } else {
>     // Clear all fragments; update context with force states and
under-invalidation checking
>   }
> }
> 

Thanks, your pseudocode helps a lot.

Don't we need to have different contexts passed down for each fragment?

For example, lets say we create a different paint offset transform node for each
of 3 fragments. If we have a 3d transform node in just one of these fragments,
what code connects this 3d transform node's parent to the appropriate paint
offset transform?

[chrishtr, 2017-04-20 17:42:13]

On 2017/04/20 at 17:36:59, pdr wrote:
> > On 2017/04/20 at 05:48:00, pdr. wrote:
> > > Using pseudocode like above, can you sketch out future design? I don't see
why we need to split out the code for whether a property is needed.
> > 
> > The pseudo-code I have in mind in a future step is:
> > 
> > // Repeat for B, C, etc.
> > bool NeedsPropertyA() { ...}
> > bool UpdatePropertyAForFragment(obj, fragment, context) {
> >   if (needs_update)) {
> >     if (NeedsPropertyA(obj))
> >       // update property using fragment
> >     else
> >       // clear property
> >   }
> > }
> > 
> > bool NeedsFragments(obj) {
> >   return needsPropertyA(obj) || needsPropertyB(obj) || ...;
> > }
> > 
> > void UpdateProperties(obj, context) {
> >   if (NeedsFragments(obj)) {
> >     // Not really a Fragments object, just a linked list of
ObjectPaintProperties.
> >     // Also computes ancestor fragment to connect pointers to in property
tree.
> >     Fragments fragments = GetFragments(obj);
> >     // Usually just one fragment.
> >     for (fragment: fragments) {
> >       UpdatePropertyAForFragment(obj, fragment, context);
> >       UpdatePropertyAForFragment(obj, fragment, context);
> >       ...
> >    }
> >   } else {
> >     // Clear all fragments; update context with force states and
under-invalidation checking
> >   }
> > }
> > 
> 
> Thanks, your pseudocode helps a lot.
> 
> Don't we need to have different contexts passed down for each fragment?
> 
> For example, lets say we create a different paint offset transform node for
each of 3 fragments. If we have a 3d transform node in just one of these
fragments, what code connects this 3d transform node's parent to the appropriate
paint offset transform?

Yes. ComputeFragments would do that. That is what I was getting at with the
comment
 "Also computes ancestor fragment to connect pointers to in property tree" also.

[pdr., 2017-04-20 18:15:13]

On 2017/04/20 at 17:42:13, chrishtr wrote:
> On 2017/04/20 at 17:36:59, pdr wrote:
> > > On 2017/04/20 at 05:48:00, pdr. wrote:
> > > > Using pseudocode like above, can you sketch out future design? I don't
see why we need to split out the code for whether a property is needed.
> > > 
> > > The pseudo-code I have in mind in a future step is:
> > > 
> > > // Repeat for B, C, etc.
> > > bool NeedsPropertyA() { ...}
> > > bool UpdatePropertyAForFragment(obj, fragment, context) {
> > >   if (needs_update)) {
> > >     if (NeedsPropertyA(obj))
> > >       // update property using fragment
> > >     else
> > >       // clear property
> > >   }
> > > }
> > > 
> > > bool NeedsFragments(obj) {
> > >   return needsPropertyA(obj) || needsPropertyB(obj) || ...;
> > > }
> > > 
> > > void UpdateProperties(obj, context) {
> > >   if (NeedsFragments(obj)) {
> > >     // Not really a Fragments object, just a linked list of
ObjectPaintProperties.
> > >     // Also computes ancestor fragment to connect pointers to in property
tree.
> > >     Fragments fragments = GetFragments(obj);
> > >     // Usually just one fragment.
> > >     for (fragment: fragments) {
> > >       UpdatePropertyAForFragment(obj, fragment, context);
> > >       UpdatePropertyAForFragment(obj, fragment, context);
> > >       ...
> > >    }
> > >   } else {
> > >     // Clear all fragments; update context with force states and
under-invalidation checking
> > >   }
> > > }
> > > 
> > 
> > Thanks, your pseudocode helps a lot.
> > 
> > Don't we need to have different contexts passed down for each fragment?
> > 
> > For example, lets say we create a different paint offset transform node for
each of 3 fragments. If we have a 3d transform node in just one of these
fragments, what code connects this 3d transform node's parent to the appropriate
paint offset transform?
> 
> Yes. ComputeFragments would do that. That is what I was getting at with the
comment
>  "Also computes ancestor fragment to connect pointers to in property tree"
also.

UpdateProperties updates the passed-in context for its descendants. If there are
multiple possible contexts depending on the fragment, do we need to pass around
an array of per-fragment contexts? If not, which fragment determines the paint
offset that's in the context used by paint invalidation?

[chrishtr, 2017-04-20 18:39:45]

On 2017/04/20 at 18:15:13, pdr wrote:
> On 2017/04/20 at 17:42:13, chrishtr wrote:
> > On 2017/04/20 at 17:36:59, pdr wrote:
> > > > On 2017/04/20 at 05:48:00, pdr. wrote:
> > > > > Using pseudocode like above, can you sketch out future design? I don't
see why we need to split out the code for whether a property is needed.
> > > > 
> > > > The pseudo-code I have in mind in a future step is:
> > > > 
> > > > // Repeat for B, C, etc.
> > > > bool NeedsPropertyA() { ...}
> > > > bool UpdatePropertyAForFragment(obj, fragment, context) {
> > > >   if (needs_update)) {
> > > >     if (NeedsPropertyA(obj))
> > > >       // update property using fragment
> > > >     else
> > > >       // clear property
> > > >   }
> > > > }
> > > > 
> > > > bool NeedsFragments(obj) {
> > > >   return needsPropertyA(obj) || needsPropertyB(obj) || ...;
> > > > }
> > > > 
> > > > void UpdateProperties(obj, context) {
> > > >   if (NeedsFragments(obj)) {
> > > >     // Not really a Fragments object, just a linked list of
ObjectPaintProperties.
> > > >     // Also computes ancestor fragment to connect pointers to in
property tree.
> > > >     Fragments fragments = GetFragments(obj);
> > > >     // Usually just one fragment.
> > > >     for (fragment: fragments) {
> > > >       UpdatePropertyAForFragment(obj, fragment, context);
> > > >       UpdatePropertyAForFragment(obj, fragment, context);
> > > >       ...
> > > >    }
> > > >   } else {
> > > >     // Clear all fragments; update context with force states and
under-invalidation checking
> > > >   }
> > > > }
> > > > 
> > > 
> > > Thanks, your pseudocode helps a lot.
> > > 
> > > Don't we need to have different contexts passed down for each fragment?
> > > 
> > > For example, lets say we create a different paint offset transform node
for each of 3 fragments. If we have a 3d transform node in just one of these
fragments, what code connects this 3d transform node's parent to the appropriate
paint offset transform?
> > 
> > Yes. ComputeFragments would do that. That is what I was getting at with the
comment
> >  "Also computes ancestor fragment to connect pointers to in property tree"
also.
> 
> UpdateProperties updates the passed-in context for its descendants. If there
are multiple possible contexts depending on the fragment, do we need to pass
around an array of per-fragment contexts? If not, which fragment determines the
paint offset that's in the context used by paint invalidation?

No I don't think an array of contexts will be needed. I think we can get away
with using the single context to determine things like containing block (which
is unaffected
by fragmentation), and then look at the fragments for the containing block
inside of "GetFragments" to figure out how to extract the parent pointers for
each fragment's
transforms etc.
Also, paint offset for a fragment would be adjusted here, based on its fragment
offset plus the adjustment for local transform as usual.

[pdr., 2017-04-20 19:15:08]

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

[pdr., 2017-04-20 19:15:31]

+reviewer Tien-Ren, does the above pseudocode/direction look good to you too?

[trchen, 2017-04-20 21:18:34]

On 2017/04/20 19:15:31, pdr. wrote:
> +reviewer Tien-Ren, does the above pseudocode/direction look good to you too?

Yes lgtm. I was thinking the same question whether we need to pass along an
array of contexts, but either way will be built on top of this CL.

---- Beyond the scope of this CL ----
I'm not very sure how out-of-flow position children interact with pagination
context. That will affect the structure of the context.

Let's leave alone out-of-flow children for now. For in-flow context, it used to
be a global set of property states + paint offset. With fragmentation, the
context becomes a mapping of LayoutPoint-->PropertyTreeStates, i.e. partitioning
the logical layout space into regions, where each region could have different
property states + paint offset. The non-fragmentated case is really a
degenerated mapping that every input maps to the same states.

If we don't really care about fragmentation performance, passing the
fragmentation container and derive the partition on demand is fine. I do worry
about eBook apps that uses Blink to display .epub though. Those documents often
contain thousands of fragments. Invalidation can be very slow if for each
LayoutObject we have to re-enumerate every fragments to compute its screen
visual rect. I think O(logn) lookup to map from LayoutRect to fragments (and
associated property states) is still important.

[chrishtr, 2017-04-20 22:33:19]

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

[commit-bot: I haz the power, 2017-04-20 22:33:56]

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, 2017-04-20 22:36:59]

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

[commit-bot: I haz the power, 2017-04-20 22:37:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[pdr., 2017-04-20 22:56:28]

Overall looks good, mostly nits.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:743: // The
<input> elements can't have contents thus CSS overflow property
This comment was dropped. Is it wrong?

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:227: static
bool NeedsPaintPropertiesForPaintOffsetTranslation(
Bikeshed: how about NeedsPaintOffsetTranslation to keep the names a little
shorter? (Similarly for the others)

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:250: if
(!object.IsLayoutView() && context.current.paint_offset == LayoutPoint())
Can you add a comment here? Something like:
// As an optimization to reduce the number of properties, skip identity paint
// offset translations. An exception is the layout view because root layer
// scrolling needs a transform node to ensure fixed and absolute descendants
// use the correct transform space.

I think we want to add the RLS check to the layout view check too. Maybe:
if (uses_paint_offset_translation && context.current.paint_offset ==
LayoutPoint()) {
  if (!(object.IsLayoutView() &&
RuntimeEnabledFeatures::rootLayerScrollingEnabled()))
    uses_paint_offset_translation = false;
}

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:253: auto*
properties = object.GetMutableForPainting().PaintProperties();
In the future maybe we can refactor the Update* functions to take a paint
properties param which will save a lot of duplicated code.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:482: static
bool NeedsPaintPropertiesForEffect(const LayoutObject& object) {
Do we need to include the "!is_css_isolated_group && !object.IsSVGChild()" check
here? It's still in UpdateEffect.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:165:
ALWAYS_INLINE static void UpdatePaintProperties(const LayoutObject&);
Can you add a comment here? Something like:
// Ensure the ObjectPaintProperties object is created if it will be needed, or
cleared otherwise.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp
(right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp:555:
EXPECT_EQ(nullptr, target->GetLayoutObject()->PaintProperties());
Can you add a comment here so we don't accidentally refactor this away in the
future? Something like:
// Ensure the paint properties object was cleared as it is no longer needed.
(Or add a new test of the paint property clearing optimization)

[chrishtr, 2017-04-21 01:20:03]

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:743: // The
<input> elements can't have contents thus CSS overflow property
On 2017/04/20 at 22:56:28, pdr. wrote:
> This comment was dropped. Is it wrong?

It was obsolete, because control clips have subsequent to this comment
been unified with overflow clips throughout, and in
LayoutBox::ShouldClipOverflow in particular.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:227: static
bool NeedsPaintPropertiesForPaintOffsetTranslation(
On 2017/04/20 at 22:56:28, pdr. wrote:
> Bikeshed: how about NeedsPaintOffsetTranslation to keep the names a little
shorter? (Similarly for the others)

Done. Now there is a nice symmetry: Needs*() and Update*() are paired.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:250: if
(!object.IsLayoutView() && context.current.paint_offset == LayoutPoint())
On 2017/04/20 at 22:56:28, pdr. wrote:
> Can you add a comment here? Something like:
> // As an optimization to reduce the number of properties, skip identity paint
> // offset translations. An exception is the layout view because root layer
> // scrolling needs a transform node to ensure fixed and absolute descendants
> // use the correct transform space.
> 
> I think we want to add the RLS check to the layout view check too. Maybe:
> if (uses_paint_offset_translation && context.current.paint_offset ==
LayoutPoint()) {
>   if (!(object.IsLayoutView() &&
RuntimeEnabledFeatures::rootLayerScrollingEnabled()))
>     uses_paint_offset_translation = false;
> }

Done. Also cleaned up the code some more.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:253: auto*
properties = object.GetMutableForPainting().PaintProperties();
On 2017/04/20 at 22:56:28, pdr. wrote:
> In the future maybe we can refactor the Update* functions to take a paint
properties param which will save a lot of duplicated code.

Agreed.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:482: static
bool NeedsPaintPropertiesForEffect(const LayoutObject& object) {
On 2017/04/20 at 22:56:28, pdr. wrote:
> Do we need to include the "!is_css_isolated_group && !object.IsSVGChild()"
check here? It's still in UpdateEffect.

Ah good catch. Added, and simplified UpdateEffect.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:165:
ALWAYS_INLINE static void UpdatePaintProperties(const LayoutObject&);
On 2017/04/20 at 22:56:28, pdr. wrote:
> Can you add a comment here? Something like:
> // Ensure the ObjectPaintProperties object is created if it will be needed, or
cleared otherwise.

Done.

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp
(right):

https://codereview.chromium.org/2831683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp:555:
EXPECT_EQ(nullptr, target->GetLayoutObject()->PaintProperties());
On 2017/04/20 at 22:56:28, pdr. wrote:
> Can you add a comment here so we don't accidentally refactor this away in the
future? Something like:
> // Ensure the paint properties object was cleared as it is no longer needed.
> (Or add a new test of the paint property clearing optimization)

Added comment.

[chrishtr, 2017-04-21 01:21:58]

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

[commit-bot: I haz the power, 2017-04-21 01:22:11]

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

[pdr., 2017-04-21 01:24:35]

LGTM

[chrishtr, 2017-04-21 01:25:01]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-04-21 01:25:07]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-21 01:25:07]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps160001
(title: "none")

[pdr., 2017-04-21 01:25:09]

please update the change description

[commit-bot: I haz the power, 2017-04-21 01:25:15]

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

[pdr., 2017-04-21 01:26:08]

On 2017/04/21 at 01:25:09, pdr. wrote:
> please update the change description

nvm, change description is fine.

cleared to land!

[chrishtr, 2017-04-21 01:29:52]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* Paint offset translation is always present if there is transform, not just
if the inherited PaintOffset is non-zero.
* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* Scroll transforms are always added when scrolling is possible, but not
when scrolling is not possible but a main thread scrolling reason changed.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-21 01:30:12]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperty's are
needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperties are
needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

[chrishtr, 2017-04-21 01:30:18]

Updated

[commit-bot: I haz the power, 2017-04-21 03:06:26]

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

[commit-bot: I haz the power, 2017-04-21 03:06:28]

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

[chrishtr, 2017-04-21 22:02:37]

https://codereview.chromium.org/2831683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2831683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1180:
context.force_subtree_update = true;
Updated to force subtree update if we change allocation of PaintProperties.
The previous patch set had a bug in which we would not do so, which would
fail, because if we go from requiring a paint property for some reason to
having no reasons, the paint properties are cleared. In that case, the

if (paint_properties)
  forced_subtree_update |= update paint_properties.propertyX()

would never execute because the if failed.

[chrishtr, 2017-04-21 22:02:41]

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

[commit-bot: I haz the power, 2017-04-21 22:03:14]

Dry run: CQ is trying da patch.

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

[pdr., 2017-04-21 22:07:30]

On 2017/04/21 at 22:02:37, chrishtr wrote:
>
https://codereview.chromium.org/2831683003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2831683003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1180:
context.force_subtree_update = true;
> Updated to force subtree update if we change allocation of PaintProperties.
> The previous patch set had a bug in which we would not do so, which would
> fail, because if we go from requiring a paint property for some reason to
> having no reasons, the paint properties are cleared. In that case, the
> 
> if (paint_properties)
>   forced_subtree_update |= update paint_properties.propertyX()
> 
> would never execute because the if failed.

LGTM nice find. Please make a trivial change to only force subtree updates when
clearing.

[chrishtr, 2017-04-21 22:10:15]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-21 22:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps200001
(title: "none")

[commit-bot: I haz the power, 2017-04-21 22:10:57]

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, 2017-04-21 23:44:34]

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

[commit-bot: I haz the power, 2017-04-21 23:44:36]

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

[chrishtr, 2017-04-24 22:13:49]

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

[commit-bot: I haz the power, 2017-04-24 22:14:21]

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, 2017-04-24 23:26:40]

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

[commit-bot: I haz the power, 2017-04-24 23:26:41]

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

[chrishtr, 2017-04-25 00:02:09]

Fixed to adjust for blending (and added a unittest).
Re-added check for whether a local to border box transform is not identity.

[chrishtr, 2017-04-25 00:02:12]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-25 00:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps220001
(title: "none")

[commit-bot: I haz the power, 2017-04-25 00:02:51]

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, 2017-04-25 01:06:44]

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

[commit-bot: I haz the power, 2017-04-25 01:06:46]

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

[chrishtr, 2017-04-25 01:42:13]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-25 01:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps240001
(title: "none")

[commit-bot: I haz the power, 2017-04-25 01:42: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, 2017-04-25 01:57:45]

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

[commit-bot: I haz the power, 2017-04-25 01:57:46]

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

[chrishtr, 2017-04-25 02:56:45]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-25 02:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps260001
(title: "Merge branch 'master' into multicol")

[commit-bot: I haz the power, 2017-04-25 02:57:06]

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, 2017-04-25 03:05:27]

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

[commit-bot: I haz the power, 2017-04-25 03:05:29]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chrishtr, 2017-04-25 03:17:46]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-25 03:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps280001
(title: "none")

[commit-bot: I haz the power, 2017-04-25 03:18:15]

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, 2017-04-25 04:18:41]

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

[commit-bot: I haz the power, 2017-04-25 04:18:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-04-25 05:50:11]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-25 05:50:11]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2831683003/#ps300001
(title: "none")

[commit-bot: I haz the power, 2017-04-25 05:50: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, 2017-04-25 07:27:48]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1493099411204370,
"parent_rev": "ce75e0da74bdaf313e73520f040ff2bd446d2d7a", "commit_rev":
"7c90da71a8f435ae99bb7f90e0cc1cfcc96cc1fd"}

[commit-bot: I haz the power, 2017-04-25 07:28:01]

Description was changed from

==========
Refactor to centralize code which decides whether ObjectPaintProperties are
needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

to

==========
Refactor to centralize code which decides whether ObjectPaintProperties are
needed.

This is not a pure refactor. Changes:

* SVG local to border box transforms are always present, not just if the
LocalToBorderBoxTransform+PaintOffset is non-identity.
* ObjectPaintProperty objects are cleared from the PaintRareData if not needed.

This is step 1 for SPv2 multicol.

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

Review-Url: https://codereview.chromium.org/2831683003
Cr-Commit-Position: refs/heads/master@{#466916}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c90da71a8f435ae99bb7f90e0cc...
==========

[commit-bot: I haz the power, 2017-04-25 07:28:02]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/7c90da71a8f435ae99bb7f90e0cc...