Use ExecuteJavascriptInWebFrameNotifyResult to return results even in error condition

Use the ExecuteJavascriptInWebFrameNotifyResult to return results rather than domui.

Also fixes the hanging issue by allowing result to returned always even when javascript to be executed is bogus.

BUG=86074, 85990
R=dtseng@chromium.org
TEST=browser_tests --gtest_filter=WebUIBrowserTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89286

[Sheridan Rawlins, 2011-06-14 22:43:23]

This fixes the hanging problem if the javascript is incorrect by always
returning a result to the NotificationObserver even if null (where ASSERT's will
fail).

[Sheridan Rawlins, 2011-06-15 05:28:53]

Ben, can I get your OWNERS LG for content/renderer/render_view.cc?

In the case where web_frame->executeScriptAndReturnValue() fails, the result
will be an empty v8::Handle.  Rather than hitting the assertion in v8 when
converting FromV8Value, it would be more useful to notify with a null response
so that tests waiting on the notification can recognize failure instead of
hanging.

Thanks,

-SCR

[Paweł Hajdan Jr., 2011-06-15 08:18:20]

Drive-by with testing comments (chrome/test/OWNERS).

Thank you for dealing with testing infrastructure issues, it's awesome!

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: // This fails on purpose to
test hanging case in crbug.com/85990
If this starts passing you won't get any notification. FAILS does not mean
XFAIL. Also, how long does this test take to run?

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_test_handler.cc (right):

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_test_handler.cc:15: bool is_test) {
nit: Please align this line.

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_test_handler.cc:49: is_waiting_ = true;
The new version of the code doesn't seem to use is_waiting_, could you
completely remove that member variable?

[Sheridan Rawlins, 2011-06-15 15:41:14]

Thanks for the drive by - I'll include on future CL's - your comments are
helpful!

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: // This fails on purpose to
test hanging case in crbug.com/85990
It was previously the 25s timeout, but with these changes it is now immediate!

Does gtest have a notion of XFAIL?  I don't think so - I'll update the test to
pass by doing checks instead of gtest ASSERT* here.

On 2011/06/15 08:18:21, Paweł Hajdan Jr. wrote:
> If this starts passing you won't get any notification. FAILS does not mean
> XFAIL. Also, how long does this test take to run?

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_test_handler.cc (right):

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_test_handler.cc:15: bool is_test) {
On 2011/06/15 08:18:21, Paweł Hajdan Jr. wrote:
> nit: Please align this line.

Done.

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_test_handler.cc:49: is_waiting_ = true;
Nice catch - actually is_waiting_ is needed.  I'll add it to the Observe.

On 2011/06/15 08:18:21, Paweł Hajdan Jr. wrote:
> The new version of the code doesn't seem to use is_waiting_, could you
> completely remove that member variable?

[Paweł Hajdan Jr., 2011-06-15 15:49:30]

Code I commented in the drive-by LGTM, thanks!

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7146024/diff/3007/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: // This fails on purpose to
test hanging case in crbug.com/85990
On 2011/06/15 15:41:14, Sheridan Rawlins wrote:
> It was previously the 25s timeout, but with these changes it is now immediate!

Cool!

> Does gtest have a notion of XFAIL?  I don't think so - I'll update the test to
> pass by doing checks instead of gtest ASSERT* here.

Right, I don't think it has XFAIL. FAILS has been added for Chromium in our test
suite, but it only works one way: when a test marked as FAILS fails, the failure
does not influence the exit code; however, if such a test passes, the exit code
is also not changed.

http://codereview.chromium.org/7146024/diff/7003/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: // This fails on purpose to
test hanging case in crbug.com/85990
nit: Maybe put the "This fails on purpose" right above the ASSERT_FALSE (above
too). It might look weird if later someone marks the entire test as FAILS,
FLAKY, or DISABLED (hopefully that won't happen, but let's avoid a possible
confusion).

[Ben Goodger (Google), 2011-06-15 15:55:38]

I have no idea what any of this code does.

[sky, 2011-06-15 16:47:45]

render_view change LGTM

[Sheridan Rawlins, 2011-06-15 17:08:16]

Pawel, I found a utility in gtest-spi.h documented at
http://code.google.com/p/googletest/wiki/AdvancedGuide#Catching_Failures

Which lets me make the failure a test case.

http://codereview.chromium.org/7146024/diff/7003/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7146024/diff/7003/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: // This fails on purpose to
test hanging case in crbug.com/85990
I used the gtest-spi method EXPECT_FATAL_FAILURE along with SCOPED_TRACE to make
the failure expected (and pass only when fails).

On 2011/06/15 15:49:30, Paweł Hajdan Jr. wrote:
> nit: Maybe put the "This fails on purpose" right above the ASSERT_FALSE (above
> too). It might look weird if later someone marks the entire test as FAILS,
> FLAKY, or DISABLED (hopefully that won't happen, but let's avoid a possible
> confusion).

[Paweł Hajdan Jr., 2011-06-15 17:16:20]

http://codereview.chromium.org/7146024/diff/7005/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7146024/diff/7005/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:212: static WebUIBrowserTest*
s_test_;
Hm, I'm not a fan of this s_test variable. How about switching to
EXPECT_NONFATAL_FAILURE, that allows you to reference "this" directly if I
understand correctly? Then of course you'd need to switch to EXPECT macros in
::Observe and maybe explicitly "return" there, but I think it'd still be better.

[scr, 2011-06-15 17:31:00]

Changing ASSERT to EXPECT is cumbersome inasmuch as I need to check
results to proceed. Would need to convert to if with ADD_FAILURE (not
near reference - it was something like that) and return each place I
have an assertion. I can scope the s_test_ in some way if that would
make it better.

-SCR

Sent from my iPhone

On Jun 15, 2011, at 10:16 AM, "phajdan.jr@chromium.org"
<phajdan.jr@chromium.org> wrote:

>
>
http://codereview.chromium.org/7146024/diff/7005/chrome/browser/ui/webui/web_...
> File chrome/browser/ui/webui/web_ui_browsertest.cc (right):
>
>
http://codereview.chromium.org/7146024/diff/7005/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.cc:212: static
> WebUIBrowserTest* s_test_;
> Hm, I'm not a fan of this s_test variable. How about switching to
> EXPECT_NONFATAL_FAILURE, that allows you to reference "this" directly if
> I understand correctly? Then of course you'd need to switch to EXPECT
> macros in ::Observe and maybe explicitly "return" there, but I think
> it'd still be better.
>
> http://codereview.chromium.org/7146024/

[Paweł Hajdan Jr., 2011-06-15 17:38:26]

LGTM, please just add a short comment why we need s_test.

By the way, thanks for finding EXPECT_..._FAILURE, I didn't know about it!

[commit-bot: I haz the power, 2011-06-15 18:38:44]

Presubmit check for 7146024-8005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/renderer/render_view.cc

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Sheridan Rawlins, 2011-06-15 18:43:22]

Brett,

Sky took a look at the render_view.cc change - can you LG as an OWNER?

[brettw, 2011-06-15 23:08:39]

LGTM

[commit-bot: I haz the power, 2011-06-16 01:55:48]

Change committed as 89286