Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1384233003:
Improve usefulness of webglcontextcreationerror statusMessage (Closed)
|
Created:
5 years, 2 months ago by sivag Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove usefulness of webglcontextcreationerror.
Renamed blink::WebGLInfo to WebGraphicsContext3DInfo.
Added helper functions to process, extract and format GPU
information, error messages in blink/chromium.
Added new gpu data types like gpu process crash count,
sandboxed etc.Changed layout tests to validate gpu info
collected on webglcontextcreationerror statusMessage.
Append fake device and vendor id when they are 0, at
present layout tests skips gpu information loading.
BUG=534710
Committed: https://crrev.com/cb671452edad08a56ac2dae720c22e559380f6dc
Cr-Commit-Position: refs/heads/master@{#355485}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 6
Patch Set 8 : Updated tests and refactored code. #Patch Set 9 : #Patch Set 10 : Allow GPU data loading for shell. #Patch Set 11 : Resetnotification strategy should be uint. #
Total comments: 22
Patch Set 12 : Addressed review comments. #Patch Set 13 : Fix for build issue. #Patch Set 14 : Build fix in chromium. #Patch Set 15 : #Patch Set 16 : #
Total comments: 14
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : Rebasing TOT. #Patch Set 21 : #Patch Set 22 : #
Total comments: 13
Patch Set 23 : #Patch Set 24 : #
Total comments: 2
Patch Set 25 : #Messages
Total messages: 74 (30 generated)
siva.gunturi@samsung.com changed reviewers: + kbr@chromium.org, sievers@chromium.org
ptal..
I don't like the change of vendorId and deviceId to strings. Let's please keep them as numbers. https://codereview.chromium.org/1384233003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1384233003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:555: if (glInfo.vendorId.isEmpty()) { Could you please figure out some way to do the number -> hex conversion on the Blink side, and fix the tests of glInfo.vendorId and glInfo.deviceId to look for 0 here?
siva.gunturi@samsung.com changed reviewers: + tkent@chromium.org
ptal.. https://codereview.chromium.org/1384233003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1384233003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:555: if (glInfo.vendorId.isEmpty()) { On 2015/10/06 23:33:15, Ken Russell wrote: > Could you please figure out some way to do the number -> hex conversion on the > Blink side, and fix the tests of glInfo.vendorId and glInfo.deviceId to look for > 0 here? When i tested on my linux machine, deviceid and vendorid are coming zero, but chrome://gpu displays the deviceid and vendorid correctly. Need to check why GPUInfo is holding null for this data.
Thanks for revising this CL, but what state is it in? You're indicating that it doesn't seem to work correctly on Linux? Could you expand the layout tests that force a context creation error to cover more of the fields that are now provided? https://codereview.chromium.org/1384233003/diff/120001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/120001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1020: "failed"); Please combine this and the previous line. https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:133: bool lenovoDcute; I think the lenovoDcute flag doesn't provide any useful information and could be removed. https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:140: WebString displayLinkVersion; Similarly for the displayLinkVersion.
siva.gunturi@samsung.com changed reviewers: + piman@chromium.org
ptal.. https://codereview.chromium.org/1384233003/diff/120001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/120001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1020: "failed"); On 2015/10/12 23:52:42, Ken Russell wrote: > Please combine this and the previous line. Done. https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:133: bool lenovoDcute; On 2015/10/12 23:52:42, Ken Russell wrote: > I think the lenovoDcute flag doesn't provide any useful information and could be > removed. Done. https://codereview.chromium.org/1384233003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:140: WebString displayLinkVersion; On 2015/10/12 23:52:43, Ken Russell wrote: > Similarly for the displayLinkVersion. Done.
The CQ bit was checked by kbr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/200001
kbr@chromium.org changed reviewers: + zmo@chromium.org
Thanks, this generally seems god -- again, what is the status of this? Is there still a problem with the device and vendor IDs being zero? +zmo. Mo, could you please review this too? https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1051: gpu_channel_host.get()); I think this code should just return NULL here, rather than only in the situation where contextFailed is true. That way the calling code doesn't need to test to see whether "testFailContext" is set, but only deal with the situation where the returned context is NULL. What do you think? https://codereview.chromium.org/1384233003/diff/200001/content/shell/app/shel... File content/shell/app/shell_main_delegate.cc (left): https://codereview.chromium.org/1384233003/diff/200001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); Slightly concerned about this change since it may break the virtual machine based bots. Why is this necessary? https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:30: if(key == "DEVICE" && (value == "0x0000" || value == "")) Space after "if". https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:31: error = error +"DeviceID is null."; Add space after "+". https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:32: if(key == "VENDOR" && (value == "0x0000" || value == "")) Space after "if". https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:34: if((key != "GL_VENDOR" || Space after "if". https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:42: key != "ErrorMessage") && (value == "")) Please use an open-brace after the parenthesis here. The multi-line if-statement is very hard to read and understand that there's a single-line statement after it. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:45: if(error.length) Space after "if".
(god -> good.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1051: gpu_channel_host.get()); On 2015/10/15 21:30:28, Ken Russell wrote: > I think this code should just return NULL here, rather than only in the > situation where contextFailed is true. That way the calling code doesn't need to > test to see whether "testFailContext" is set, but only deal with the situation > where the returned context is NULL. What do you think? +1 https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:43: error = error + "Gpu data is incorrect."; This condition is true if key == "DEVICE" or "VENDOR" and value is "". I don't think that's intentional, is it? https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:601: virtual WebGraphicsContext3D* createOffscreenGraphicsContext3D(const WebGraphicsContext3D::Attributes&, WebGraphicsContext3D* shareContext, WebGraphicsContext3D::WebGraphicsContext3DInfo& glInfo) { return nullptr; } I don't like the new name. It's misleading that this info is about the 3D context, but it is not. Can it just be WebGraphicsInfo or GraphicsInfo.
Also, why did http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... fail compilation with this patch?
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/300001
siva.gunturi@samsung.com changed reviewers: + caitkp@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
siva.gunturi@samsung.com changed reviewers: + jochen@chromium.org - caitkp@chromium.org
siva.gunturi@samsung.com changed reviewers: + sky@chromium.org - jochen@chromium.org
ptal.. @sievers/piman -> content/ @kbr/zmo -> webgl/ @tkent -> third_party/Webkit/ @sky -> components/html_viewer/ https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/200001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1051: gpu_channel_host.get()); On 2015/10/15 21:30:28, Ken Russell wrote: > I think this code should just return NULL here, rather than only in the > situation where contextFailed is true. That way the calling code doesn't need to > test to see whether "testFailContext" is set, but only deal with the situation > where the returned context is NULL. What do you think? Done. https://codereview.chromium.org/1384233003/diff/200001/content/shell/app/shel... File content/shell/app/shell_main_delegate.cc (left): https://codereview.chromium.org/1384233003/diff/200001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); On 2015/10/15 21:30:28, Ken Russell wrote: > Slightly concerned about this change since it may break the virtual machine > based bots. Why is this necessary? when we run layout test, we skip collecting vendorid, device id due to below code https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp.... https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:30: if(key == "DEVICE" && (value == "0x0000" || value == "")) On 2015/10/15 21:30:28, Ken Russell wrote: > Space after "if". Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:31: error = error +"DeviceID is null."; On 2015/10/15 21:30:28, Ken Russell wrote: > Add space after "+". Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:32: if(key == "VENDOR" && (value == "0x0000" || value == "")) On 2015/10/15 21:30:28, Ken Russell wrote: > Space after "if". Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:34: if((key != "GL_VENDOR" || On 2015/10/15 21:30:28, Ken Russell wrote: > Space after "if". Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:42: key != "ErrorMessage") && (value == "")) On 2015/10/15 21:30:28, Ken Russell wrote: > Please use an open-brace after the parenthesis here. The multi-line if-statement > is very hard to read and understand that there's a single-line statement after > it. Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:43: error = error + "Gpu data is incorrect."; On 2015/10/15 22:30:37, Zhenyao Mo wrote: > This condition is true if key == "DEVICE" or "VENDOR" and value is "". > > I don't think that's intentional, is it? i corrected it, it was supposed to check if one of the key other than vendor and device has a blank value. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:43: error = error + "Gpu data is incorrect."; On 2015/10/15 22:30:37, Zhenyao Mo wrote: > This condition is true if key == "DEVICE" or "VENDOR" and value is "". > > I don't think that's intentional, is it? Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:45: if(error.length) On 2015/10/15 21:30:28, Ken Russell wrote: > Space after "if". Done. https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1384233003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:601: virtual WebGraphicsContext3D* createOffscreenGraphicsContext3D(const WebGraphicsContext3D::Attributes&, WebGraphicsContext3D* shareContext, WebGraphicsContext3D::WebGraphicsContext3DInfo& glInfo) { return nullptr; } On 2015/10/15 22:30:37, Zhenyao Mo wrote: > I don't like the new name. It's misleading that this info is about the 3D > context, but it is not. Can it just be WebGraphicsInfo or GraphicsInfo. Done.
https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... File content/shell/app/shell_main_delegate.cc (left): https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); Note that removing this may slightly slows down tests that run on content shell, say blink layout tests. https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:43: error = error + "Gpu data is incorrect."; We may not always have GL strings because we only collect them on GPU process startup, and then send such info back to browser, and then to renderer. So it's likely the renderer process already starts creating a WebGL context and failed. https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:44: } nit: indentation wrong.
https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... File components/html_viewer/blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... components/html_viewer/blink_platform_impl.cc:175: blink::WebGraphicsContext3D::WebGraphicsInfo glInfo; gl_info here and 193. https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... File components/html_viewer/blink_platform_impl.h (right): https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... components/html_viewer/blink_platform_impl.h:86: blink::WebGraphicsContext3D::WebGraphicsInfo& gl_info); Not sure about blink, but in chrome style guide says that if you're going to modify a value it should be a pointer. The only time you use references in functions are if the ref is const.
Thanks, this looks fairly good overall but please address sky's review comments. Only one major comment about one of the changes. https://codereview.chromium.org/1384233003/diff/300001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/300001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1049: // purpousefully in caseof layout tests. Fix typos. "purposefully in case of" https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... File content/shell/app/shell_main_delegate.cc (left): https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); On 2015/10/16 18:46:11, Zhenyao Mo wrote: > Note that removing this may slightly slows down tests that run on content shell, > say blink layout tests. Will it have any other side-effects? Could it break tests that run on virtual machines? @sivag, is this change absolutely necessary? If not please take it out.
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/320001
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/380001
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal... https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... File components/html_viewer/blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... components/html_viewer/blink_platform_impl.cc:175: blink::WebGraphicsContext3D::WebGraphicsInfo glInfo; On 2015/10/16 20:28:50, sky wrote: > gl_info here and 193. Done. https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... File components/html_viewer/blink_platform_impl.h (right): https://codereview.chromium.org/1384233003/diff/300001/components/html_viewer... components/html_viewer/blink_platform_impl.h:86: blink::WebGraphicsContext3D::WebGraphicsInfo& gl_info); On 2015/10/16 20:28:50, sky wrote: > Not sure about blink, but in chrome style guide says that if you're going to > modify a value it should be a pointer. The only time you use references in > functions are if the ref is const. Done. https://codereview.chromium.org/1384233003/diff/300001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/300001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1049: // purpousefully in caseof layout tests. On 2015/10/16 21:44:13, Ken Russell wrote: > Fix typos. "purposefully in case of" Done. https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... File content/shell/app/shell_main_delegate.cc (left): https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); On 2015/10/16 18:46:11, Zhenyao Mo wrote: > Note that removing this may slightly slows down tests that run on content shell, > say blink layout tests. Done. https://codereview.chromium.org/1384233003/diff/300001/content/shell/app/shel... content/shell/app/shell_main_delegate.cc:161: command_line.AppendSwitch(switches::kSkipGpuDataLoading); On 2015/10/16 21:44:13, Ken Russell wrote: > On 2015/10/16 18:46:11, Zhenyao Mo wrote: > > Note that removing this may slightly slows down tests that run on content > shell, > > say blink layout tests. > > Will it have any other side-effects? Could it break tests that run on virtual > machines? > > @sivag, is this change absolutely necessary? If not please take it out. I removed this change. If the id is null, i am appending the fake id to pass this in case of tests. https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:43: error = error + "Gpu data is incorrect."; On 2015/10/16 18:46:12, Zhenyao Mo wrote: > We may not always have GL strings because we only collect them on GPU process > startup, and then send such info back to browser, and then to renderer. So it's > likely the renderer process already starts creating a WebGL context and failed. Done. https://codereview.chromium.org/1384233003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:44: } On 2015/10/16 18:46:12, Zhenyao Mo wrote: > nit: indentation wrong. Done.
Mostly looks good with a few nits fixed. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:984: return; nit: maybe DCHECK(gl_info)? because caller makes sure gl_info isn't null https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1052: // purposefully in caseof layout tests. nit: caseof -> case of https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:40: testFailed("webglcontextcreationerror test failed" + error); nit: maybe an extra space after "failed"? Otherwise you will get something like "failedDeviceID is null" https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:123: , sandboxed(false) please also initialize amdSwitchable and optimus to false.
Mo, thanks for reviewing. Could you take ownership of the review of this CL? Thanks.
On 2015/10/19 18:52:10, Ken Russell wrote: > Mo, thanks for reviewing. Could you take ownership of the review of this CL? > Thanks. Sure.
LGTM+nits for content/ but please address other reviewers' comments https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:979: static void collect3DContextInformationOnFailure( nit: this is chromium code, please use chromium style (Collect3DContextInformationOnFailure) https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:985: std::string errorMessage("OffscreenContext Creation failed, "); nit: Chromium style (error_message)
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/460001
Description was changed from ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Allow shell to collect gpu data, stop appending switch kSkipGpuDataLoading. BUG=534710 ========== to ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Append fake device and vendor id when they are NULL, as of now layout tests skips gpu information loading. BUG=534710 ==========
Description was changed from ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Append fake device and vendor id when they are NULL, as of now layout tests skips gpu information loading. BUG=534710 ========== to ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Append fake device and vendor id when they are 0, at present layout tests skips gpu information loading. BUG=534710 ==========
Description was changed from ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Append fake device and vendor id when they are 0, at present layout tests skips gpu information loading. BUG=534710 ========== to ========== Improve usefulness of webglcontextcreationerror. Renamed blink::WebGLInfo to WebGraphicsContext3DInfo. Added helper functions to process, extract and format GPU information, error messages in blink/chromium. Added new gpu data types like gpu process crash count, sandboxed etc.Changed layout tests to validate gpu info collected on webglcontextcreationerror statusMessage. Append fake device and vendor id when they are 0, at present layout tests skips gpu information loading. BUG=534710 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal.. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:979: static void collect3DContextInformationOnFailure( On 2015/10/20 01:45:04, piman (slow to review) wrote: > nit: this is chromium code, please use chromium style > (Collect3DContextInformationOnFailure) Done. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:979: static void collect3DContextInformationOnFailure( On 2015/10/20 01:45:04, piman (slow to review) wrote: > nit: this is chromium code, please use chromium style > (Collect3DContextInformationOnFailure) Done. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:984: return; On 2015/10/19 17:44:48, Zhenyao Mo wrote: > nit: maybe DCHECK(gl_info)? because caller makes sure gl_info isn't null Done. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:985: std::string errorMessage("OffscreenContext Creation failed, "); On 2015/10/20 01:45:04, piman (slow to review) wrote: > nit: Chromium style (error_message) Done. https://codereview.chromium.org/1384233003/diff/420001/content/renderer/rende... content/renderer/renderer_blink_platform_impl.cc:1052: // purposefully in caseof layout tests. On 2015/10/19 17:44:48, Zhenyao Mo wrote: > nit: caseof -> case of Done. https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/webgl/webgl-error-response.html:40: testFailed("webglcontextcreationerror test failed" + error); On 2015/10/19 17:44:48, Zhenyao Mo wrote: > nit: maybe an extra space after "failed"? Otherwise you will get something like > "failedDeviceID is null" Done. https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:123: , sandboxed(false) On 2015/10/19 17:44:48, Zhenyao Mo wrote: > please also initialize amdSwitchable and optimus to false. Done.
LGTM.
third_party/WebKit/public/ lgtm https://codereview.chromium.org/1384233003/diff/460001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/460001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:119: : vendorId(0) nit: C++11 non-static member initializer would simplify the code.
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@sky , ptal.. https://codereview.chromium.org/1384233003/diff/460001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1384233003/diff/460001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3D.h:119: : vendorId(0) On 2015/10/20 23:08:32, tkent wrote: > nit: C++11 non-static member initializer would simplify the code. Done.
LGTM
The CQ bit was checked by siva.gunturi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, tkent@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1384233003/#ps480001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384233003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384233003/480001
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/cb671452edad08a56ac2dae720c22e559380f6dc Cr-Commit-Position: refs/heads/master@{#355485} |
