Add basic UI tests for dom_ui/options.

Add basic UI tests for dom_ui/options.

BUG=none
TEST=none

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

[csilv, 2010-06-21 17:38:55]

Basic UI tests for options_ui.cc.  Modeled after similar UI tests for the
bookmarks manager.

[Paweł Hajdan Jr., 2010-06-21 17:43:04]

Drive-by. I remember reviewing a similar CL. I think that we should really
handle IDC_OPTIONS in a different, more correct way, similar to IDC_DOWNLOADS
and IDC_HISTORY.

Please let me take another look before committing.

http://codereview.chromium.org/2848012/diff/5/4002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/5/4002#newcode57
chrome/browser/dom_ui/options_ui_uitest.cc:57:
ASSERT_TRUE(browser->WaitForTabToBecomeActive(1, action_max_timeout_ms()));
I think this is used as a method of waiting for RunCommand(IDC_OPTIONS) to
finish. I'm worried it may be error prone. Could you rather move the waiting to
the AutomationProvider?

[Evan Stade, 2010-06-21 19:39:22]

http://codereview.chromium.org/2848012/diff/5/4002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/5/4002#newcode57
chrome/browser/dom_ui/options_ui_uitest.cc:57:
ASSERT_TRUE(browser->WaitForTabToBecomeActive(1, action_max_timeout_ms()));
On 2010/06/21 17:43:04, Paweł Hajdan Jr. wrote:
> I think this is used as a method of waiting for RunCommand(IDC_OPTIONS) to
> finish. I'm worried it may be error prone. Could you rather move the waiting
to
> the AutomationProvider?

RunCommand is already supposed to be synchronous. (RunCommandAsync is not).
Check out AutomationProvider::ExecuteBrowserCommand. You will have to extend
ExecuteBrowserCommandObserver::CreateAndRegisterObserver.

[Paweł Hajdan Jr., 2010-06-21 19:42:41]

http://codereview.chromium.org/2848012/diff/5/4002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/5/4002#newcode57
chrome/browser/dom_ui/options_ui_uitest.cc:57:
ASSERT_TRUE(browser->WaitForTabToBecomeActive(1, action_max_timeout_ms()));
On 2010/06/21 19:39:22, Evan Stade wrote:
> RunCommand is already supposed to be synchronous. (RunCommandAsync is not).
> Check out AutomationProvider::ExecuteBrowserCommand. You will have to extend
> ExecuteBrowserCommandObserver::CreateAndRegisterObserver.

The default "synchronous-ness" of RunCommand means that it returns after the
command has been dispatched. It does not mean that the browser has executed what
we mean by the command, which is clearly the case with IDC_HISTORY and
IDC_DOWNLOADS.

This is quite tricky (in a bad sense), so I'd prefer to get this right.

The verification would be pretty simple. If we can remove the
WaitForTabToBecomeActive call, and the test is not flaky, then the current code
is fine. However, I bet it's not.

[Evan Stade, 2010-06-21 19:45:14]

http://codereview.chromium.org/2848012/diff/5/4002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/5/4002#newcode57
chrome/browser/dom_ui/options_ui_uitest.cc:57:
ASSERT_TRUE(browser->WaitForTabToBecomeActive(1, action_max_timeout_ms()));
On 2010/06/21 19:42:41, Paweł Hajdan Jr. wrote:
> On 2010/06/21 19:39:22, Evan Stade wrote:
> > RunCommand is already supposed to be synchronous. (RunCommandAsync is not).
> > Check out AutomationProvider::ExecuteBrowserCommand. You will have to extend
> > ExecuteBrowserCommandObserver::CreateAndRegisterObserver.
> 
> The default "synchronous-ness" of RunCommand means that it returns after the
> command has been dispatched. It does not mean that the browser has executed
what
> we mean by the command, which is clearly the case with IDC_HISTORY and
> IDC_DOWNLOADS.
> 
> This is quite tricky (in a bad sense), so I'd prefer to get this right.

Not so. Again, I refer you to AutomationProvider::ExecuteBrowserCommand

[Paweł Hajdan Jr., 2010-06-21 19:47:45]

http://codereview.chromium.org/2848012/diff/5/4002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/5/4002#newcode57
chrome/browser/dom_ui/options_ui_uitest.cc:57:
ASSERT_TRUE(browser->WaitForTabToBecomeActive(1, action_max_timeout_ms()));
On 2010/06/21 19:45:14, Evan Stade wrote:
> Not so. Again, I refer you to AutomationProvider::ExecuteBrowserCommand

Oh, sorry. Seems that I didn't read the end of your comment. Yeah, extending
ExecuteBrowserCommandObserver::CreateAndRegisterObserver would be the right
thing to do.

[csilv, 2010-06-22 23:20:35]

I've made changes to the this CL that should be consistent with Pawel's request.
 Namely, I tweaked automation_provider_observers to treat IDC_OPTIONS like it
treats similar commands (DOWNLOADS, HISTORY).

So now RunCommand(IDC_OPTIONS) will wait for a new tab to be created.  In cases
where we expect that a tab will not be created (for example, issuing IDC_OPTIONS
when there is already an options tab open), I use RunCommandAsync instead.

