Re: Enable gpu crash tests in content (issue 10984040)

[z...@chromium.org]

Sorry for the delayed response: I am out of office for a few days.

Yes, we visit about:gpucrash in a new tab.


http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (left):

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#oldcode27
content/browser/gpu/gpu_crash_browsertest.cc:27:
chrome::SelectPreviousTab(browser);
This behavior is lost in the new test. Is there a way to add this back?

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (right):

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#newcode45
content/browser/gpu/gpu_crash_browsertest.cc:45:
IN_PROC_BROWSER_TEST_F(GPUCrashTest, MANUAL_DISABLED_Kill) {
Remove DISABLED_

http://codereview.chromium.org/10984040/

[yongsh...@intel.com]

thanks for review.

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (left):

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#oldcode27
content/browser/gpu/gpu_crash_browsertest.cc:27:
chrome::SelectPreviousTab(browser);

On 2012/10/22 22:54:33, Zhenyao Mo wrote:
> This behavior is lost in the new test. Is there a way to add this
back?

there is no function in shell to activate one.
From my tests, it can work without select another tab/shell.
If it's necessary, we need to add functions in shell to support it.

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (right):

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#newcode45
content/browser/gpu/gpu_crash_browsertest.cc:45:
IN_PROC_BROWSER_TEST_F(GPUCrashTest, MANUAL_DISABLED_Kill) {

On 2012/10/22 22:54:33, Zhenyao Mo wrote:
> Remove DISABLED_
done.

http://codereview.chromium.org/10984040/

[z...@chromium.org]

LGTM

On 2012/10/23 05:51:42, yongsheng wrote:
> thanks for review.


http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
> File content/browser/gpu/gpu_crash_browsertest.cc (left):


http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#oldcode27
> content/browser/gpu/gpu_crash_browsertest.cc:27:
> chrome::SelectPreviousTab(browser);
> On 2012/10/22 22:54:33, Zhenyao Mo wrote:
> > This behavior is lost in the new test. Is there a way to add this back?
> there is no function in shell to activate one.
> From my tests, it can work without select another tab/shell.
> If it's necessary, we need to add functions in shell to support it.

Please do so in a separate CL.

[yongsh...@intel.com]

On 2012/10/23 17:56:12, Zhenyao Mo wrote:
> LGTM

> On 2012/10/23 05:51:42, yongsheng wrote:
> > thanks for review.
> >
> >

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
> > File content/browser/gpu/gpu_crash_browsertest.cc (left):
> >
> >

http://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#oldcode27
> > content/browser/gpu/gpu_crash_browsertest.cc:27:
> > chrome::SelectPreviousTab(browser);
> > On 2012/10/22 22:54:33, Zhenyao Mo wrote:
> > > This behavior is lost in the new test. Is there a way to add this
> back?
> > there is no function in shell to activate one.
> > From my tests, it can work without select another tab/shell.
> > If it's necessary, we need to add functions in shell to support it.

> Please do so in a separate CL.

ok, i'll add a TODO in that file.
John, do you have any concerns? need your l g t m.

http://codereview.chromium.org/10984040/

[j...@chromium.org]

https://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (left):

https://codereview.chromium.org/10984040/diff/25001/content/browser/gpu/gpu_crash_browsertest.cc#oldcode27
content/browser/gpu/gpu_crash_browsertest.cc:27:
chrome::SelectPreviousTab(browser);

On 2012/10/23 05:51:42, yongsheng wrote:
> On 2012/10/22 22:54:33, Zhenyao Mo wrote:
> > This behavior is lost in the new test. Is there a way to add this
back?
> there is no function in shell to activate one.
> From my tests, it can work without select another tab/shell.
> If it's necessary, we need to add functions in shell to support it.

what does the SelectPreviousTab call give us?

https://codereview.chromium.org/10984040/diff/38001/content/browser/gpu/gpu_crash_browsertest.cc
File content/browser/gpu/gpu_crash_browsertest.cc (right):

https://codereview.chromium.org/10984040/diff/38001/content/browser/gpu/gpu_crash_browsertest.cc#newcode24
content/browser/gpu/gpu_crash_browsertest.cc:24: // for shells
why? what does this give us? i.e. I'm not sure why the test used to do
this.

https://codereview.chromium.org/10984040/

[z...@chromium.org]

Adding apatrick, the original author who wrote this test. Al, can you
explain
why we go back to previous tab after visit about:gpucrash in a second tab?

https://codereview.chromium.org/10984040/

[apat...@chromium.org]

I don't think it's necessary to do that. Presumably the test passes without?


https://codereview.chromium.org/10984040/

[yongsh...@intel.com]

I tested with content shell and it works without this.


http://codereview.chromium.org/10984040/

[z...@chromium.org]

So please remove the comments and let's land this.

http://codereview.chromium.org/10984040/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsh...@intel.com/10984040/47001


https://chromiumcodereview.appspot.com/10984040/

[commi...@chromium.org]

Presubmit check for 10984040-47001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content
content/public/test

Presubmit checks took 2.6s to calculate.



https://chromiumcodereview.appspot.com/10984040/

[yongsh...@intel.com]

On 2012/10/28 01:52:16, I haz the power (commit-bot) wrote:
> Presubmit check for 10984040-47001 failed and returned exit status 1.


> Running presubmit commit checks ...

> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].

> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
> content
> content/public/test

> Presubmit checks took 2.6s to calculate.

need John's lgtm.


http://codereview.chromium.org/10984040/