Tests for bug 5988.

Tests for bug 5988.
BUG=5988
TEST=ResourceDispatcherTest.DynamicTitle*


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

[shinyak (Google), 2011-06-01 01:51:01]

I have created automated tests for Issue 5988.

I have used Sleep to wait for opening windows, but I don't know this is an
appropriate approach. If there is a better way, could you tell me?
Also, though I have added test in resouce_dispatcher_host_uitest, if you have
more preferred test source, could you tell me it too?

[brettw, 2011-06-01 01:58:56]

+pawel for the sleep timeout behavior.

http://codereview.chromium.org/7080049/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host_uitest.cc (right):

http://codereview.chromium.org/7080049/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:467: // Create
dynamical popup.
dynamical -> dynamic

[shinyak (Google), 2011-06-01 02:03:46]

fixed typo.

[Paweł Hajdan Jr., 2011-06-01 07:23:44]

Thanks for adding me. Sleep is *never* the right solution, it's good that you
are aware of it. :-)

http://codereview.chromium.org/7080049/diff/2002/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_uitest.cc (right):

http://codereview.chromium.org/7080049/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:22: #include
"chrome/test/ui_test_utils.h"
nit: Is this used?

http://codereview.chromium.org/7080049/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:433:
scoped_refptr<WindowProxy> window2(browser_proxy2->GetWindow());
WindowProxy are discouraged, do you really need it?

The problem with WindowProxy is that it uses OS calls that might behave in
unexpected ways. I'm not sure if GetWindowTitle is one of those, but to be safe
I just recommend avoiding WindowProxy unless you absolutely have to use it.

http://codereview.chromium.org/7080049/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:436:
base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms());
Are you trying to wait for the load to finish or for the window to appear?

Please consider writing a _browser_ test instead, there are many functions in
ui_test_utils that could help you (like WaitForNavigationInCurrentTab or
similar).

There's also WaitForNavigation for UI tests, but it's a bit more ugly. I don't
really recommend it.

If you just need to wait for a window to appear, you can use
WaitForWindowCountToBecome.

If my guesses don't really cover your scenario, please explain in more detail
what you need to wait for (I'm just not familiar with every part of the browser,
especially the WebKit side), and I'll see if we have facilities for waiting for
those kinds of events or suggest how to add one.

[shinyak (Google), 2011-06-01 13:11:40]

Pawel,

thank you for the comment.

I want to test the title of the window opened by javascript (window.open()). I
searched for the method to get *window* title in BrowserProxy and TabProxy, but
they don't have it. So I used WindowProxy. GetTabTitle() in TabProxy didn't
return what I wanted.

If you know how to handle a window opened by window.open(), could you tell me?

[Paweł Hajdan Jr., 2011-06-01 13:47:20]

On 2011/06/01 13:11:40, shinyak wrote:
> I want to test the title of the window opened by javascript (window.open()). I
> searched for the method to get *window* title in BrowserProxy and TabProxy,
but
> they don't have it. So I used WindowProxy. GetTabTitle() in TabProxy didn't
> return what I wanted.

Okay, looks like you have to use WindowProxy. This should be fine, just please
let me know if you see any weird behavior related to that.

How about the rest of the questions, especially what events do you need to wait
for?

[shinyak (Google), 2011-06-02 06:51:32]

Hi,

I've tried writing browsertest instead of ui test, however, I could not create a
popup...
Do you know how to do it? Before, I defined OpenPopup() in HTML file, and I
called it using using tab->ExecuteAndExtractString(...). In this time, I tried
ui_test_utils::ExecuteJavaScript(render_view_host, L"", L"OpenPopup()") to call
the function, but it didn't work.

I have not tried WaitForNavigation or something like that yet, but it should
work I guess...

[Paweł Hajdan Jr., 2011-06-02 06:59:07]

On 2011/06/02 06:51:32, shinyak wrote:
> Do you know how to do it? Before, I defined OpenPopup() in HTML file, and I
> called it using using tab->ExecuteAndExtractString(...). In this time, I tried
> ui_test_utils::ExecuteJavaScript(render_view_host, L"", L"OpenPopup()") to
call
> the function, but it didn't work.

"It didn't work is quite vague". Have you tried to see how other calls to
ui_test_utils::ExecuteJavaScript are handled? My guess is that you need to run
the message loop.

The body of the browser test is a task that executes on the browser's main
thread. That's different than UI test where your test runs in a separate
executable. So if you're on the browser's main thread, you may need to say "now
run other tasks", otherwise they won't run at all.