The Mac/Win trybots are being finiky right now, but I'm fairly confident that
this CL is good.  Comments appreciated.

[James Hawkins, 2010-06-22 23:31:43]

http://codereview.chromium.org/2848012/diff/12001/13002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/12001/13002#newcode5
chrome/browser/dom_ui/options_ui_uitest.cc:5: #include
"chrome/test/ui/ui_test.h"
nit: This should be inserted in the list of includes, in the alphabetically
sorted position.

http://codereview.chromium.org/2848012/diff/12001/13002#newcode15
chrome/browser/dom_ui/options_ui_uitest.cc:15: class OptionsUITest : public
UITest {
nit: wrap all of this in an unnamed namespace.

http://codereview.chromium.org/2848012/diff/12001/13002#newcode19
chrome/browser/dom_ui/options_ui_uitest.cc:19:
launch_arguments_.AppendSwitch(switches::kEnableTabbedOptions);
Add a TODO that says this line should be removed when we enable tabbed options
by default.

[csilv, 2010-06-23 00:01:05]

http://codereview.chromium.org/2848012/diff/12001/13002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/12001/13002#newcode5
chrome/browser/dom_ui/options_ui_uitest.cc:5: #include
"chrome/test/ui/ui_test.h"
On 2010/06/22 23:31:44, James Hawkins wrote:
> nit: This should be inserted in the list of includes, in the alphabetically
> sorted position.

Done.

http://codereview.chromium.org/2848012/diff/12001/13002#newcode15
chrome/browser/dom_ui/options_ui_uitest.cc:15: class OptionsUITest : public
UITest {
On 2010/06/22 23:31:44, James Hawkins wrote:
> nit: wrap all of this in an unnamed namespace.

Done.

http://codereview.chromium.org/2848012/diff/12001/13002#newcode19
chrome/browser/dom_ui/options_ui_uitest.cc:19:
launch_arguments_.AppendSwitch(switches::kEnableTabbedOptions);
On 2010/06/22 23:31:44, James Hawkins wrote:
> Add a TODO that says this line should be removed when we enable tabbed options
> by default.

Done.

[James Hawkins, 2010-06-23 00:42:57]

LGTM

[Paweł Hajdan Jr., 2010-06-23 06:33:36]

This looks much better now, thanks. Let's try to clean it up a bit more, see two
minor comments.

http://codereview.chromium.org/2848012/diff/10002/24001
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/2848012/diff/10002/24001#newcode687
chrome/browser/automation/automation_provider_observers.cc:687: {IDC_OPTIONS,
NotificationType::TAB_CONTENTS_TITLE_UPDATED},
nit: Please keep the alphabetical ordering of IDC_ "constants".

http://codereview.chromium.org/2848012/diff/10002/24002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/10002/24002#newcode25
chrome/browser/dom_ui/options_ui_uitest.cc:25: // tab->GetCurrentURL is not up
to date.
It seems it was a hack to work around the tab possibly not being fully loaded.
After the proper change for IDC_OPTIONS, this should now work right away. Could
you try just verifying the title, and removing the DOM automation bits?

[csilv, 2010-06-23 17:45:14]

http://codereview.chromium.org/2848012/diff/10002/24001
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/2848012/diff/10002/24001#newcode687
chrome/browser/automation/automation_provider_observers.cc:687: {IDC_OPTIONS,
NotificationType::TAB_CONTENTS_TITLE_UPDATED},
On 2010/06/23 06:33:37, Paweł Hajdan Jr. wrote:
> nit: Please keep the alphabetical ordering of IDC_ "constants".

Done.

http://codereview.chromium.org/2848012/diff/10002/24002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/10002/24002#newcode25
chrome/browser/dom_ui/options_ui_uitest.cc:25: // tab->GetCurrentURL is not up
to date.
This is used in the LoadOptionsByURL test where we test that we can navigate to
the chrome://options URL.  So for that test, we are not using RunCommand.  Can
you suggest an alternate method for performing that test?

[Paweł Hajdan Jr., 2010-06-23 17:59:41]

http://codereview.chromium.org/2848012/diff/10002/24002
File chrome/browser/dom_ui/options_ui_uitest.cc (right):

http://codereview.chromium.org/2848012/diff/10002/24002#newcode25
chrome/browser/dom_ui/options_ui_uitest.cc:25: // tab->GetCurrentURL is not up
to date.
On 2010/06/23 17:45:15, Chris Silverberg wrote:
> This is used in the LoadOptionsByURL test where we test that we can navigate
to
> the chrome://options URL.  So for that test, we are not using RunCommand.  Can
> you suggest an alternate method for performing that test?

So, it seems you just want to verify the URL. Well, you can choose to verify the
tab title as in other tests, or check the active tab's url. For that, you can
use UITest's GetActiveTabURL or GetCurrentURL from TabProxy.

[csilv, 2010-06-23 18:34:18]

Thx for the feedback.  Changes made, please take a look.

[Paweł Hajdan Jr., 2010-06-23 18:39:47]

LGTM