Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772773005:
Make resource lookup more uniform in SVGResources::buildResources (Closed)
|
Created:
3 years, 9 months ago by fs Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, fmalita+watch_chromium.org, 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceBar>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
Review-Url: https://codereview.chromium.org/2772773005
Cr-Commit-Position: refs/heads/master@{#459533}
Committed: https://chromium.googlesource.com/chromium/src/+/d7b6fa21c13aebade124c00d08dfc4e19dcf48d3
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (10 generated)
The CQ bit was checked by fs@opera.com 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
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceFoo>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well.
This makes the return value of the setFoo(...) methods unnecessary, so
make them return void.
BUG=454767
==========
to
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceFoo>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
==========
Description was changed from
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceFoo>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
==========
to
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceBar>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
==========
fs@opera.com changed reviewers: + pdr@chromium.org, schenney@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right): https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:145: bool isResourceOfType<LayoutSVGResourcePaintServer>( The LayoutSVGResourcePaintServer and LayoutSVGResourceContainer templates were pretty confusing to me but I don't see a better way to do this. Is there any way to reel those special cases in?
https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right): https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:145: bool isResourceOfType<LayoutSVGResourcePaintServer>( On 2017/03/24 at 16:46:34, pdr. wrote: > The LayoutSVGResourcePaintServer and LayoutSVGResourceContainer templates were pretty confusing to me but I don't see a better way to do this. Is there any way to reel those special cases in? We should (be able to) get rid of the latter by removing the "linked resource" from SVGResources (this would make it entirely a "style" thingy.) The latter is more difficult, and I don't have a direct plan ATM, although I suspect it might erode eventually as we migrate away from storing LayoutSVGResource<foo> pointers.
On 2017/03/24 at 19:26:54, fs wrote: > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right): > > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:145: bool isResourceOfType<LayoutSVGResourcePaintServer>( > On 2017/03/24 at 16:46:34, pdr. wrote: > > The LayoutSVGResourcePaintServer and LayoutSVGResourceContainer templates were pretty confusing to me but I don't see a better way to do this. Is there any way to reel those special cases in? > > We should (be able to) get rid of the latter by removing the "linked resource" from SVGResources (this would make it entirely a "style" thingy.) The latter is more difficult, and I don't have a direct plan ATM, although I suspect it might erode eventually as we migrate away from storing LayoutSVGResource<foo> pointers. ...and since I used the word "plan" above: When I get this bug properly sorted I'll try to remember to file an issue (task) and braindump my future plans for "SVGResources". Den som väntar på något gott, väntar alltid för länge.
The CQ bit was checked by fs@opera.com
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": 1, "attempt_start_ts": 1490386178600900, "parent_rev":
"2c73f746bc5896b76f2405a4f227718fe03326aa", "commit_rev":
"d7b6fa21c13aebade124c00d08dfc4e19dcf48d3"}
Message was sent while issue was closed.
Description was changed from
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceBar>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
==========
to
==========
Make resource lookup more uniform in SVGResources::buildResources
Replace the pattern:
if (!ensureResources(resources).setFoo(
getLayoutSVGResourceById<LayoutSVGResourceBar>(...)))
treeScopeResources.addPendingResource(...);
with a new pattern using a new attachToResource helper which folds in
the last step (addPendingResource) as well. This makes for less
callsites to adjust when modifying resource lookup.
This also makes the return value of the setFoo(...) methods unnecessary,
so make them return void.
BUG=454767
Review-Url: https://codereview.chromium.org/2772773005
Cr-Commit-Position: refs/heads/master@{#459533}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7b6fa21c13aebade124c00d08df...
==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/d7b6fa21c13aebade124c00d08df...
Message was sent while issue was closed.
On 2017/03/24 at 20:09:06, fs wrote: > On 2017/03/24 at 19:26:54, fs wrote: > > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right): > > > > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:145: bool isResourceOfType<LayoutSVGResourcePaintServer>( > > On 2017/03/24 at 16:46:34, pdr. wrote: > > > The LayoutSVGResourcePaintServer and LayoutSVGResourceContainer templates were pretty confusing to me but I don't see a better way to do this. Is there any way to reel those special cases in? > > > > We should (be able to) get rid of the latter by removing the "linked resource" from SVGResources (this would make it entirely a "style" thingy.) The latter is more difficult, and I don't have a direct plan ATM, although I suspect it might erode eventually as we migrate away from storing LayoutSVGResource<foo> pointers. > > ...and since I used the word "plan" above: When I get this bug properly sorted I'll try to remember to file an issue (task) and braindump my future plans for "SVGResources". Den som väntar på något gott, väntar alltid för länge. Ingen ko på isen. Tack!
Message was sent while issue was closed.
On 2017/03/24 at 20:45:38, pdr wrote: > On 2017/03/24 at 20:09:06, fs wrote: > > On 2017/03/24 at 19:26:54, fs wrote: > > > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/layout/svg/SVGResources.cpp (right): > > > > > > https://codereview.chromium.org/2772773005/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/layout/svg/SVGResources.cpp:145: bool isResourceOfType<LayoutSVGResourcePaintServer>( > > > On 2017/03/24 at 16:46:34, pdr. wrote: > > > > The LayoutSVGResourcePaintServer and LayoutSVGResourceContainer templates were pretty confusing to me but I don't see a better way to do this. Is there any way to reel those special cases in? > > > > > > We should (be able to) get rid of the latter by removing the "linked resource" from SVGResources (this would make it entirely a "style" thingy.) The latter is more difficult, and I don't have a direct plan ATM, although I suspect it might erode eventually as we migrate away from storing LayoutSVGResource<foo> pointers. > > > > ...and since I used the word "plan" above: When I get this bug properly sorted I'll try to remember to file an issue (task) and braindump my future plans for "SVGResources". Den som väntar på något gott, väntar alltid för länge. > > Ingen ko på isen. Tack! Nej, inte längre, för den sjönk när isen smälte! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
