Convert PyAuto's NavigateToURL, GetActiveTabIndex, Refresh, RefreshActiveTab, and AppendTab to the …

Convert PyAuto's NavigateToURL, GetActiveTabIndex, Refresh, RefreshActiveTab, and AppendTab to the JSON interface.

BUG=132794
TEST=chrome/test/functional/browser.py, chrome/test/functional/navigation.py


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

[craigdh, 2012-07-02 23:50:50]

I'm converting these in small chunks.

[dennis_jeffrey, 2012-07-03 00:47:04]

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:1702:
handler_map["GetActiveTabIndex"] =
should these be in browser_handler_map instead, since they apply to a particular
window?

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1082: def ReloadTab(self, windex, tab_index):
should we provide defaults of 0 for "windex" and "tab_index"?

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1112: window.
add a Returns: section to this docstring

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1125: it. Blocks until the page is loaded.
Creates a new tab at the end of the given browser window and activates it.
Blocks until the specified page is loaded.

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1130: window.
add a Returns: section to this docstring.

https://chromiumcodereview.appspot.com/10692067/diff/8001/chrome/test/functio...
File chrome/test/functional/test_basic.py (left):

https://chromiumcodereview.appspot.com/10692067/diff/8001/chrome/test/functio...
chrome/test/functional/test_basic.py:24: 
why was this removed?

[craigdh, 2012-07-03 16:48:08]

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:1702:
handler_map["GetActiveTabIndex"] =
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> should these be in browser_handler_map instead, since they apply to a
particular
> window?

Nirnimesh expressed an interest in getting rid of the separate browser map. It
seems most new hooks use the utility functions in automation_provider_json.h to
parse browser and tab arguments themselves. It makes sense to me to consistently
pass all arguments through the JSON dictionary. I'll double check with Nirnimesh
to make sure he is OK with this.

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1082: def ReloadTab(self, windex, tab_index):
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> should we provide defaults of 0 for "windex" and "tab_index"?

Done.

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1112: window.
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> add a Returns: section to this docstring

Done.

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1125: it. Blocks until the page is loaded.
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> Creates a new tab at the end of the given browser window and activates it.
> Blocks until the specified page is loaded.

Done.

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1130: window.
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> add a Returns: section to this docstring.

Done.

https://chromiumcodereview.appspot.com/10692067/diff/8001/chrome/test/functio...
File chrome/test/functional/test_basic.py (left):

https://chromiumcodereview.appspot.com/10692067/diff/8001/chrome/test/functio...
chrome/test/functional/test_basic.py:24: 
On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> why was this removed?

Done.

[dennis_jeffrey, 2012-07-03 17:07:42]

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10692067/diff/2001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:1702:
handler_map["GetActiveTabIndex"] =
On 2012/07/03 16:48:08, craigdh wrote:
> On 2012/07/03 00:47:04, dennis_jeffrey wrote:
> > should these be in browser_handler_map instead, since they apply to a
> particular
> > window?
> 
> Nirnimesh expressed an interest in getting rid of the separate browser map. It
> seems most new hooks use the utility functions in automation_provider_json.h
to
> parse browser and tab arguments themselves. It makes sense to me to
consistently
> pass all arguments through the JSON dictionary. I'll double check with
Nirnimesh
> to make sure he is OK with this.

I'm also in favor of getting rid of the browser_handler_map.  It seems like it's
not necessary.  If Nirnimesh does indeed want to do that, then the current use
of handler_map is fine.

[Nirnimesh, 2012-07-03 23:13:26]

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1065: def NavigateToURL(self, url, windex=0,
tab_index=0, navigation_count=1):
maintain the order of args. tab_index should come before windex

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1085: self._GetResultFromJSONRequest(cmd_dict,
windex=windex)
Can we return the result of the navigation in case the user would like to know?

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1102: self._GetResultFromJSONRequest(cmd_dict,
windex=windex)
windex=None here and all calls that are serviced from handler_map

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1114: """Get the index of the currently active
tab in the given browser window.
It's generally not good to rely on the concept of 'active' tab, since it might
change during a test without the user noticing.

Add a warning here to use corresponding functions where you can specify the
tab_index.

[craigdh, 2012-07-11 19:10:25]

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1065: def NavigateToURL(self, url, windex=0,
tab_index=0, navigation_count=1):
On 2012/07/03 23:13:26, Nirnimesh wrote:
> maintain the order of args. tab_index should come before windex

If you look at pyautolib.h you'll see that previously NavigateToURL took the
args in this order. I want to avoid breaking API compatibility wherever
possible, particularly for a function as widely used as NavigateToURL.

There are some other PyAuto methods that have the args in this order as well, so
apparently we haven't been very consistent in the past.

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1085: self._GetResultFromJSONRequest(cmd_dict,
windex=windex)
On 2012/07/03 23:13:26, Nirnimesh wrote:
> Can we return the result of the navigation in case the user would like to
know?

A quick grep of test/functional/* indicates no one currently uses the return
value. I'd prefer to have the navigation observer SendError(), raising an
exception if the navigation fails, otherwise this call would fail silently in
most cases.

This would have the benefit of other calls that use the navigation observer
(AppendTab, RunCommand, ApplyAccelerator, etc) also raising an exception with a
good descriptive error message.

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1102: self._GetResultFromJSONRequest(cmd_dict,
windex=windex)
On 2012/07/03 23:13:26, Nirnimesh wrote:
> windex=None here and all calls that are serviced from handler_map

Done.

https://chromiumcodereview.appspot.com/10692067/diff/3010/chrome/test/pyautol...
chrome/test/pyautolib/pyauto.py:1114: """Get the index of the currently active
tab in the given browser window.
On 2012/07/03 23:13:26, Nirnimesh wrote:
> It's generally not good to rely on the concept of 'active' tab, since it might
> change during a test without the user noticing.
> 
> Add a warning here to use corresponding functions where you can specify the
> tab_index.

Done.

[Nirnimesh, 2012-07-11 22:31:07]

LGTM

[craigdh, 2012-07-12 01:04:35]

On 2012/07/11 22:31:07, Nirnimesh wrote:
> LGTM

Dennis is away this week, but indicated offline that he was good with the cl
once preferred use of handler_map vs. browser_handler_map was clarified.

[commit-bot: I haz the power, 2012-07-12 01:05:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/10692067/14001

[commit-bot: I haz the power, 2012-07-12 02:34:53]

Change committed as 146273