Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG hairlines.

Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed because of Java heap error)

Committed: https://crrev.com/e0679d9453c26e2c2c1de1d3bfefadc7f83a364a
Cr-Commit-Position: refs/heads/master@{#427125}

[chrishtr, 2016-10-21 23:41:29]

Description was changed from

==========
none


none


none

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

to

==========
none


none


none

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

[chrishtr, 2016-10-21 23:42:34]

Description was changed from

==========
none


none


none

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

to

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

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

[chrishtr, 2016-10-22 00:01:10]

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

[chrishtr, 2016-10-22 00:01:11]

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
Because this code doesn't understand anything except the bounding box of the
shape, it is forced
to unconditionally expand the rect.

[chrishtr, 2016-10-22 00:01:22]

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

[commit-bot: I haz the power, 2016-10-22 00:02:52]

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-22 00:16:51]

This was much simpler than I expected. Looking forward to the try bot results
for svg/

[Xianzhu, 2016-10-22 00:35:47]

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
On 2016/10/22 00:01:11, chrishtr - OOO wrote:
> Because this code doesn't understand anything except the bounding box of the
> shape, it is forced
> to unconditionally expand the rect.

It seems too much to expand by 2 pixels each side.

This could be more precise with:
if (styleRef().svgStyle().hasVisibleStroke()) {
  LayoutUnit pad = 0.5;
  if (styleRef().svgStyle().capStyle() != ButtCap)
    pad += 0.5;
  visualRect.inflate(pad);
}

[commit-bot: I haz the power, 2016-10-22 01:32:57]

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

[commit-bot: I haz the power, 2016-10-22 01:32:58]

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, 2016-10-22 06:21:04]

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2444593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52:
visualRect.expand(IntRectOutsets(2, 2, 2, 2));
On 2016/10/22 at 00:35:46, Xianzhu wrote:
> On 2016/10/22 00:01:11, chrishtr - OOO wrote:
> > Because this code doesn't understand anything except the bounding box of the
> > shape, it is forced
> > to unconditionally expand the rect.
> 
> It seems too much to expand by 2 pixels each side.
> 
> This could be more precise with:
> if (styleRef().svgStyle().hasVisibleStroke()) {
>   LayoutUnit pad = 0.5;
>   if (styleRef().svgStyle().capStyle() != ButtCap)
>     pad += 0.5;
>   visualRect.inflate(pad);
> }

Done.

[chrishtr, 2016-10-22 06:21:08]

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

[commit-bot: I haz the power, 2016-10-22 06:21:16]

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-22 07:42:36]

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

[commit-bot: I haz the power, 2016-10-22 07:42:37]

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

[chrishtr, 2016-10-22 19:23:43]

Description was changed from

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

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

to

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

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

[chrishtr, 2016-10-22 19:29:23]

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

[commit-bot: I haz the power, 2016-10-22 19:29:28]

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

[chrishtr, 2016-10-22 19:32:07]

There are some failures in svg/, which are all changes to the layout tree as
text size of
LayoutSVGShape. This is because the method I removed code from in
SVGLayoutSupport is used
in mapToVisualRectInAncestorSpace, which in this case is used in (1) paint
invalidation and
(2) a couple of random cases for accessibility, and (3) SVGLayoutTreeAsText.

Therefore I think it's fine that these are smaller by 1px or so

https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

[commit-bot: I haz the power, 2016-10-22 21:01:40]

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

[commit-bot: I haz the power, 2016-10-22 21:01:41]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-10-22 23:24:49]

The latest patchset passes the dry-run CQ.

Please double-check the changes to invalidation output as indicated on earlier
patchsets and
files changed in this patch.

I'm now off on vacation, so will likely not be able to take any action on this
patch for a week.
Hopefully it can just go in cleanly.

[Xianzhu, 2016-10-24 16:17:30]

lgtm.

The layout test results look good.

As chrishtr is on vacation I'd like to land this first and address the comments
with another patch.

pdr@ wdyt?

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:51: if
(styleRef().svgStyle().hasVisibleStroke()) {
We should add !visualRect.isEmpty() to avoid inflating an empty rect to
non-empty.

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52: float pad =
0.5f;
Nit: s/float/LayoutUnit/

[pdr., 2016-10-24 16:50:15]

On 2016/10/24 at 16:17:30, wangxianzhu wrote:
> lgtm.
> 
> The layout test results look good.
> 
> As chrishtr is on vacation I'd like to land this first and address the
comments with another patch.
> 
> pdr@ wdyt?
> 
>
https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):
> 
>
https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:51: if
(styleRef().svgStyle().hasVisibleStroke()) {
> We should add !visualRect.isEmpty() to avoid inflating an empty rect to
non-empty.
> 
>
https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52: float pad =
0.5f;
> Nit: s/float/LayoutUnit/

LGTM to land and fixup in a followup. Thank you!

[Xianzhu, 2016-10-24 16:58:28]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-10-24 16:59:03]

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

[Xianzhu, 2016-10-24 19:28:27]

Description was changed from

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

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

to

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed
because of Java heap error)
==========

[Xianzhu, 2016-10-24 19:28:35]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2016-10-24 19:28:40]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-10-24 19:28:53]

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-24 19:48:30]

Description was changed from

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed
because of Java heap error)
==========

to

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed
because of Java heap error)
==========

[commit-bot: I haz the power, 2016-10-24 19:48:32]

Committed patchset #4 (id:3)

[commit-bot: I haz the power, 2016-10-24 19:56:00]

Description was changed from

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed
because of Java heap error)
==========

to

==========
Introduce Layout*::adjustVisualRectForRasterEffects and use it for SVG
hairlines.

BUG=651511
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
NOTRY=true (added after it passed on all CQ bots except android which failed
because of Java heap error)

Committed: https://crrev.com/e0679d9453c26e2c2c1de1d3bfefadc7f83a364a
Cr-Commit-Position: refs/heads/master@{#427125}
==========

[commit-bot: I haz the power, 2016-10-24 19:56:02]

Patchset 4 (id:??) landed as
https://crrev.com/e0679d9453c26e2c2c1de1d3bfefadc7f83a364a
Cr-Commit-Position: refs/heads/master@{#427125}

[chrishtr, 2016-10-31 20:52:01]

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp (right):

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:51: if
(styleRef().svgStyle().hasVisibleStroke()) {
On 2016/10/24 at 16:17:30, Xianzhu wrote:
> We should add !visualRect.isEmpty() to avoid inflating an empty rect to
non-empty.

Verified done by wangxianzhu.

https://codereview.chromium.org/2444593002/diff/3/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGShape.cpp:52: float pad =
0.5f;
On 2016/10/24 at 16:17:30, Xianzhu wrote:
> Nit: s/float/LayoutUnit/


Verified done by wangxianzhu.