[chromium] Let PlatformContextSkia draw to a GLES2Canvas in addition to an SkCanvas

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Let PlatformContextSkia draw to a GLES2Canvas in addition to an SkCanvas

[James Robinson]

Created attachment 62729 [details]
Patch

[WebKit Review Bot]

Attachment 62729 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/PlatformContextSkia.cpp:780:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Comment on attachment 62729 [details]
Patch

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:706
 +                      m_canvas->getDevice()->width(), m_canvas->getDevice()->height()};
width and height already computed (above).

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:733
 +              // Last drawn in software; upload everything we've drawn
WebKit style doesn't say whether comments should be full sentences ending in periods or not, but you should probably pick one...

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:737
 +              // the hardware stuff.
... or the other.  :)

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:749
 +              int height = m_canvas->getDevice()->height();
width & height superfluous here.

WebCore/platform/graphics/skia/PlatformContextSkia.h:50
 +  #endif
Not your fault, but we should probably move PlatformContextSkia into the WebCore namespace at some point.

WebCore/platform/graphics/skia/PlatformContextSkia.h:197
 +      void forceToSoftware() const;
forceToSoftware() is kind of awkwardly-named.  Suggest updateBuffer() or syncBuffers() or syncCanvas() or syncSoftwareCanvas() or ...

[James Robinson]

Created attachment 62739 [details]
Patch

[James Robinson]

Went for full sentences (or something approximating them) and syncSoftwareCanvas().  Please take another look :)

[WebKit Review Bot]

Attachment 62739 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/skia/PlatformContextSkia.cpp:775:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 62739 [details]
Patch

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:679
 +  void PlatformContextSkia::setGLES2Context(WebCore::GLES2Context* context, const WebCore::IntSize& size)
This code should really be within the WebCore namespace :)

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:212
 +      , m_GPUCanvas(0)
style-nit: m_gpuCanvas <- acronym at the start of a term should be lowercase.

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:687
 +      if (m_useGPU) {
nit: consider returning early if !m_useGPU to reduce indentation of the interesting
part of this function.  of course, if you don't like this style for the shorter
methods and prefer consistency, then ignore this nit.

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:689
 +              // Depending on the blend mode we need to one of a few things:
nit: "need to _do_ one..."

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:744
 +          if (m_backingStoreState == Hardware) {
nit: no brackets around single line statements

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:762
 +      // FIXME: Keep a texture around for this rather than constantly creating/destroying  one.
nit: "destroying  one"  <- one space instead of two

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:775
 +      WTF::OwnArrayPtr<uint32_t> buf(new uint32_t[width]);
ordinarily people drop the WTF:: prefix for OwnArrayPtr.  the header
file has a using WTF::OwnArrayPtr.

R=me

[James Robinson]

Thanks for the review.  I think we will move the code that manages the two backing stores to some common location and refactor it to not be skia-specific at some point in the future, but for now this is expedient.

Fixed the nits locally, will land by hand.

[James Robinson]

Committed r64161: <http://trac.webkit.org/changeset/64161>