Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001653002:
Remove SkDevice, obsolete constructor (Closed)
DescriptionRemove SkDevice, obsolete constructor
We already have all the API we need to rewrite skia_utils_mac without
referring to the to-be-deprecated SkBaseDevice or its
BitmapPlatformDevice subclasses. Also get rid of a constructor that
was marked "TODO: delete this" and all of the expensive code behind
it which was *only* activated by unit tests.
R=fmalita@chromium.org
BUG=543755
Committed: https://crrev.com/ad6f9da1d6372d185ffe51dc62246d5e4ffa8782
Cr-Commit-Position: refs/heads/master@{#398636}
Patch Set 1 #Patch Set 2 : -> -> . #Patch Set 3 : No more exactitude #Patch Set 4 : obsolete constructor implementation delenda est #Patch Set 5 : unit test hack #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Remove NoBits checks from unit test #Patch Set 9 : clipping diagnostics #Patch Set 10 : allow null userClipRect #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : remove debug code, fix spelling error in comment #Patch Set 15 : Trim another out-of-date comment #
Total comments: 6
Patch Set 16 : expect inverse CTM to avoid additional clipping #
Total comments: 1
Patch Set 17 : Florin knows my intent better than I do #Patch Set 18 : debug again #Patch Set 19 : fix %f->%d in debug #Patch Set 20 : unary negative of unsigned is surprising #Patch Set 21 : clean out debugging statements #
Messages
Total messages: 37 (15 generated)
Description was changed from ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this". R=fmalita@chromium.org BUG=543755 ========== to ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this". R=fmalita@chromium.org BUG=543755 ==========
Description was changed from ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this". R=fmalita@chromium.org BUG=543755 ========== to ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this" and all of the expensive code behind it which was *only* activated by unit tests. R=fmalita@chromium.org BUG=543755 ==========
The CQ bit was checked by tomhudson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001653002/60001
Florin, PTAL; this gets rid of two more SkDevice users and a lot of dead code.
Nice, LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
tomhudson@google.com changed reviewers: + reed@google.com
SkiaBitLocker is only used from Blink's platform/mac/LocalCurrentGraphicsContext. ThemePainterMac::paintTextField() ThemePainterMac::paintCapsLockIndicator() ThemePainterMac::paintTextArea() ThemePainterMac::paintMenuList() ThemePainterMac::paintProgressBar() ThemePainterMac::paintSearchField() ThemeMac.paintCheckbox() ThemeMac.paintRadio() ThemeMac.paintButton() ThemeMac.paintStepper() ScrollbarThemeMac::paintTrackBackground() ScrollbarThemeMac::paintThumb() There are no visible clips being set on the SkCanvas and expected to apply to the CGContext in this code. Because of that, per reed@, we're going to remove the contract that post-SkiaBitLocker-creation clips are propagated from SkCanvas to CGContext, and remove relevant sections of the unit test.
The CQ bit was checked by tomhudson@google.com to run a CQ dry run
PTAL, Mike; I wasn't able to change the clipping argument to a pointer because of awkwardnesses in how the SkiaBitLocker class is used in Blink. Am I right in thinking that SkIRect::MakeSize(canvas.getBaseLayerSize()) is a reasonable "noop" argument to pass to clip? Florin, after discussion with Mike removed the TestNoBits cases of the unit test because that was *supposed* to be checking on a case where bitmap.installPixels() wasn't ever called, which hasn't been actually executed in this test for years. By inspection, the ClipNoBits codepaths aren't used by Chrome, and I don't think TranslateNoBits is either; frankly no idea about plain NoBits.
The CQ bit was checked by tomhudson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001653002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_mac.h File skia/ext/skia_utils_mac.h (right): https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac.h:104: SkIRect::MakeSize(canvas->getBaseLayerSize()). If the clip is in local coords, then the last part of the comment doesn't seem right: one would need to also run the clip through the inverse CTM. Do we have unit test coverage for non-identity CTM cases?
https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_mac.h File skia/ext/skia_utils_mac.h (right): https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac.h:107: const SkIRect& userClipRect, Not new to this CL, but userClipRect should be an SkRect: we convert to one anyway, and we probably need the additional precision for local coords. https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... File skia/ext/skia_utils_mac_unittest.mm (right): https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac_unittest.mm:153: skia::SkiaBitLocker bitLocker(&canvas, SkIRect::MakeSize(canvas.getBaseLayerSize())); (based on earlier comment) I would expect this to be something like SkIRect clip = SkIRect::MakeSize(canvas.getBaseLayerSize()).makeOffset((test & TestTranslate) ? -width / 2 : 0, 0);
On 2016/06/08 13:16:55, f(malita) wrote: > https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_mac.h > File skia/ext/skia_utils_mac.h (right): > > https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... > skia/ext/skia_utils_mac.h:104: SkIRect::MakeSize(canvas->getBaseLayerSize()). > If the clip is in local coords, then the last part of the comment doesn't seem > right: one would need to also run the clip through the inverse CTM. Ugh, I knew I was right to question this. I need to think a bit broader in how we restructure these. The only place that uses this is Blink's LocalCurrentGraphicsContext. LocalCurrentGraphicsContext::LocalCurrentGraphicsContext(SkCanvas* canvas, float deviceScaleFactor, const IntRect& dirtyRect) : m_didSetGraphicsContext(false) , m_inflatedDirtyRect(ThemeMac::inflateRectForAA(dirtyRect)) , m_skiaBitLocker(canvas, m_inflatedDirtyRect, deviceScaleFactor) Since it's in Blink it wants to take in IntRect rather than SkIRect (or SkRect). There's an implicit cast function, but I can't cast it and then pass in the address. I think we want to retain the possibility of an empty rect, instead of treating that as a marker saying don't clip, but maybe that'd be worth investigating. There are ~12 sites that construct LocalCurrentGraphicsContext. Maybe the easy thing to do would be to more-or-less put back in the constructor that ccameron@ had suggested removing.
On 2016/06/08 13:31:00, tomhudson wrote: > On 2016/06/08 13:16:55, f(malita) wrote: > > > https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_mac.h > > File skia/ext/skia_utils_mac.h (right): > > > > > https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... > > skia/ext/skia_utils_mac.h:104: SkIRect::MakeSize(canvas->getBaseLayerSize()). > > If the clip is in local coords, then the last part of the comment doesn't seem > > right: one would need to also run the clip through the inverse CTM. > > Ugh, I knew I was right to question this. I need to think a bit broader in how > we restructure these. > > The only place that uses this is Blink's LocalCurrentGraphicsContext. > > LocalCurrentGraphicsContext::LocalCurrentGraphicsContext(SkCanvas* canvas, float > deviceScaleFactor, const IntRect& dirtyRect) > : m_didSetGraphicsContext(false) > , m_inflatedDirtyRect(ThemeMac::inflateRectForAA(dirtyRect)) > , m_skiaBitLocker(canvas, > m_inflatedDirtyRect, > deviceScaleFactor) > > Since it's in Blink it wants to take in IntRect rather than SkIRect (or SkRect). > There's an implicit cast function, but I can't cast it and then pass in the > address. > I think we want to retain the possibility of an empty rect, instead of treating > that as a > marker saying don't clip, but maybe that'd be worth investigating. > There are ~12 sites that construct LocalCurrentGraphicsContext. > > Maybe the easy thing to do would be to more-or-less put back in the constructor > that ccameron@ had suggested removing. How many places care about the no-user-clip case? If it's just the test I think it'd be OK to make them go through reverse-map-to-local gymnastics (and leave to comment at "is in local coordinates").
https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_mac.h File skia/ext/skia_utils_mac.h (right): https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac.h:104: SkIRect::MakeSize(canvas->getBaseLayerSize()). On 2016/06/08 13:16:54, f(malita) wrote: > If the clip is in local coords, then the last part of the comment doesn't seem > right: one would need to also run the clip through the inverse CTM. > > Do we have unit test coverage for non-identity CTM cases? Done. https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac.h:107: const SkIRect& userClipRect, On 2016/06/08 13:27:17, f(malita) wrote: > Not new to this CL, but userClipRect should be an SkRect: we convert to one > anyway, and we probably need the additional precision for local coords. Blink has always provided an IntRect. https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... File skia/ext/skia_utils_mac_unittest.mm (right): https://codereview.chromium.org/2001653002/diff/280001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac_unittest.mm:153: skia::SkiaBitLocker bitLocker(&canvas, SkIRect::MakeSize(canvas.getBaseLayerSize())); On 2016/06/08 13:27:17, f(malita) wrote: > (based on earlier comment) I would expect this to be something like > > SkIRect clip = SkIRect::MakeSize(canvas.getBaseLayerSize()).makeOffset((test & > TestTranslate) ? -width / 2 : 0, 0); Done.
Thanks for looking into this. Still LGTM % unit test clip tweak. https://codereview.chromium.org/2001653002/diff/300001/skia/ext/skia_utils_ma... File skia/ext/skia_utils_mac_unittest.mm (right): https://codereview.chromium.org/2001653002/diff/300001/skia/ext/skia_utils_ma... skia/ext/skia_utils_mac_unittest.mm:155: skia::SkiaBitLocker bitLocker(&canvas, SkIRect::MakeSize(canvas.getBaseLayerSize())); You meant to pass clip here :)
The CQ bit was checked by tomhudson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2001653002/#ps320001 (title: "Florin knows my intent better than I do")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001653002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> SkIRect clip = SkIRect::MakeSize(canvas.getBaseLayerSize()).makeOffset((test & > TestTranslate) ? -width / 2 : 0, 0); Somehow this is returning (0, 0, 0, 0) when (test & TestTranslate) is true.
On 2016/06/08 16:38:11, tomhudson wrote: > > SkIRect clip = SkIRect::MakeSize(canvas.getBaseLayerSize()).makeOffset((test & > > TestTranslate) ? -width / 2 : 0, 0); > > Somehow this is returning (0, 0, 0, 0) when (test & TestTranslate) is true. Or not, retesting...
On 2016/06/08 16:38:48, tomhudson wrote: > On 2016/06/08 16:38:11, tomhudson wrote: > > > SkIRect clip = SkIRect::MakeSize(canvas.getBaseLayerSize()).makeOffset((test > & > > > TestTranslate) ? -width / 2 : 0, 0); > > > > Somehow this is returning (0, 0, 0, 0) when (test & TestTranslate) is true. > > Or not, retesting... Yeah, something fishy's going on, I suspect we might have uncovered a bug. My understanding of the test (TestTranslate only): * bitmap canvas [2x2] * canvas.translate(1, 0) * bitlocker(canvas, local clip: [-1, 0, 2x2]) => should be device clip: [0, 0, 2x2] => should be the same as no clip * bitlocker.GCDrawRect([0, 0, 2x2], 0xffffffff) input bitmap 0x33 0x66 0x99 0xcc expected result 0x33 0xff 0x99 0xff actual result 0x33 0x66 0x99 0xcc I can't quite see what's going on yet, if you get a chance it'd be interesting to add debug for whether we're taking the useDeviceBits_ path or not. I see releaseIfNeeded() used to apply some dubious offsetting based on the detected dirty rect previously, for the !useDeviceBits_ case.
The CQ bit was checked by tomhudson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2001653002/#ps400001 (title: "clean out debugging statements")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001653002/400001
Message was sent while issue was closed.
Description was changed from ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this" and all of the expensive code behind it which was *only* activated by unit tests. R=fmalita@chromium.org BUG=543755 ========== to ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this" and all of the expensive code behind it which was *only* activated by unit tests. R=fmalita@chromium.org BUG=543755 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this" and all of the expensive code behind it which was *only* activated by unit tests. R=fmalita@chromium.org BUG=543755 ========== to ========== Remove SkDevice, obsolete constructor We already have all the API we need to rewrite skia_utils_mac without referring to the to-be-deprecated SkBaseDevice or its BitmapPlatformDevice subclasses. Also get rid of a constructor that was marked "TODO: delete this" and all of the expensive code behind it which was *only* activated by unit tests. R=fmalita@chromium.org BUG=543755 Committed: https://crrev.com/ad6f9da1d6372d185ffe51dc62246d5e4ffa8782 Cr-Commit-Position: refs/heads/master@{#398636} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/ad6f9da1d6372d185ffe51dc62246d5e4ffa8782 Cr-Commit-Position: refs/heads/master@{#398636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