> I have not tried WaitForNavigation or something like that yet, but it should
> work I guess...

Just please try to find a test that does a similar thing. cs.chromium.org may
help.

[shinyak (Google), 2011-06-02 07:14:09]

Pawel,

Is there some method to wait for the window title change? When I call
window->GetWindowTitle without sleep, an old title is returned.

[shinyak (Google), 2011-06-02 07:25:33]

I found ErrorPageTest::WaitForTitleMatching, but its implementation is just a
loop of get and sleep...

[Paweł Hajdan Jr., 2011-06-02 08:03:42]

On 2011/06/02 07:14:09, shinyak wrote:
> Is there some method to wait for the window title change? When I call
> window->GetWindowTitle without sleep, an old title is returned.

Are you waiting for the navigation to finish?

About the Sleeping code in ErrorPageTest - yeah, we have some old pieces that
still do it the wrong way. We should not do that in new code.

[shinyak (Google), 2011-06-02 09:28:30]

Let me try to explain it again.

"The JavaScript didn't work" means the execution was stuck and ExecuteJavaScript
did not return until timeout.

> Are you waiting for the navigation to finish?
Actually I'm waiting for JavaScript execution to finish (because the JavaScript
changes title).

Let me copy and paste my code here.

IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
                       DynamicTitle1) {
  // EnableDOMAutomation();
  ASSERT_TRUE(test_server()->Start());

  GURL url(test_server()->GetURL("files/dynamic1.html"));
  ui_test_utils::NavigateToURL(browser(), url);

  string16 title = browser()->GetWindowTitleForCurrentTab();
  cout << title << endl;

  // Create dynamic popup.                                                      
                                                                                
                                                                           
  EXPECT_TRUE(ui_test_utils::ExecuteJavaScript(render_view_host(),
                                               L"",
                                               L"OpenPopup();"));

  cout << "executed." << endl;

  // TODO: test should be added here.                                           
                                                                                
                                                                           
}

[Paweł Hajdan Jr., 2011-06-02 11:34:05]

I think you need to call EnableDOMAutomation(); and then proxy the call through
window.domAutomationController. Just try to look for examples in existing tests.

[shinyak (Google), 2011-06-02 13:17:40]

On 2011/06/02 11:34:05, Paweł Hajdan Jr. wrote:
> I think you need to call EnableDOMAutomation(); and then proxy the call
through
> window.domAutomationController. Just try to look for examples in existing
tests.

I did call EnabledDOMAutomation(); and tried the following code. But the
execution was stuck at ExecuteJavaScriptString.. Did I do something wrong...?
Since I though OpenPopup() might do something wrong, I tried removing
OpenPopup(), but the execution was also stuck...

 // Create dynamic popup.                                                       
                                                                                
                                                                          
  std::string result;
  EXPECT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractString(
      render_view_host(),
      L"",
      L"OpenPopup(); window.domAutomationController.send('xyzzy')",
      &result));

[Paweł Hajdan Jr., 2011-06-02 14:15:07]

Ken, Ilya: I'm not very familiar with DOM automation controller, and I think you
were working on that code. Could you help with writing tests for this CL?

[kkania, 2011-06-02 14:24:10]

Sure. Looking through past comments, you mentioned
that ui_test_utils::ExecuteJavaScript(render_view_host, L"", L"OpenPopup()")
did not work for you. Perhaps this is simply because that func appends
'window.domAutomationController.send(0);' to the end, and you don't have a
semicolon after OpenPopup()?  Did you open the debugger during the test to
see if you got any errors?  If that's the reason, you might want to change
ExecuteJavaScript to wrap the given script in a anonymous function that is
executed, or just put a semicolon after the user script...

On Thu, Jun 2, 2011 at 7:15 AM, <phajdan.jr@chromium.org> wrote:

> Ken, Ilya: I'm not very familiar with DOM automation controller, and I
> think you
> were working on that code. Could you help with writing tests for this CL?
>
>
> http://codereview.chromium.org/7080049/
>

[shinyak (Google), 2011-06-03 04:05:07]

OK, I added semicolon just after the OpenPopup(). However, the execution is
still stuck.
I found  the error message that 

[27408:27408:0603/125847:2145009136426:INFO:CONSOLE(1)] "Uncaught TypeError:
Cannot call method 'setAutomationId' of 
undefined", source:  (1)

