Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536493002:
Let SVGForeignObject's local SVG coordinates mean what it should (Closed)
|
Created:
4 years ago by Xianzhu Modified:
4 years ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet SVGForeignObject's local SVG coordinates mean what it should
We define "local SVG coordinates" as the space where SVG local transform
etc. apply. With this definition, SVGForeignObject's localSVGTransform
and localToSVGParentTransform should be the same which should both
transform from local SVG coordinates instead of local HTML coordinates
to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should
return the visual rect in local SVG coordinates instead of local HTML
coordinates.
With the above coordinates fixed, LayoutSVGBlock becomes the same for
LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML
coordinates: SVG coordinates = HTML coordinates + location() (though
location() is always zero for LayoutSVGText after
https://codereview.chromium.org/2531943002/). The HTML coordinate
mapping methods should also map between the HTML and SVG coordinates.
BUG=666416
TEST=refectoring, no outside behavior change. All existing tests should
pass. The internal behavior change will be tested in the next CL which
removes the special treatment of paint offsets of SVGForeignObjects.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57
Cr-Commit-Position: refs/heads/master@{#435337}
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : Rebase #Patch Set 4 : - #
Total comments: 2
Patch Set 5 : - #
Total comments: 2
Patch Set 6 : - #
Messages
Total messages: 42 (29 generated)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change ==========
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass ==========
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" as the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. ==========
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" as the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" as the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2536493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h (right): https://codereview.chromium.org/2536493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h:32: // which is the same as other normal SVG objects; WDYT of mentioning LayoutSVGModelObject? Maybe something like: - local SVG coordinate space: similar to LayoutSVGModelObject, the space that localSVGTransform() applies.
https://codereview.chromium.org/2536493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h (right): https://codereview.chromium.org/2536493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h:32: // which is the same as other normal SVG objects; On 2016/11/29 22:33:00, pdr. wrote: > WDYT of mentioning LayoutSVGModelObject? > > Maybe something like: > - local SVG coordinate space: similar to LayoutSVGModelObject, the space that > localSVGTransform() applies. Done.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2536493002/#ps80001 (title: "-")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2536493002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h (right): https://codereview.chromium.org/2536493002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h:32: // that localSVGTransform(), Nit: "the space that localSVGTransform() applies. (lets leave this in the cq and just fix it in one of the followups)
The CQ bit was unchecked by wangxianzhu@chromium.org
https://codereview.chromium.org/2536493002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h (right): https://codereview.chromium.org/2536493002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.h:32: // that localSVGTransform(), On 2016/11/30 00:11:41, pdr. wrote: > Nit: "the space that localSVGTransform() applies. > > (lets leave this in the cq and just fix it in one of the followups) Done.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2536493002/#ps100001 (title: "-")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480525841783370,
"parent_rev": "2f6dfdffd4065b9a22f0bd33ebaea21512e4c47e", "commit_rev":
"6c0d041dd82e5cfa3b0c45ecb2fd950dfe595f11"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" as the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Let SVGForeignObject's local SVG coordinates mean what it should We define "local SVG coordinates" as the space where SVG local transform etc. apply. With this definition, SVGForeignObject's localSVGTransform and localToSVGParentTransform should be the same which should both transform from local SVG coordinates instead of local HTML coordinates to parent SVG coordinates, and visualRectInLocalSVGCoordinates() should return the visual rect in local SVG coordinates instead of local HTML coordinates. With the above coordinates fixed, LayoutSVGBlock becomes the same for LayoutSVGText and LayoutSVGForeignObject regarding to SVG/HTML coordinates: SVG coordinates = HTML coordinates + location() (though location() is always zero for LayoutSVGText after https://codereview.chromium.org/2531943002/). The HTML coordinate mapping methods should also map between the HTML and SVG coordinates. BUG=666416 TEST=refectoring, no outside behavior change. All existing tests should pass. The internal behavior change will be tested in the next CL which removes the special treatment of paint offsets of SVGForeignObjects. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 Cr-Commit-Position: refs/heads/master@{#435337} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f451d9b5de5a1b4691d60d8c74341ac8a466cf57 Cr-Commit-Position: refs/heads/master@{#435337} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
