Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615493003:
use SkSurface behind gfx::Canvas (Closed)
Descriptionuse SkSurface behind gfx::Canvas
Using PlatformCanvas (previously) always paid for the cost of the native graphics context (e.g. HDC or CGContext), but there are no callers of gfx::Canvas that use those facilities.
BUG=675977
Review-Url: https://codereview.chromium.org/2615493003
Cr-Commit-Position: refs/heads/master@{#442034}
Committed: https://chromium.googlesource.com/chromium/src/+/151280976aad5ebe2285cf1b446e98ea9635600c
Patch Set 1 #Patch Set 2 : fix build #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : anon namespace #
Total comments: 2
Patch Set 5 : add ws #Patch Set 6 : rebase #Messages
Total messages: 40 (26 generated)
The CQ bit was checked by reed@google.com to run a CQ dry run
reed@google.com changed reviewers: + fmalita@chromium.org
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by reed@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by reed@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + asvitkine@chromium.org, sadrul@chromium.org
Description was changed from ========== use SkSurface behind gfx::Canvas BUG= ========== to ========== use SkSurface behind gfx::Canvas Using PlatformCanvas (previously) always paid for the cost of the native graphics context (e.g. HDC or CGContext), but there are no callers of gfx::Canvas that use those facilities. BUG=675977 ==========
https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (left): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#oldcode65 ui/gfx/canvas.cc:65: canvas_owner_ = skia::CreatePlatformCanvas(pixel_size.width(), Looks like these are the only uses of skia::CreatePlatformCanvas(). Can that be removed? https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#newcode29 ui/gfx/canvas.cc:29: sk_sp<SkSurface> create_surface(int width, int height, bool is_opaque) { Should be called 'CreateSurface'. The signature could be "(const gfx::Size& size, bool is_opaque)" https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#newcode37 ui/gfx/canvas.cc:37: } This should be in anon namespace.
non-owner LGTM, % sadrul's comments. https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.h#newcode17 ui/gfx/canvas.h:17: #include "third_party/skia/include/core/SkSurface.h" Can we forward-declare SkSurface instead?
https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (left): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#oldcode65 ui/gfx/canvas.cc:65: canvas_owner_ = skia::CreatePlatformCanvas(pixel_size.width(), On 2017/01/06 05:41:32, sadrul wrote: > Looks like these are the only uses of skia::CreatePlatformCanvas(). Can that be > removed? Good observation. Florin and I are interleaving CLs as part of the meta-effort to remove *all* of the platform-canvas infrastructure. I will attempt this removal in a follow-on CL (partly because it is being called today by unittests). https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#newcode29 ui/gfx/canvas.cc:29: sk_sp<SkSurface> create_surface(int width, int height, bool is_opaque) { On 2017/01/06 05:41:32, sadrul wrote: > Should be called 'CreateSurface'. > > The signature could be "(const gfx::Size& size, bool is_opaque)" Done. https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.cc#newcode37 ui/gfx/canvas.cc:37: } On 2017/01/06 05:41:32, sadrul wrote: > This should be in anon namespace. Done.
https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2615493003/diff/40001/ui/gfx/canvas.h#newcode17 ui/gfx/canvas.h:17: #include "third_party/skia/include/core/SkSurface.h" On 2017/01/06 14:59:07, f(malita) wrote: > Can we forward-declare SkSurface instead? Done.
The CQ bit was checked by reed@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...
lgtm https://codereview.chromium.org/2615493003/diff/60001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2615493003/diff/60001/ui/gfx/canvas.cc#newcode30 ui/gfx/canvas.cc:30: namespace { Nit: Add empty lines within the namespace.
https://codereview.chromium.org/2615493003/diff/60001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2615493003/diff/60001/ui/gfx/canvas.cc#newcode30 ui/gfx/canvas.cc:30: namespace { On 2017/01/06 16:02:51, Alexei Svitkine (slow) wrote: > Nit: Add empty lines within the namespace. Done.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2615493003/#ps80001 (title: "add ws")
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
Failed to apply patch for ui/gfx/canvas.h:
While running git apply --index -p1;
error: patch failed: ui/gfx/canvas.h:13
error: ui/gfx/canvas.h: patch does not apply
Patch: ui/gfx/canvas.h
Index: ui/gfx/canvas.h
diff --git a/ui/gfx/canvas.h b/ui/gfx/canvas.h
index
00f5b9103fc53f195d5bf3d67a697a73d2e61b5b..2bc0409c7ad01b03fc07dc00469af017ac75bd3d
100644
--- a/ui/gfx/canvas.h
+++ b/ui/gfx/canvas.h
@@ -13,12 +13,12 @@
#include "base/macros.h"
#include "base/strings/string16.h"
#include "skia/ext/platform_canvas.h"
-#include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkSurface.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/native_widget_types.h"
#include "ui/gfx/shadow_value.h"
#include "ui/gfx/text_constants.h"
-
namespace gfx {
class Rect;
@@ -506,10 +506,10 @@ class GFX_EXPORT Canvas {
float image_scale_;
// canvas_ is our active canvas object. Sometimes we are also the owner,
- // in which case canvas_owner_ will be set. Other times we are just
+ // in which case surface_ will be set. Other times we are just
// borrowing someone else's canvas, in which case canvas_ will point there
- // but canvas_owner_ will be null.
- std::unique_ptr<SkCanvas> canvas_owner_;
+ // but surface_ will be null.
+ sk_sp<SkSurface> surface_;
SkCanvas* canvas_;
DISALLOW_COPY_AND_ASSIGN(Canvas);
The CQ bit was checked by reed@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
Failed to apply patch for ui/gfx/canvas.h:
While running git apply --index -p1;
error: patch failed: ui/gfx/canvas.h:13
error: ui/gfx/canvas.h: patch does not apply
Patch: ui/gfx/canvas.h
Index: ui/gfx/canvas.h
diff --git a/ui/gfx/canvas.h b/ui/gfx/canvas.h
index
00f5b9103fc53f195d5bf3d67a697a73d2e61b5b..2bc0409c7ad01b03fc07dc00469af017ac75bd3d
100644
--- a/ui/gfx/canvas.h
+++ b/ui/gfx/canvas.h
@@ -13,12 +13,12 @@
#include "base/macros.h"
#include "base/strings/string16.h"
#include "skia/ext/platform_canvas.h"
-#include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkSurface.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/native_widget_types.h"
#include "ui/gfx/shadow_value.h"
#include "ui/gfx/text_constants.h"
-
namespace gfx {
class Rect;
@@ -506,10 +506,10 @@ class GFX_EXPORT Canvas {
float image_scale_;
// canvas_ is our active canvas object. Sometimes we are also the owner,
- // in which case canvas_owner_ will be set. Other times we are just
+ // in which case surface_ will be set. Other times we are just
// borrowing someone else's canvas, in which case canvas_ will point there
- // but canvas_owner_ will be null.
- std::unique_ptr<SkCanvas> canvas_owner_;
+ // but surface_ will be null.
+ sk_sp<SkSurface> surface_;
SkCanvas* canvas_;
DISALLOW_COPY_AND_ASSIGN(Canvas);
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2615493003/#ps100001 (title: "rebase")
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": 100001, "attempt_start_ts": 1483728125109510,
"parent_rev": "03f7edd7976c0c96aff58cdb2a85b9cb775168e5", "commit_rev":
"151280976aad5ebe2285cf1b446e98ea9635600c"}
Message was sent while issue was closed.
Description was changed from ========== use SkSurface behind gfx::Canvas Using PlatformCanvas (previously) always paid for the cost of the native graphics context (e.g. HDC or CGContext), but there are no callers of gfx::Canvas that use those facilities. BUG=675977 ========== to ========== use SkSurface behind gfx::Canvas Using PlatformCanvas (previously) always paid for the cost of the native graphics context (e.g. HDC or CGContext), but there are no callers of gfx::Canvas that use those facilities. BUG=675977 Review-Url: https://codereview.chromium.org/2615493003 Cr-Commit-Position: refs/heads/master@{#442034} Committed: https://chromium.googlesource.com/chromium/src/+/151280976aad5ebe2285cf1b446e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/151280976aad5ebe2285cf1b446e... |