So I guess dom automation is not enabled though I called EnableDOMAutomation()
at the first of the test function...

[Paweł Hajdan Jr., 2011-06-03 06:11:10]

On 2011/06/03 04:05:07, shinyak wrote:
> OK, I added semicolon just after the OpenPopup(). However, the execution is
> still stuck.
> I found  the error message that 
> 
> [27408:27408:0603/125847:2145009136426:INFO:CONSOLE(1)] "Uncaught TypeError:
> Cannot call method 'setAutomationId' of 
> undefined", source:  (1)
> 
> So I guess dom automation is not enabled though I called EnableDOMAutomation()
> at the first of the test function...

Ouchhh, that's too late. You need to call it in the test fixture's constructor.
I think it would be much easier to help you if you could post the full version
of the code. Looks like a few misunderstandings already happened.

[shinyak (Google), 2011-06-03 07:27:43]

AAAAAAAAHHHHHHH!!!

Thank you! I didn't know that! Now the execution is not stuck!!!

> Ouchhh, that's too late. You need to call it in the test fixture's
constructor.
> I think it would be much easier to help you if you could post the full version
> of the code. Looks like a few misunderstandings already happened.

[shinyak (Google), 2011-06-04 05:11:32]

All, thank you for reviewing.

I have created browser test instead of ui test finally.

[Paweł Hajdan Jr., 2011-06-04 09:33:54]

Much better, thanks! Just a few minor comments.

http://codereview.chromium.org/7080049/diff/14001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/14001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:27: // See
bug 5988
nit: It's useful to put a URL instead of just a number, i.e.
http://crbug.com/5988

http://codereview.chromium.org/7080049/diff/14001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:40:
Browser* popup = ui_test_utils::WaitForBrowserNotInSet(excluded);
Why not just WaitForNewBrowser? If that'd have a race condition, how about using
a WindowedNotificationObserver with NotificationType::BROWSER_WINDOW_READY?

