Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1463573002:
publish canvas context API stat metrics (Closed)
|
Created:
5 years, 1 month ago by zmin Modified:
5 years, 1 month ago CC:
chromium-reviews, krit, dshwang, drott+blinkwatch_chromium.org, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, pdr+graphicswatchlist_chromium.org, jbroman, danakj, blink-reviews-platform-graphics_chromium.org, dglazkov+blink, Rik, f(malita), asvitkine+watch_chromium.org, blink-reviews, vmpstr+blinkwatch_chromium.org, Stephen Chennney, rwlbuis, Zhenyao Mo, Ken Russell (switch to Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionpublish canvas context API usage metrics
Update Canvas.ContextType in histograms.xml to match the enum in C++ file.
demo link:
https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA
BUG=303068
Committed: https://crrev.com/06d32e3b68c141a641d40545b037ee9b9f713b49
Cr-Commit-Position: refs/heads/master@{#361006}
Patch Set 1 #
Total comments: 20
Patch Set 2 : cr #
Total comments: 9
Patch Set 3 : cr #
Total comments: 1
Patch Set 4 : xml file change #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 30 (11 generated)
zmin@chromium.org changed reviewers: + junov@chromium.org
unit test will be uploaded when it's done.
https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:121: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreated); ContextCreated -> CanvasCreated https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:659: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithGPUAccelerated); GPUAccelerated2DCanvasImageBufferCreated https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:662: CanvasMetrics::countCanvasContextStat(CanvasMetrics::GPUContextCreationFailed); GPUAccelerated2DCanvasImageBufferCreationFailed https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:670: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithDisplayList); DisplayList2DCanvasImageBufferCreated https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:675: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithSoftware); Too early to count this, we don't know if it will succeed yet. Should be Unaccelerated2DCanvasImageBufferCreated https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:678: CanvasMetrics::countCanvasContextStat(CanvasMetrics::SoftwareContextCreationFailed); Unaccelerated2DCanvasImageBufferCreationFailed https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:382: CanvasMetrics::countCanvasContextStat(CanvasMetrics::GPUContextLost); Accelerated2DCanvasGPUContextLost https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/CanvasMetrics.h (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/CanvasMetrics.h:23: * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. This is the wrong header. In new files we just use this: http://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:99: CanvasMetrics::countCanvasContextStat(CanvasMetrics::DisplayListFallbackToRaster); DisplayList2DCanvasFallbackToRaster https://codereview.chromium.org/1463573002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:55457: + <int value="0" label="ContextsCreated"/> update names based on earlier comments.
Description was changed from ========== publish canvas context API stat metrics demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ========== to ========== publish canvas context API stat metrics demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ==========
zmin@chromium.org changed reviewers: + rkaplow@chromium.org
zmin@chromium.org changed reviewers: + zmo@chromium.org
https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:121: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreated); On 2015/11/19 20:36:08, Justin Novosad wrote: > ContextCreated -> CanvasCreated Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:659: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithGPUAccelerated); On 2015/11/19 20:36:08, Justin Novosad wrote: > GPUAccelerated2DCanvasImageBufferCreated Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:662: CanvasMetrics::countCanvasContextStat(CanvasMetrics::GPUContextCreationFailed); On 2015/11/19 20:36:08, Justin Novosad wrote: > GPUAccelerated2DCanvasImageBufferCreationFailed Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:670: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithDisplayList); On 2015/11/19 20:36:08, Justin Novosad wrote: > DisplayList2DCanvasImageBufferCreated Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:675: CanvasMetrics::countCanvasContextStat(CanvasMetrics::ContextCreatedWithSoftware); On 2015/11/19 20:36:08, Justin Novosad wrote: > Too early to count this, we don't know if it will succeed yet. Should be > Unaccelerated2DCanvasImageBufferCreated Done. Also change this to if (!surface->isValid()) { CanvasMetrics....Failed); } else { CanvasMetrics...Created); } https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:678: CanvasMetrics::countCanvasContextStat(CanvasMetrics::SoftwareContextCreationFailed); On 2015/11/19 20:36:08, Justin Novosad wrote: > Unaccelerated2DCanvasImageBufferCreationFailed Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:382: CanvasMetrics::countCanvasContextStat(CanvasMetrics::GPUContextLost); On 2015/11/19 20:36:08, Justin Novosad wrote: > Accelerated2DCanvasGPUContextLost Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/CanvasMetrics.h (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/CanvasMetrics.h:23: * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. On 2015/11/19 20:36:09, Justin Novosad wrote: > This is the wrong header. In new files we just use this: > http://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/1463573002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:99: CanvasMetrics::countCanvasContextStat(CanvasMetrics::DisplayListFallbackToRaster); On 2015/11/19 20:36:09, Justin Novosad wrote: > DisplayList2DCanvasFallbackToRaster Done. https://codereview.chromium.org/1463573002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:55457: + <int value="0" label="ContextsCreated"/> On 2015/11/19 20:36:09, Justin Novosad wrote: > update names based on earlier comments. Done.
https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:137: CanvasMetrics::countCanvasContextStat(CanvasMetrics::CanvasContextCreated); This should be more specific: I'd like 2DContextCreated, but C++ symbols can only start with letters, so Canvas2DContextCreated would be okay https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:114: CanvasMetrics::countCanvasContextStat(CanvasMetrics::CanvasContextCreated); Can we make this one more specific: WebGLContextCreated https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55451: <enum name="CanvasContextType" type="int"> Looks like someone else had the idea to count context creations, but this does not appear to be referenced anywhere. I think it should be removed along with the associated Canvas.ContextType histogram earlier in this file. https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51285: + <owner>zmin@chromium.org</owner> Since your team assignment is not set in stone yet, better add my name here, just in case.
zmin@chromium.org changed reviewers: - zmo@chromium.org
Description was changed from ========== publish canvas context API stat metrics demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ========== to ========== publish canvas context API stat metrics Update Canvas.ContextType in histograms.xml to match the enum in C++ file. demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ==========
Remove @zmo as the WebGL2RenderingContextBase.cpp is reverted. cc @kbr as I add him into the owner of Canvas.ContextType. https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:137: CanvasMetrics::countCanvasContextStat(CanvasMetrics::CanvasContextCreated); On 2015/11/20 03:30:44, Justin Novosad wrote: > This should be more specific: I'd like 2DContextCreated, but C++ symbols can > only start with letters, so Canvas2DContextCreated would be okay Removed. https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1463573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:114: CanvasMetrics::countCanvasContextStat(CanvasMetrics::CanvasContextCreated); On 2015/11/20 03:30:44, Justin Novosad wrote: > Can we make this one more specific: WebGLContextCreated Removed. https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55451: <enum name="CanvasContextType" type="int"> On 2015/11/20 03:30:44, Justin Novosad wrote: > Looks like someone else had the idea to count context creations, but this does > not appear to be referenced anywhere. I think it should be removed along with > the associated Canvas.ContextType histogram earlier in this file. Talk with @junov offline. We will remove CanvasContextCreated for CanvasContextStat as it's duplicated with Canvas.ContextType. Also update the Cnvas.ContextType to match the enum in C++ file. https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51285: + <owner>zmin@chromium.org</owner> On 2015/11/20 03:30:44, Justin Novosad wrote: > Since your team assignment is not set in stone yet, better add my name here, > just in case. Done. https://codereview.chromium.org/1463573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51285: + <owner>zmin@chromium.org</owner> On 2015/11/20 03:30:44, Justin Novosad wrote: > Since your team assignment is not set in stone yet, better add my name here, > just in case. Done.
lgtm for everything under thrid_party/WebKit https://codereview.chromium.org/1463573002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1463573002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55453: - <int value="1" label="webkit-3d"/> I'm not sure we should remove this even though it is no longer used. We may have historical data that references this value. Deferring to rkaplow who knows more about how this stuff works.
On 2015/11/20 17:30:27, Justin Novosad wrote: > lgtm for everything under thrid_party/WebKit > > https://codereview.chromium.org/1463573002/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (left): > > https://codereview.chromium.org/1463573002/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:55453: - <int value="1" > label="webkit-3d"/> > I'm not sure we should remove this even though it is no longer used. We may have > historical data that references this value. Deferring to rkaplow who knows more > about how this stuff works. Talk with @junov and @asvitkine offline. It's a better idea to leave the entry with obsolete tag because it makes historical data more clear on the dashboard.
Still lgtm, Ping rkaplow
zmin@chromium.org changed reviewers: + jwd@chromium.org - rkaplow@chromium.org
https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51288: + <summary>The State of Canvas 2D Context API.</summary> Can you mention when this is logged.
https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51285: +<histogram name="WebCore.CanvasContextStat" enum="CanvasContextStat"> nit: state not stat?
https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51285: +<histogram name="WebCore.CanvasContextStat" enum="CanvasContextStat"> Actually, maybe change to CanvasContextUsageStats. https://codereview.chromium.org/1463573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51288: + <summary>The State of Canvas 2D Context API.</summary> On 2015/11/20 22:07:05, Jesse Doherty wrote: > Can you mention when this is logged. Something like "Logged when the particular api is used"?
Description was changed from ========== publish canvas context API stat metrics Update Canvas.ContextType in histograms.xml to match the enum in C++ file. demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ========== to ========== publish canvas context API usage metrics Update Canvas.ContextType in histograms.xml to match the enum in C++ file. demo link: https://drive.google.com/corp/drive/u/0/folders/0BySFHAS13VhbaWJzbUh4dXRERjA BUG=303068 ==========
Hi Jesse, Can you take a look again. Thanks, Owen
kbr@chromium.org changed reviewers: + kbr@chromium.org
Seems fine. LGTM
metrics LGTM
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/1463573002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1463573002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1463573002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/06d32e3b68c141a641d40545b037ee9b9f713b49 Cr-Commit-Position: refs/heads/master@{#361006} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
