Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
645613002:
[WebGL-chromium] Return meaningful information in WebGL context creation error message (Closed)
Description[WebGL] Return meaningful information in WebGL context creation error message.
For example, vendor, renderer, driver version, etc.This
way a developer can gather data on what platforms the app
fails to run. This needs changes on both blink and chromium.
Below steps are followed to implement this
1. Add basic headers in blink.
2. Add the necessary apis in chromium.
3. Start using this in WebglRenderingContxt.cpp.
This patch represents step-2 in this process.
BUG=412440
Committed: https://crrev.com/90f19fbfc25603d942ee4fe525cde30e5e28fbbb
Cr-Commit-Position: refs/heads/master@{#302390}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Removed changes in public files. #Patch Set 4 : Dnt alter public api, created new api. #
Total comments: 4
Patch Set 5 : Move the main logic to argument with glinfo. #
Messages
Total messages: 29 (11 generated)
siva.gunturi@samsung.com changed reviewers: + piman@chromium.org, sievers@chromium.org
sievers, piman ptal.. connecting blink patch @https://codereview.chromium.org/640293002/
sievers@chromium.org changed reviewers: + kbr@chromium.org
+kbr
https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.cc:932: blink::WebGLInfo glInfo; nit: gl_info https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.cc:940: blink::WebGLInfo& glInfo) { nit: gl_info https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.h:134: blink::WebGLInfo& gl_info); As mentioned in the blink patch, use a pointer, not non-const references.
Please see your Blink CL for comment about avoiding even temporarily breaking the overrides of these methods.
ptal.. https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.cc:932: blink::WebGLInfo glInfo; On 2014/10/10 20:25:00, piman (Very slow to review) wrote: > nit: gl_info Done. https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.cc:940: blink::WebGLInfo& glInfo) { On 2014/10/10 20:25:00, piman (Very slow to review) wrote: > nit: gl_info Done. https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/645613002/diff/20001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.h:134: blink::WebGLInfo& gl_info); On 2014/10/10 20:25:00, piman (Very slow to review) wrote: > As mentioned in the blink patch, use a pointer, not non-const references. Done.
https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:932: blink::WebGLInfo gl_info; Why create a WebGLInfo if we're going to discard it? https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:944: GpuChannelHost* gpu_channel_host = render_thread->GetGpuChannel(); This could be NULL, or a different channel than the one that was created/used when creating the context (l.979), e.g. on the first creation or after a GPU process loss. It could lead to inconsistent data. How about: moving this code after l.981 in the other overload (passing gl_info to that one and making this one a trivial wrapper).
ptal.. https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:932: blink::WebGLInfo gl_info; On 2014/10/27 19:43:47, piman (Very slow to review) wrote: > Why create a WebGLInfo if we're going to discard it? Done. Sorry i missed this. https://codereview.chromium.org/645613002/diff/140001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:944: GpuChannelHost* gpu_channel_host = render_thread->GetGpuChannel(); On 2014/10/27 19:43:47, piman (Very slow to review) wrote: > This could be NULL, or a different channel than the one that was created/used > when creating the context (l.979), e.g. on the first creation or after a GPU > process loss. It could lead to inconsistent data. > > How about: moving this code after l.981 in the other overload (passing gl_info > to that one and making this one a trivial wrapper). Done.
lgtm
LGTM too FWIW.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645613002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/10/29 03:32:15, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_gn_rel on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) You'll need to land the Blink-side change describing the WebGLInfo type, and have Blink roll into Chromium, before landing this one.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645613002/160001
The CQ bit was unchecked by siva.gunturi@samsung.com
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645613002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siva.gunturi@samsung.com
The CQ bit was unchecked by siva.gunturi@samsung.com
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645613002/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/90f19fbfc25603d942ee4fe525cde30e5e28fbbb Cr-Commit-Position: refs/heads/master@{#302390} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