http://codereview.chromium.org/7080049/diff/14001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:45:
ASSERT_EQ(string16::size_type(0),
nit: Please use << to make the actual title a part of the error message, like
this:

ASSERT_EQ(...) << title;

[shinyak (Google), 2011-06-06 05:21:09]

Pawel,

I've added bug URL in comment, and actual title in ASSERT_EQ log.

The reason I don't use ui_test_utils::WaitForNewBrowser is that it causes the
execution to be stuck. I guess an event has arrived before calling
WaitForNewBrowser, so no more event comes. WaitForBrowserNotInSet won't check
the event that a window appears, so it won't make the execution stuck.

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

On 2011/06/06 05:21:09, shinyak wrote:
> The reason I don't use ui_test_utils::WaitForNewBrowser is that it causes the
> execution to be stuck. I guess an event has arrived before calling
> WaitForNewBrowser, so no more event comes. WaitForBrowserNotInSet won't check
> the event that a window appears, so it won't make the execution stuck.

That's the use case for WindowedNotificationObserver. It starts listening for
the notification _before_ the event that triggers it. Could you switch to
WindowedNotificationObserver?

[Paweł Hajdan Jr., 2011-06-06 07:17:23]

http://codereview.chromium.org/7080049/diff/15003/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/15003/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:47: <<
"Actial title: " << title;
nit: Probably a typo, Actial -> Actual, same below.

[shinyak (Google), 2011-06-06 07:25:27]

Oops, sorry, fixed typo.

[shinyak (Google), 2011-06-06 07:27:13]

On 2011/06/06 07:25:27, shinyak wrote:
> Oops, sorry, fixed typo.

Oops, sorry I've missed your first comment.

[shinyak (Google), 2011-06-06 08:25:24]

Now using WindowedNotificationObserver.

[Paweł Hajdan Jr., 2011-06-06 09:40:47]

The waiting logic is much better now. The two tests have mostly identical code.
Could you share most of it in some test fixture methods?

Brett, I'm not sure if the current waits ensure that the load of the popup has
finished, i.e. whether its title is guaranteed to be updated. Could you take a
look?

[brettw, 2011-06-06 14:58:59]

I think we should get the title before the load completes.

[shinyak (Google), 2011-06-07 04:25:14]

I made a shared method to get a title.

[Paweł Hajdan Jr., 2011-06-07 08:28:53]

Just minor comments.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:28:
string16 ResourceDispatcherHostBrowserTest::GetPopupTitle(GURL url) {
nit: Why not const GURL& url?

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:36:
EXPECT_TRUE(ui_test_utils::ExecuteJavaScript(
When this fails, observer.Wait() below will hang. Could you make
ResourceDispatcherHostBrowserTest::GetPopupTitle return bool and use string16*
out-parameter? Then instead of EXPECT_TRUE here you'd just return false.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:45:
EXPECT_TRUE(popup);
Similarly here, if popup is false the next line will crash.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:57:
ASSERT_EQ(string16::size_type(0),
nit: I just realized it may be simpler to use StartsWith from base/string_util.

[shinyak (Google), 2011-06-07 08:54:14]

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:28:
string16 ResourceDispatcherHostBrowserTest::GetPopupTitle(GURL url) {
On 2011/06/07 08:28:55, Paweł Hajdan Jr. wrote:
> nit: Why not const GURL& url?

Done.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:36:
EXPECT_TRUE(ui_test_utils::ExecuteJavaScript(
On 2011/06/07 08:28:55, Paweł Hajdan Jr. wrote:
> When this fails, observer.Wait() below will hang. Could you make
> ResourceDispatcherHostBrowserTest::GetPopupTitle return bool and use string16*
> out-parameter? Then instead of EXPECT_TRUE here you'd just return false.

Done.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:45:
EXPECT_TRUE(popup);
On 2011/06/07 08:28:55, Paweł Hajdan Jr. wrote:
> Similarly here, if popup is false the next line will crash.

Done.

http://codereview.chromium.org/7080049/diff/6004/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:57:
ASSERT_EQ(string16::size_type(0),
On 2011/06/07 08:28:55, Paweł Hajdan Jr. wrote:
> nit: I just realized it may be simpler to use StartsWith from
base/string_util.

Done.

[Paweł Hajdan Jr., 2011-06-07 09:46:17]

LGTM with comments, thank you for the work on making this test much more solid.

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:8:
#include "chrome/browser/ui/browser_list.h"
nit: Is this #include needed?

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:62:
EXPECT_TRUE(StartsWith(title, ASCIIToUTF16("My Popup Title"), false))
nit: Are you sure you want a case-insensitive comparison? I think in the
previous patch sets it was case-sensitive.

[shinyak (Google), 2011-06-07 13:28:48]

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_browsertest.cc
(right):

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:8:
#include "chrome/browser/ui/browser_list.h"
On 2011/06/07 09:46:18, Paweł Hajdan Jr. wrote:
> nit: Is this #include needed?

Done.

http://codereview.chromium.org/7080049/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_browsertest.cc:62:
EXPECT_TRUE(StartsWith(title, ASCIIToUTF16("My Popup Title"), false))
On 2011/06/07 09:46:18, Paweł Hajdan Jr. wrote:
> nit: Are you sure you want a case-insensitive comparison? I think in the
> previous patch sets it was case-sensitive.

Done.

[commit-bot: I haz the power, 2011-06-07 13:31:30]

No LGTM from valid reviewers yet.

[shinyak (Google), 2011-06-08 12:34:06]

Pawel,

Ping?

Could you give LGTM again if the change is OK. I cannot commit it without LGTM.

[Paweł Hajdan Jr., 2011-06-08 13:13:55]

LGTM

I think it accepted my LGTM for chrome/test, but you need another approval for
chrome/ and content/

[commit-bot: I haz the power, 2011-06-08 13:18:03]

No LGTM from valid reviewers yet.

[shinyak (Google), 2011-06-09 02:34:34]

Brett,

If you think this patch is OK, could you give LGTM?
Maybe only Pawel's LGTM is not enough to commit this patch.

On 2011/06/08 13:18:03, I haz the power (commit-bot) wrote:
> No LGTM from valid reviewers yet.

[shinyak (Google), 2011-06-10 04:53:04]

Brett,

Ping?

On 2011/06/09 02:34:34, shinyak wrote:
> Brett,
> 
> If you think this patch is OK, could you give LGTM?
> Maybe only Pawel's LGTM is not enough to commit this patch.
> 
> On 2011/06/08 13:18:03, I haz the power (commit-bot) wrote:
> > No LGTM from valid reviewers yet.

[brettw, 2011-06-12 13:11:50]

Pawel's LGTM is fine.

[commit-bot: I haz the power, 2011-06-13 03:09:20]

Try job failure for 7080049-25001 on win for step pyauto_functional_tests:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-06-14 05:30:46]

Change committed as 88960