Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241473002:
Delete quickRejectY() (Closed)
DescriptionDelete quickRejectY()
This is the first step in a refactor of quickReject().
TBR=reed@google.com
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002
Committed: https://skia.googlesource.com/skia/+/6372e6590924c459d88fa1fae52ff996a56ee9ab
Patch Set 1 #
Total comments: 2
Patch Set 2 : Inline quickRejectY logic #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 29 (17 generated)
Description was changed from ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). The current users of quickRejectY() are not performance critical. BUG=skia: ========== to ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). The current users of quickRejectY() are not performance critical. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ==========
The CQ bit was checked by msarett@google.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...
msarett@google.com changed reviewers: + mtklein@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/2241473002/diff/1/src/core/SkPicturePlayback.cpp File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/2241473002/diff/1/src/core/SkPicturePlayback.... src/core/SkPicturePlayback.cpp:426: if (!canvas->quickReject(src) && paint) { Lets write own own check here -- should be trivial, and much faster than calling back into quickReject... e.g. src = getClipBounds if (top < src.bottom && bottom > src.top) draw
Description was changed from ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). The current users of quickRejectY() are not performance critical. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ========== to ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ==========
https://codereview.chromium.org/2241473002/diff/1/src/core/SkPicturePlayback.cpp File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/2241473002/diff/1/src/core/SkPicturePlayback.... src/core/SkPicturePlayback.cpp:426: if (!canvas->quickReject(src) && paint) { On 2016/08/11 16:49:08, reed1 wrote: > Lets write own own check here -- should be trivial, and much faster than calling > back into quickReject... > > e.g. > > src = getClipBounds > if (top < src.bottom && bottom > src.top) > draw Of course, thanks. Done.
lgtm I'd go as far as to drop the check entirely and just make the call.
The CQ bit was checked by msarett@google.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...
On 2016/08/11 16:57:36, mtklein wrote: > lgtm > > I'd go as far as to drop the check entirely and just make the call. Let's consider that in a follow-up? There are a few scrolling text benches that I don't want to impact.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/11 17:00:57, msarett wrote: > On 2016/08/11 16:57:36, mtklein wrote: > > lgtm > > > > I'd go as far as to drop the check entirely and just make the call. > > Let's consider that in a follow-up? There are a few scrolling text benches that > I don't want to impact. Sure (but this code never runs in any bench).
The CQ bit was checked by mtklein@google.com
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: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
On 2016/08/11 17:00:57, msarett wrote: > On 2016/08/11 16:57:36, mtklein wrote: > > lgtm > > > > I'd go as far as to drop the check entirely and just make the call. > > Let's consider that in a follow-up? There are a few scrolling text benches that > I don't want to impact. Ahh ok, never mind. I don't feel strongly.
Description was changed from ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ========== to ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). TBR=reed@google.com BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ==========
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). TBR=reed@google.com BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 ========== to ========== Delete quickRejectY() This is the first step in a refactor of quickReject(). TBR=reed@google.com BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241473002 Committed: https://skia.googlesource.com/skia/+/6372e6590924c459d88fa1fae52ff996a56ee9ab ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/6372e6590924c459d88fa1fae52ff996a56ee9ab
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
