Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716073002:
Remove unused branch in native theme win painting (Closed)
|
Created:
3 years, 10 months ago by enne (OOO) Modified:
3 years, 10 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, danakj Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unused branch in native theme win painting
Once impl-side painting was turned on for the browser compositor, all
paint code turned into recording instead of direct rasterizaton.
This branch is not used in practice anymore (verified by adding
a CHECK and running a win rel try job), so it can be removed.
This simplifies the cc::PaintCanvas interface and will allow for it
to be possible to make PaintCanvas and SkCanvas be different types.
In the future, it could be possible to make PaintRecord save different
ops in the recording (like drawScrollbar or some such) and then move
this direct rasterization into the compositor to regain this lost
optimization from the past.
BUG=671433
Review-Url: https://codereview.chromium.org/2716073002
Cr-Commit-Position: refs/heads/master@{#453042}
Committed: https://chromium.googlesource.com/chromium/src/+/8205fefc8bff94c075029d00eaeeeb61fbfda67b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refactor PaintIndirect; add TODO perf comment #Patch Set 3 : Fix up case statement;add comment #Messages
Total messages: 21 (16 generated)
Description was changed from ========== Remove unused branch in native theme win painting Once impl-side painting was turned on for the browser compositor, all paint code turned into recording instead of direct rasterizaton. This branch is not used in practice anymore (verified by adding a CHECK and running a win rel try job), so it can be removed. This simplifies the cc::PaintCanvas interface and will allow for it to be possible to make PaintCanvas and SkCanvas be different types. BUG=671433 ========== to ========== Remove unused branch in native theme win painting Once impl-side painting was turned on for the browser compositor, all paint code turned into recording instead of direct rasterizaton. This branch is not used in practice anymore (verified by adding a CHECK and running a win rel try job), so it can be removed. This simplifies the cc::PaintCanvas interface and will allow for it to be possible to make PaintCanvas and SkCanvas be different types. In the future, it could be possible to make PaintRecord save different ops in the recording (like drawScrollbar or some such) and then move this direct rasterization into the compositor to regain this lost optimization from the past. BUG=671433 ==========
enne@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was checked by enne@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! Can you file an appropriate bug for the optimization stuff you mention in the CL description? https://codereview.chromium.org/2716073002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2716073002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_win.cc:282: PaintIndirect(canvas, part, state, rect, extra); Nit: Now that this call is so simple, I suggest moving this up into the "default:" case above and terminating that with a return (instead of break).
On 2017/02/24 at 23:10:10, pkasting wrote: > Can you file an appropriate bug for the optimization stuff you mention in the CL description? I can file a bug, but I've become a little bit allergic to P3 feature requests, because the bug tracker will mark them as untriaged on a regular basis and create more bug tracker churn and triage work than speculative low priority feature requests merit. I've left a longer TODO in the code so that somebody who is optimizing this code in the future can immediately have some more obvious options. I think that's in the spirit of what you're asking for. https://codereview.chromium.org/2716073002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2716073002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_win.cc:282: PaintIndirect(canvas, part, state, rect, extra); On 2017/02/24 at 23:10:10, Peter Kasting wrote: > Nit: Now that this call is so simple, I suggest moving this up into the "default:" case above and terminating that with a return (instead of break). Done.
The CQ bit was checked by enne@chromium.org 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...
The CQ bit was checked by enne@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2716073002/#ps40001 (title: "Fix up case statement;add comment")
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": 40001, "attempt_start_ts": 1487987731045550,
"parent_rev": "eda2b1a7d3a149a67bc9801f126abd7666bba2fb", "commit_rev":
"8205fefc8bff94c075029d00eaeeeb61fbfda67b"}
Message was sent while issue was closed.
Description was changed from ========== Remove unused branch in native theme win painting Once impl-side painting was turned on for the browser compositor, all paint code turned into recording instead of direct rasterizaton. This branch is not used in practice anymore (verified by adding a CHECK and running a win rel try job), so it can be removed. This simplifies the cc::PaintCanvas interface and will allow for it to be possible to make PaintCanvas and SkCanvas be different types. In the future, it could be possible to make PaintRecord save different ops in the recording (like drawScrollbar or some such) and then move this direct rasterization into the compositor to regain this lost optimization from the past. BUG=671433 ========== to ========== Remove unused branch in native theme win painting Once impl-side painting was turned on for the browser compositor, all paint code turned into recording instead of direct rasterizaton. This branch is not used in practice anymore (verified by adding a CHECK and running a win rel try job), so it can be removed. This simplifies the cc::PaintCanvas interface and will allow for it to be possible to make PaintCanvas and SkCanvas be different types. In the future, it could be possible to make PaintRecord save different ops in the recording (like drawScrollbar or some such) and then move this direct rasterization into the compositor to regain this lost optimization from the past. BUG=671433 Review-Url: https://codereview.chromium.org/2716073002 Cr-Commit-Position: refs/heads/master@{#453042} Committed: https://chromium.googlesource.com/chromium/src/+/8205fefc8bff94c075029d00eaee... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8205fefc8bff94c075029d00eaee... |
