Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1812493002:
If add 'id' on pending SVG resource references does not work for non-resources (Closed)
|
Created:
4 years, 9 months ago by hyunjunekim2 Modified:
4 years, 9 months ago CC:
blink-reviews, chromium-reviews, krit, f(malita), gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIf add 'id' on pending SVG resource does not work for non-resources
When add 'id' on pending resource(<path>), no apply to
non-resources(<textPath>).
Because if need to build pending resource, re-layout non-resources.
BUG=585822
Committed: https://crrev.com/6a5978f382eca334bfab56ab7409a71e45a51bca
Cr-Commit-Position: refs/heads/master@{#381954}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 33 (14 generated)
Description was changed from ========== WIP 585822 WIP BUG=585822 ========== to ========== WIP 585822 WIP BUG=585822 ==========
Description was changed from ========== WIP 585822 WIP BUG=585822 ========== to ========== If adding 'id' on pending SVG resource references does not work for non-resources WIP BUG=585822 ==========
Description was changed from ========== If adding 'id' on pending SVG resource references does not work for non-resources WIP BUG=585822 ========== to ========== If adding 'id' on pending SVG resource does not work for non-resources WIP BUG=585822 ==========
Description was changed from ========== If adding 'id' on pending SVG resource does not work for non-resources WIP BUG=585822 ========== to ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, layout non-resources. BUG=585822 ==========
Description was changed from ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org
fs, pdr Could you check this patch and give me advice? Thank you.
https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path-expected.html (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path-expected.html:2: <div> Not needed here either. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html:2: <div> Not needed. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html:12: setTimeout(function() { It'd probably be preferable to use runAfterLayoutAndPaint here instead. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElement.cpp:151: markForLayoutAndParentResourceInvalidation(layoutObject); I think we usually have invalidation in the specific buildPendingResource() method (because their requirements differ etc, so this would invalidate unnecessarily in some cases.)
Description was changed from ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
Description was changed from ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources. Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
Description was changed from ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
fs, Could you check PS4? https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path-expected.html (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path-expected.html:2: <div> On 2016/03/17 17:27:33, fs wrote: > Not needed here either. Done. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html:2: <div> On 2016/03/17 17:27:33, fs wrote: > Not needed. Done. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/adding-id-on-pending-path.html:12: setTimeout(function() { On 2016/03/17 17:27:33, fs wrote: > It'd probably be preferable to use runAfterLayoutAndPaint here instead. Done. https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/1812493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElement.cpp:151: markForLayoutAndParentResourceInvalidation(layoutObject); On 2016/03/17 17:27:33, fs wrote: > I think we usually have invalidation in the specific buildPendingResource() > method (because their requirements differ etc, so this would invalidate > unnecessarily in some cases.) Moved this one to buildPendingResource().
Description was changed from ========== If add 'id' on pending SVG resource(<path>) does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
lgtm
fs, Could you re-check PS5? I'm sorry. I found a bug on PS4. Thank you.
Added one more test named 'removing-id-on-path.html'.
lgtm https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html (right): https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: <!DOCTYPE html> You can remove everything else but this.
Updated this patch. https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html (right): https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: <!DOCTYPE html> On 2016/03/18 09:46:54, fs wrote: > You can remove everything else but this. Done.
On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > Updated this patch. > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html (right): > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: <!DOCTYPE html> > On 2016/03/18 09:46:54, fs wrote: > > You can remove everything else but this. > > Done. You should've kept the doctype declaration (we don't want quirks mode.)
On 2016/03/18 10:41:55, fs wrote: > On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > > Updated this patch. > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html > (right): > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: > <!DOCTYPE html> > > On 2016/03/18 09:46:54, fs wrote: > > > You can remove everything else but this. > > > > Done. > > You should've kept the doctype declaration (we don't want quirks mode.) fs, I fixed it that kept the doctype declaration. Thank you.
On 2016/03/18 10:44:54, hyunjunekim2 wrote: > On 2016/03/18 10:41:55, fs wrote: > > On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > > > Updated this patch. > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > File > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html > > (right): > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: > > <!DOCTYPE html> > > > On 2016/03/18 09:46:54, fs wrote: > > > > You can remove everything else but this. > > > > > > Done. > > > > You should've kept the doctype declaration (we don't want quirks mode.) > > fs, I fixed it that kept the doctype declaration. Thank you. Hang on!
On 2016/03/18 at 10:44:54, hyunjune.kim wrote: > On 2016/03/18 10:41:55, fs wrote: > > On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > > > Updated this patch. > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html > > (right): > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: > > <!DOCTYPE html> > > > On 2016/03/18 09:46:54, fs wrote: > > > > You can remove everything else but this. > > > > > > Done. > > > > You should've kept the doctype declaration (we don't want quirks mode.) > > fs, I fixed it that kept the doctype declaration. Thank you. Thanks, looks good.
On 2016/03/18 at 10:46:39, hyunjune.kim wrote: > On 2016/03/18 10:44:54, hyunjunekim2 wrote: > > On 2016/03/18 10:41:55, fs wrote: > > > On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > > > > Updated this patch. > > > > > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > > File > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html > > > (right): > > > > > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: > > > <!DOCTYPE html> > > > > On 2016/03/18 09:46:54, fs wrote: > > > > > You can remove everything else but this. > > > > > > > > Done. > > > > > > You should've kept the doctype declaration (we don't want quirks mode.) > > > > fs, I fixed it that kept the doctype declaration. Thank you. > > Hang on! Sure thing! https://s-media-cache-ak0.pinimg.com/736x/ac/14/b5/ac14b5fcd9a186dbf45b3a3e23...
On 2016/03/18 10:51:18, fs wrote: > On 2016/03/18 at 10:46:39, hyunjune.kim wrote: > > On 2016/03/18 10:44:54, hyunjunekim2 wrote: > > > On 2016/03/18 10:41:55, fs wrote: > > > > On 2016/03/18 at 10:36:54, hyunjune.kim wrote: > > > > > Updated this patch. > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > > > File > > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html > > > > (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1812493002/diff/90001/third_party/WebKit/Layo... > > > > > > third_party/WebKit/LayoutTests/svg/text/removing-id-on-path-expected.html:1: > > > > <!DOCTYPE html> > > > > > On 2016/03/18 09:46:54, fs wrote: > > > > > > You can remove everything else but this. > > > > > > > > > > Done. > > > > > > > > You should've kept the doctype declaration (we don't want quirks mode.) > > > > > > fs, I fixed it that kept the doctype declaration. Thank you. > > > > Hang on! > > Sure thing! > https://s-media-cache-ak0.pinimg.com/736x/ac/14/b5/ac14b5fcd9a186dbf45b3a3e23... fs, Added the doctype declaration. :) Thank you. https://codereview.chromium.org/1812493002/diff2/160001:200001/third_party/We...
lgtm3
On 2016/03/18 11:43:42, fs wrote: > lgtm3 fs, Thank you. Go ahead.
The CQ bit was checked by hyunjune.kim@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1812493002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812493002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812493002/200001
Message was sent while issue was closed.
Description was changed from ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 ========== to ========== If add 'id' on pending SVG resource does not work for non-resources When add 'id' on pending resource(<path>), no apply to non-resources(<textPath>). Because if need to build pending resource, re-layout non-resources. BUG=585822 Committed: https://crrev.com/6a5978f382eca334bfab56ab7409a71e45a51bca Cr-Commit-Position: refs/heads/master@{#381954} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6a5978f382eca334bfab56ab7409a71e45a51bca Cr-Commit-Position: refs/heads/master@{#381954} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
