Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862733002:
Remove all use of crop edges from SkImageFilter::CropRect. (Closed)
|
Created:
4 years, 8 months ago by Stephen White Modified:
4 years, 8 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, danakj+watch_chromium.org, ajuma+watch-canvas_chromium.org, dshwang, jbroman, haraken, Rik, f(malita), blink-reviews-platform-graphics_chromium.org, blink-reviews, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove all use of crop edges from SkImageFilter::CropRect.
Since we're now always passing full crop rects to Skia (or nothing),
we don't need to use individual edge flags. This required teaching
canvas filters to call determineFilterPrimitiveSubregion() in order to
ensure that the filter subregions are valid.
BUG=599933
Committed: https://crrev.com/8e52c44a881302e67c4d7d924bcbe69f603e1aa5
Cr-Commit-Position: refs/heads/master@{#386709}
Patch Set 1 #Patch Set 2 : Fix FEMorphology bug #
Total comments: 5
Patch Set 3 : Simplify #Patch Set 4 : Update to ToT #
Messages
Total messages: 39 (19 generated)
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/1
Description was changed from ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 ========== to ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. In order to make the crop rect nullable, I've changed getCropRect() to take storage for the rect, and return that storage (filled) or nullptr for no crop rect. BUG=599933 ==========
senorblanco@chromium.org changed reviewers: + fmalita@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/20001
FLorin: PTAL. Thanks!
senorblanco@chromium.org changed reviewers: + junov@chromium.org
+junov@ for modules/canvas2d
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, just some getCropRect bike-shedding :) https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp (right): https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:333: lastEffect->determineFilterPrimitiveSubregion(); Is this something that could be pushed into FilterEffectBuilder::build()? https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/filters/FEBlend.cpp (right): https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/filters/FEBlend.cpp:64: return fromSkSp(SkXfermodeImageFilter::Make(std::move(mode), background.get(), foreground.get(), getCropRect(&cropRect))); Would it look cleaner to use a RAII helper and encapsulate the storage vs pointer details? AutoCropRect acr(this); // <- ctor implements the current getCropRect logic ... SkXfermodeImageFilter::Make(..., acr.get());
modules/canvas2d lgtm
https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp (right): https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:333: lastEffect->determineFilterPrimitiveSubregion(); On 2016/04/06 14:21:08, f(malita) wrote: > Is this something that could be pushed into FilterEffectBuilder::build()? Unfortunately, each of the paths (HTML and SVG) is already calling this from a different place. And I'd rather not mess with them just yet. https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/filters/FEBlend.cpp (right): https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/filters/FEBlend.cpp:64: return fromSkSp(SkXfermodeImageFilter::Make(std::move(mode), background.get(), foreground.get(), getCropRect(&cropRect))); On 2016/04/06 14:21:08, f(malita) wrote: > Would it look cleaner to use a RAII helper and encapsulate the storage vs > pointer details? > > AutoCropRect acr(this); // <- ctor implements the current getCropRect logic > ... > > SkXfermodeImageFilter::Make(..., acr.get()); > Thanks. Actually I think I'm going to rework this a different way.
Description was changed from ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. In order to make the crop rect nullable, I've changed getCropRect() to take storage for the rect, and return that storage (filled) or nullptr for no crop rect. BUG=599933 ========== to ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 ==========
New patch up. This one still uses the flags param, but only uses it to disable crop rects entirely (0). I'll likely turn it into a boolean at some point.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp (right): https://codereview.chromium.org/1862733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp:333: lastEffect->determineFilterPrimitiveSubregion(); On 2016/04/11 21:30:22, Stephen White wrote: > On 2016/04/06 14:21:08, f(malita) wrote: > > Is this something that could be pushed into FilterEffectBuilder::build()? > > Unfortunately, each of the paths (HTML and SVG) is already calling this from a > different place. And I'd rather not mess with them just yet. Ack.
The CQ bit was checked by senorblanco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/1862733002/#ps40001 (title: "Simplify")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by senorblanco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/1862733002/#ps60001 (title: "Update to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862733002/60001
Message was sent while issue was closed.
Description was changed from ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 ========== to ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 ========== to ========== Remove all use of crop edges from SkImageFilter::CropRect. Since we're now always passing full crop rects to Skia (or nothing), we don't need to use individual edge flags. This required teaching canvas filters to call determineFilterPrimitiveSubregion() in order to ensure that the filter subregions are valid. BUG=599933 Committed: https://crrev.com/8e52c44a881302e67c4d7d924bcbe69f603e1aa5 Cr-Commit-Position: refs/heads/master@{#386709} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e52c44a881302e67c4d7d924bcbe69f603e1aa5 Cr-Commit-Position: refs/heads/master@{#386709} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
