Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
715413003:
Layout test for validating GLinfo on webglcontextcreationerror. (Closed)
DescriptionLayout test for validating GLinfo on Webglcontextcreationerror.
This test uses the forceNextWebGLContextCreationToFail(bool)
to fail the webgl context forcefully from JS-test runner
utility and validates the status message for GLinfo generated
by webglcontextcreationerror event.
BUG=412440
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187766
Patch Set 1 #
Total comments: 6
Patch Set 2 : Refactoring layout test. #Patch Set 3 : Fix for include issue in tests. #Patch Set 4 : Handle gpu info collection failure. #Patch Set 5 : #Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 43 (12 generated)
siva.gunturi@samsung.com changed reviewers: + kbr@chromium.org, tkent@chromium.org
ptal..
LGTM overall. Couple of minor comments. https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... File LayoutTests/compositing/webgl/webgl-error-response.html (right): https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:2: WebGL context creation error message. --> Could you please put this test in fast/canvas/webgl instead? https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:18: if (canvas.addEventListener) { addEventListener will always be available. Testing for it is unnecessary. https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:38: testRunner.dumpAsText(); This dumpAsText() call should go into initTest() for clarity.
https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... File LayoutTests/compositing/webgl/webgl-error-response.html (right): https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:2: WebGL context creation error message. --> On 2014/11/12 22:13:22, Ken Russell wrote: > Could you please put this test in fast/canvas/webgl instead? Done. https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:18: if (canvas.addEventListener) { On 2014/11/12 22:13:22, Ken Russell wrote: > addEventListener will always be available. Testing for it is unnecessary. Done. https://codereview.chromium.org/715413003/diff/1/LayoutTests/compositing/webg... LayoutTests/compositing/webgl/webgl-error-response.html:38: testRunner.dumpAsText(); On 2014/11/12 22:13:22, Ken Russell wrote: > This dumpAsText() call should go into initTest() for clarity. Done.
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/715413003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36538)
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/715413003/40001
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/715413003/40001
Looks great. Thanks for adding this test.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979)
On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) It looks like the Mac Blink bots don't have GPUs, or something similar. You should probably add more logging to the test when it fails.
On 2014/11/15 00:01:03, Ken Russell wrote: > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > mac_blink_rel on tryserver.blink > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > You should probably add more logging to the test when it fails. hi kbr, i checked this failure test on mac pc. The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not populated , its null. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... I observed Mac is having a dual gpu, and not sure how its selected . May be this needs to be fixed first , to make this test pass. Please let me know if you have any inputs on this.
On 2014/11/18 11:56:45, Sikugu wrote: > On 2014/11/15 00:01:03, Ken Russell wrote: > > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > mac_blink_rel on tryserver.blink > > > > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > > > You should probably add more logging to the test when it fails. > > hi kbr, > i checked this failure test on mac pc. > The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not > populated , its null. > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > I observed Mac is having a dual gpu, and not sure how its selected . This is on your local Mac? It looks like the machine on which your test failed (vm650-m4) is a virtual machine, likely with no GPU at all (not a dual-GPU machine). If the vendor, etc. in the GPUInfo are all null when you run this on your local machine, that means that something's wrong with the GPU info collection on Mac OS, and we'll need to fix that. > May be this needs to be fixed first , to make this test pass. > Please let me know if you have any inputs on this. This needs more debugging. Can you please dig further?
On 2014/11/18 22:23:19, Ken Russell wrote: > On 2014/11/18 11:56:45, Sikugu wrote: > > On 2014/11/15 00:01:03, Ken Russell wrote: > > > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > > > Try jobs failed on following builders: > > > > mac_blink_rel on tryserver.blink > > > > > > > > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > > > > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > > > > > You should probably add more logging to the test when it fails. > > > > hi kbr, > > i checked this failure test on mac pc. > > The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not > > populated , its null. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > I observed Mac is having a dual gpu, and not sure how its selected . > > This is on your local Mac? It looks like the machine on which your test failed > (vm650-m4) is a virtual machine, likely with no GPU at all (not a dual-GPU > machine). > > If the vendor, etc. in the GPUInfo are all null when you run this on your local > machine, that means that something's wrong with the GPU info collection on Mac > OS, and we'll need to fix that. > > > May be this needs to be fixed first , to make this test pass. > > Please let me know if you have any inputs on this. > > This needs more debugging. Can you please dig further? I will check this.
On 2014/11/19 12:59:20, Sikugu wrote: > On 2014/11/18 22:23:19, Ken Russell wrote: > > On 2014/11/18 11:56:45, Sikugu wrote: > > > On 2014/11/15 00:01:03, Ken Russell wrote: > > > > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > > > > Try jobs failed on following builders: > > > > > mac_blink_rel on tryserver.blink > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > > > > > > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > > > > > > > You should probably add more logging to the test when it fails. > > > > > > hi kbr, > > > i checked this failure test on mac pc. > > > The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not > > > populated , its null. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > I observed Mac is having a dual gpu, and not sure how its selected . > > > > This is on your local Mac? It looks like the machine on which your test failed > > (vm650-m4) is a virtual machine, likely with no GPU at all (not a dual-GPU > > machine). > > > > If the vendor, etc. in the GPUInfo are all null when you run this on your > local > > machine, that means that something's wrong with the GPU info collection on Mac > > OS, and we'll need to fix that. > > > > > May be this needs to be fixed first , to make this test pass. > > > Please let me know if you have any inputs on this. > > > > This needs more debugging. Can you please dig further? > > I will check this. Unfortunately, due to the unavailability of mac PC i am not able debug and check this. I am trying to get one such pc and will update the analysis once i check this on that.
On 2014/11/18 22:23:19, Ken Russell wrote: > On 2014/11/18 11:56:45, Sikugu wrote: > > On 2014/11/15 00:01:03, Ken Russell wrote: > > > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > > > Try jobs failed on following builders: > > > > mac_blink_rel on tryserver.blink > > > > > > > > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > > > > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > > > > > You should probably add more logging to the test when it fails. > > > > hi kbr, > > i checked this failure test on mac pc. > > The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not > > populated , its null. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > I observed Mac is having a dual gpu, and not sure how its selected . > > This is on your local Mac? It looks like the machine on which your test failed > (vm650-m4) is a virtual machine, likely with no GPU at all (not a dual-GPU > machine). > > If the vendor, etc. in the GPUInfo are all null when you run this on your local > machine, that means that something's wrong with the GPU info collection on Mac > OS, and we'll need to fix that. > > > May be this needs to be fixed first , to make this test pass. > > Please let me know if you have any inputs on this. > > This needs more debugging. Can you please dig further? I can see we are intentionally skipping the collecting gpu info for mac here https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ma... which points to crbug.com/222934.
On 2014/12/10 14:19:35, Sikugu wrote: > On 2014/11/18 22:23:19, Ken Russell wrote: > > On 2014/11/18 11:56:45, Sikugu wrote: > > > On 2014/11/15 00:01:03, Ken Russell wrote: > > > > On 2014/11/14 23:52:59, I haz the power (commit-bot) wrote: > > > > > Try jobs failed on following builders: > > > > > mac_blink_rel on tryserver.blink > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30979) > > > > > > > > It looks like the Mac Blink bots don't have GPUs, or something similar. > > > > > > > > You should probably add more logging to the test when it fails. > > > > > > hi kbr, > > > i checked this failure test on mac pc. > > > The vendor, renderer, driverversion in gpu_channel_host GPUInfo are not > > > populated , its null. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > I observed Mac is having a dual gpu, and not sure how its selected . > > > > This is on your local Mac? It looks like the machine on which your test failed > > (vm650-m4) is a virtual machine, likely with no GPU at all (not a dual-GPU > > machine). > > > > If the vendor, etc. in the GPUInfo are all null when you run this on your > local > > machine, that means that something's wrong with the GPU info collection on Mac > > OS, and we'll need to fix that. > > > > > May be this needs to be fixed first , to make this test pass. > > > Please let me know if you have any inputs on this. > > > > This needs more debugging. Can you please dig further? > > I can see we are intentionally skipping the collecting gpu info for mac here > https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ma... > which points to crbug.com/222934. Please work directly with zmo@ about this concern -- I am pretty sure that a different code path is taken when "full" GPU info collection is performed asynchronously by the browser process. It would be most helpful if you could work with your management to procure a machine on which you could test this yourself. Alternatively, you could mark this as failing in TestExpectations on Mac for the time being, which would be unfortunate, but at least would provide a certain degree of test coverage.
@kbr, ptal..
The win8_chromium_dbg and mac_chromium_dbg patch application failures are strange. Please resubmit try jobs and see if they happen again. You still have my LGTM from above. Please work with bajones@ and zmo@ over the next couple of weeks to get your Blink and Chromium patches reviewed and landed. Thanks. https://codereview.chromium.org/715413003/diff/100001/public/platform/WebGrap... File public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/715413003/diff/100001/public/platform/WebGrap... public/platform/WebGraphicsContext3D.h:67: WebString contextInfoCollectionFailure; I think you'll need to land the addition of this WebString first, then wait for Blink to roll, then land the Chromium CL which will populate it, and then land the rest of this CL adding the new test.
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/715413003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)
siva.gunturi@samsung.com changed reviewers: + pdr@chromium.org - tkent@chromium.org
@pdr, ptal @public/platform/WebGraphicsContext3D.h changes.
siva.gunturi@samsung.com changed reviewers: + philipj@opera.com
philipj, ptal @WebKit/public changes.
It looks like the Chromium-side CL is https://codereview.chromium.org/800483002/ This CL doesn't verify the message added in that CL and that CL has no tests of its own, so I think only one of the code paths added in WebGLRenderingContext.cpp is actually tested. Can you fix that? Please also fix the description so that there's no line break in what should be the first line of the resulting commit. And a space after [Webgl-blink] or no [topic] at all, since it's already very clear that this is Blink and that the commit is WebGL-related.
On 2014/12/25 13:14:29, philipj wrote: > It looks like the Chromium-side CL is https://codereview.chromium.org/800483002/ > > This CL doesn't verify the message added in that CL and that CL has no tests of > its own, so I think only one of the code paths added in > WebGLRenderingContext.cpp is actually tested. Can you fix that? > > Please also fix the description so that there's no line break in what should be > the first line of the resulting commit. And a space after [Webgl-blink] or no > [topic] at all, since it's already very clear that this is Blink and that the > commit is WebGL-related. Hi Philip, Formated the content as per your comments. Regarding test, LayoutTests/fast/canvas/webgl/webgl-error-response.html already takes care of GPUInfoCollectioFailure string check, which serves the purpouse.
On 2014/12/26 14:25:53, sikugu wrote: > Regarding test, LayoutTests/fast/canvas/webgl/webgl-error-response.html > already takes care of GPUInfoCollectioFailure string check, > which serves the purpouse. AFAICT the test only causes one failure, which means only one of the code paths can be tested.
https://codereview.chromium.org/715413003/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/715413003/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContext.cpp:92: statusMessage.append("Could not create a WebGL context."); Add a space at the end of this string or assert that glInfo.contextInfoCollectionFailure begins with a space.
On 2014/12/27 09:13:07, philipj wrote: > On 2014/12/26 14:25:53, sikugu wrote: > > Regarding test, LayoutTests/fast/canvas/webgl/webgl-error-response.html > > already takes care of GPUInfoCollectioFailure string check, > > which serves the purpouse. > > AFAICT the test only causes one failure, which means only one of the code paths > can be test Here we are returning appropriate strings based on the GPUInfo/State return status. 1.GPU failed to fetch info 2.vendor info, rendered info, driver info (returns valid or"not available" on some gpu return null strings like mac ) If our test fails to collect any of the above info we treat it as a failure. Validations for vendor info, rendered info, driver info is still a issue that needs to be fixed. On mac failures are happening for this I am planning to work on this. https://code.google.com/p/chromium/issues/detail?id=445360.
https://codereview.chromium.org/715413003/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/715413003/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContext.cpp:92: statusMessage.append("Could not create a WebGL context."); On 2014/12/27 09:13:14, philipj wrote: > Add a space at the end of this string or assert that > glInfo.contextInfoCollectionFailure begins with a space. Done.
OK, since kbr@ was happy this LGTM.
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/715413003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187766 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
