New pyauto hook for the translate feature.

New pyauto hook for the translate feature.

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

[Nirnimesh, 2010-07-22 07:29:39]

Haven't looked at it too carefully. Expecting large changes.
Please add a sample test in chrome/test/functional/translate.py

http://codereview.chromium.org/3026016/diff/4001/1006
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/4001/1006#newcode2638
chrome/browser/automation/automation_provider.cc:2638: void
AutomationProvider::IsPageTranslated(Browser* browser,
We should add in IsPageTranslated and CanTranslatePage  to GetBrowserInfo(), in
the 'tabs' section. We don't need separate calls, there's a lot of repeated
code. It'll save a lot of boilerplate code.
You could create a 'translate' section for each tab and fill in all info there.

http://codereview.chromium.org/3026016/diff/4001/1006#newcode2687
chrome/browser/automation/automation_provider.cc:2687: void
AutomationProvider::ClickTranslateBarTranslate(
There's a lot of code common to these translate bar action functions.
Let's club them together, something similar to PerformActionOnInfobar

http://codereview.chromium.org/3026016/diff/4001/1007
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/3026016/diff/4001/1007#newcode503
chrome/browser/automation/automation_provider.h:503: // page.
Does it wait for the translated page to load?

http://codereview.chromium.org/3026016/diff/4001/1007#newcode534
chrome/browser/automation/automation_provider.h:534: void
DismissInfoBar(Browser* browser,
I already implemented this my last CL. Look at PerformActionOnInfobar.

http://codereview.chromium.org/3026016/diff/4001/1008
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/4001/1008#newcode937
chrome/browser/automation/automation_provider_observers.cc:937:
NotificationService::AllSources());
nit: Does the source have to be AllSources? Or will
Source<TabContents>(tab_contents) suffice?

http://codereview.chromium.org/3026016/diff/4001/1008#newcode943
chrome/browser/automation/automation_provider_observers.cc:943: if (type !=
NotificationType::PAGE_TRANSLATED) {
Use DCHECK

http://codereview.chromium.org/3026016/diff/4001/1008#newcode957
chrome/browser/automation/automation_provider_observers.cc:957:
reply.SendSuccess(return_value.get());
call registrar_.RemoveAll() before returning

http://codereview.chromium.org/3026016/diff/4001/1009
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/3026016/diff/4001/1009#newcode495
chrome/browser/automation/automation_provider_observers.h:495: virtual void
Observe(NotificationType type, const NotificationSource& source,
spread args over 3 lines

[Jay Civelli, 2010-07-22 16:28:43]

http://codereview.chromium.org/3026016/diff/9001/6002
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2630
chrome/browser/automation/automation_provider.cc:2630: return translate_bar;
Nit: you could probably do without the local var and return directly
infobar->AsTranslateInfoBarDelegate()

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2695
chrome/browser/automation/automation_provider.cc:2695:
reply.SendError(error_message);
Add return or you will crash further down.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2714
chrome/browser/automation/automation_provider.cc:2714:
reply.SendError(error_message);
Add return.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2736
chrome/browser/automation/automation_provider.cc:2736:
reply.SendError(error_message);
Add return.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2758
chrome/browser/automation/automation_provider.cc:2758:
reply.SendError(error_message);
Add return.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2775
chrome/browser/automation/automation_provider.cc:2775: if
(!args->GetInteger(L"bar_index", &bar_index) ||
It seems you are retrieving the TabContents in 2 places (here and in
GetTranslateBarFromDict(). May be you could factor that out in a method?

http://codereview.chromium.org/3026016/diff/9001/6004
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/9001/6004#newcode952
chrome/browser/automation/automation_provider_observers.cc:952:
reply.SendError("There was a translation error.");
What happens in this case? We will follow through and SendSuccess as well?
May be you could use a scoped_ptr<PageTranslatedObserver> deleter(this) on line
947 return here (you would also have to remove the delete this line 958).

[Alyssa, 2010-07-23 21:27:45]

http://codereview.chromium.org/3026016/diff/4001/1006
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/4001/1006#newcode2638
chrome/browser/automation/automation_provider.cc:2638: void
AutomationProvider::IsPageTranslated(Browser* browser,
On 2010/07/22 07:29:39, Nirnimesh wrote:
> We should add in IsPageTranslated and CanTranslatePage  to GetBrowserInfo(),
in
> the 'tabs' section. We don't need separate calls, there's a lot of repeated
> code. It'll save a lot of boilerplate code.
> You could create a 'translate' section for each tab and fill in all info
there.

Done.

http://codereview.chromium.org/3026016/diff/4001/1006#newcode2687
chrome/browser/automation/automation_provider.cc:2687: void
AutomationProvider::ClickTranslateBarTranslate(
On 2010/07/22 07:29:39, Nirnimesh wrote:
> There's a lot of code common to these translate bar action functions.
> Let's club them together, something similar to PerformActionOnInfobar

Done.

I kept the ClickTranslateBarTranslate separate on the pyauto side because it is
kind of a separate action. The other three are all options that you can select
from the options menu, but clicking translate actually takes the action of
translating the page and also shows the 'this page has been translated...'
infobar.

I also added the revert option (again, keeping it separate on the pyauto side).

http://codereview.chromium.org/3026016/diff/4001/1007
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/3026016/diff/4001/1007#newcode503
chrome/browser/automation/automation_provider.h:503: // page.
On 2010/07/22 07:29:39, Nirnimesh wrote:
> Does it wait for the translated page to load?

Yes. Documented this fact.

http://codereview.chromium.org/3026016/diff/4001/1007#newcode534
chrome/browser/automation/automation_provider.h:534: void
DismissInfoBar(Browser* browser,
On 2010/07/22 07:29:39, Nirnimesh wrote:
> I already implemented this my last CL. Look at PerformActionOnInfobar.

Removed.

http://codereview.chromium.org/3026016/diff/4001/1008
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/4001/1008#newcode937
chrome/browser/automation/automation_provider_observers.cc:937:
NotificationService::AllSources());
On 2010/07/22 07:29:39, Nirnimesh wrote:
> nit: Does the source have to be AllSources? Or will
> Source<TabContents>(tab_contents) suffice?

Done.

http://codereview.chromium.org/3026016/diff/4001/1008#newcode943
chrome/browser/automation/automation_provider_observers.cc:943: if (type !=
NotificationType::PAGE_TRANSLATED) {
On 2010/07/22 07:29:39, Nirnimesh wrote:
> Use DCHECK

Done.

http://codereview.chromium.org/3026016/diff/4001/1008#newcode957
chrome/browser/automation/automation_provider_observers.cc:957:
reply.SendSuccess(return_value.get());
On 2010/07/22 07:29:39, Nirnimesh wrote:
> call registrar_.RemoveAll() before returning

Done.

http://codereview.chromium.org/3026016/diff/4001/1009
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/3026016/diff/4001/1009#newcode495
chrome/browser/automation/automation_provider_observers.h:495: virtual void
Observe(NotificationType type, const NotificationSource& source,
On 2010/07/22 07:29:39, Nirnimesh wrote:
> spread args over 3 lines

Done.

http://codereview.chromium.org/3026016/diff/9001/6002
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2630
chrome/browser/automation/automation_provider.cc:2630: return translate_bar;
On 2010/07/22 16:28:43, Jay Civelli wrote:
> Nit: you could probably do without the local var and return directly
> infobar->AsTranslateInfoBarDelegate()

Done.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2695
chrome/browser/automation/automation_provider.cc:2695:
reply.SendError(error_message);
On 2010/07/22 16:28:43, Jay Civelli wrote:
> Add return or you will crash further down.

Done.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2714
chrome/browser/automation/automation_provider.cc:2714:
reply.SendError(error_message);
On 2010/07/22 16:28:43, Jay Civelli wrote:
> Add return.

Done.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2736
chrome/browser/automation/automation_provider.cc:2736:
reply.SendError(error_message);
On 2010/07/22 16:28:43, Jay Civelli wrote:
> Add return.

Done.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2758
chrome/browser/automation/automation_provider.cc:2758:
reply.SendError(error_message);
On 2010/07/22 16:28:43, Jay Civelli wrote:
> Add return.

Done.

http://codereview.chromium.org/3026016/diff/9001/6002#newcode2775
chrome/browser/automation/automation_provider.cc:2775: if
(!args->GetInteger(L"bar_index", &bar_index) ||
On 2010/07/22 16:28:43, Jay Civelli wrote:
> It seems you are retrieving the TabContents in 2 places (here and in
> GetTranslateBarFromDict(). May be you could factor that out in a method?

Done.

http://codereview.chromium.org/3026016/diff/9001/6004
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/9001/6004#newcode952
chrome/browser/automation/automation_provider_observers.cc:952:
reply.SendError("There was a translation error.");
On 2010/07/22 16:28:43, Jay Civelli wrote:
> What happens in this case? We will follow through and SendSuccess as well?
> May be you could use a scoped_ptr<PageTranslatedObserver> deleter(this) on
line
> 947 return here (you would also have to remove the delete this line 958).

This issue got taken away with the changes I made.

I realized that this probably shouldn't automatically error out if there was a
translation error. That might make the tests pretty flaky. So I chose to return
a flag indicating the success or failure of the translation and the tests will
re-try on error (a few times).

[Jay Civelli, 2010-07-23 22:01:30]

LGTM

http://codereview.chromium.org/3026016/diff/26001/27003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/26001/27003#newcode954
chrome/browser/automation/automation_provider_observers.cc:954:
registrar_.RemoveAll();
Nit: I don't think you need to do that.

http://codereview.chromium.org/3026016/diff/26001/27003#newcode1005
chrome/browser/automation/automation_provider_observers.cc:1005:
registrar_.RemoveAll();
Nit: same as above

[Nirnimesh, 2010-07-24 00:02:48]

LGTM, mostly small comments

http://codereview.chromium.org/3026016/diff/30001/31001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2665
chrome/browser/automation/automation_provider.cc:2665: // Get the TabContents
from the dictionary of arguments.
nit: "the dictionary" -> "a dictionary"

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2666
chrome/browser/automation/automation_provider.cc:2666: TabContents*
GetTabContentsFromDict(const Browser& browser,
nit: Keep it consistent. pass const Browser* & const DictionaryValue*

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2677
chrome/browser/automation/automation_provider.cc:2677: *error_message = "No tab
at that index.";
*error_message = StringPrintf("No tab at index %d", tab_index)

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2683
chrome/browser/automation/automation_provider.cc:2683: // Get the
TranslateInfoBarDelegate from the TabContents.
nit: "the TabContents" -> "TabContents"

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2691
chrome/browser/automation/automation_provider.cc:2691: // If there was no
translate info bar delegate, return null.
replace with: "no translate infobar". the "return null" part is evident

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2725
chrome/browser/automation/automation_provider.cc:2725:
Details<std::string>(&language));
Function returns without calling either SendError or SendSuccess.
Will cause DCHECK crash in AutomationJSONReply's destructor.

Fix: replace reply.SendError(..) above with AutomationJSONReply(this,
reply_message).SendError(..)

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2758
chrome/browser/automation/automation_provider.cc:2758: if (option == "never
translate language") {
maybe we should underscore these strings
never_translate_language, never_translate_site, ...

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2778
chrome/browser/automation/automation_provider.cc:2778:
translate_bar->Translate();
this success condition will cause DCHECK crash for the same reason as above

http://codereview.chromium.org/3026016/diff/30001/31003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/30001/31003#newcode946
chrome/browser/automation/automation_provider_observers.cc:946:
AutomationJSONReply reply(automation_, reply_message_);
merge this declaration into 955

http://codereview.chromium.org/3026016/diff/30001/31006
File chrome/test/functional/translate.py (right):

http://codereview.chromium.org/3026016/diff/30001/31006#newcode11
chrome/test/functional/translate.py:11: """Tests that translate works
correctly"""
"Tests for translate"

"works correctly" is evident

http://codereview.chromium.org/3026016/diff/30001/31006#newcode13
chrome/test/functional/translate.py:13: spanish_google =
'http://www.google.com/webhp?hl=es'
member vars begin with _

http://codereview.chromium.org/3026016/diff/30001/31006#newcode26
chrome/test/functional/translate.py:26: def _NavigateAndWaitForBar(self, url):
_NavigateAndWaitForTranslateBar

http://codereview.chromium.org/3026016/diff/30001/31006#newcode31
chrome/test/functional/translate.py:31: """Since the translate can fail due to
server error, continue trying until
mention what is "too many times"?
or pass as arg

http://codereview.chromium.org/3026016/diff/30001/31006#newcode35
chrome/test/functional/translate.py:35: while curr_try < max_tries and not
self.ClickTranslateBarTranslate():
This is ugly, python wise

for _ in range(max_tries):
  if self.ClickTranslateBarTranslate():
    return
self.fail(...)

http://codereview.chromium.org/3026016/diff/30001/31006#newcode42
chrome/test/functional/translate.py:42:
self._NavigateAndWaitForBar(self.spanish_google)
assert that translate infobar is there

http://codereview.chromium.org/3026016/diff/30001/31006#newcode43
chrome/test/functional/translate.py:43: self._ClickTranslateUntilSuccess()
assert that translation was successful

http://codereview.chromium.org/3026016/diff/30001/31006#newcode131
chrome/test/functional/translate.py:131:
self.assertTrue(translate_info['can_translate_page'])
match something in page contents too

http://codereview.chromium.org/3026016/diff/30001/31007
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/3026016/diff/30001/31007#newcode635
chrome/test/pyautolib/pyauto.py:635: def GetTranslateInfo(self, tab_index=0,
window_index=0):
s/window_inde/windex/ for consistency

http://codereview.chromium.org/3026016/diff/30001/31007#newcode661
chrome/test/pyautolib/pyauto.py:661: """If the translate bar is showing, clicks
the 'Translate' button on the
Keep first sentence short and sweet. "Click Translate button"

Details in next paragraph.

Mention that this waits until translation is done (I know it's obvious since it
returns the translation error, but still)

http://codereview.chromium.org/3026016/diff/30001/31007#newcode669
chrome/test/pyautolib/pyauto.py:669: True if the translation was successful or
false if there was an error.
True, if translation successful
False, otherwise. Note tht...

http://codereview.chromium.org/3026016/diff/30001/31007#newcode685
chrome/test/pyautolib/pyauto.py:685: """Select the 'Show original' button on 
the 'this page has been
same comment about function docstring as above

http://codereview.chromium.org/3026016/diff/30001/31007#newcode715
chrome/test/pyautolib/pyauto.py:715: page. See ClickTranslateBarTranslate and
PerformActioOnInfobar to do those.
>80chars

[Alyssa, 2010-07-26 17:46:29]

Thanks for the comments!

http://codereview.chromium.org/3026016/diff/26001/27003
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3026016/diff/26001/27003#newcode954
chrome/browser/automation/automation_provider_observers.cc:954:
registrar_.RemoveAll();
On 2010/07/23 22:01:30, Jay Civelli wrote:
> Nit: I don't think you need to do that.

Done.

http://codereview.chromium.org/3026016/diff/26001/27003#newcode1005
chrome/browser/automation/automation_provider_observers.cc:1005:
registrar_.RemoveAll();
On 2010/07/23 22:01:30, Jay Civelli wrote:
> Nit: same as above

Done.

http://codereview.chromium.org/3026016/diff/30001/31001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2665
chrome/browser/automation/automation_provider.cc:2665: // Get the TabContents
from the dictionary of arguments.
On 2010/07/24 00:02:49, Nirnimesh wrote:
> nit: "the dictionary" -> "a dictionary"

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2666
chrome/browser/automation/automation_provider.cc:2666: TabContents*
GetTabContentsFromDict(const Browser& browser,
On 2010/07/24 00:02:49, Nirnimesh wrote:
> nit: Keep it consistent. pass const Browser* & const DictionaryValue*

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2677
chrome/browser/automation/automation_provider.cc:2677: *error_message = "No tab
at that index.";
On 2010/07/24 00:02:49, Nirnimesh wrote:
> *error_message = StringPrintf("No tab at index %d", tab_index)

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2683
chrome/browser/automation/automation_provider.cc:2683: // Get the
TranslateInfoBarDelegate from the TabContents.
On 2010/07/24 00:02:49, Nirnimesh wrote:
> nit: "the TabContents" -> "TabContents"

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2691
chrome/browser/automation/automation_provider.cc:2691: // If there was no
translate info bar delegate, return null.
On 2010/07/24 00:02:49, Nirnimesh wrote:
> replace with: "no translate infobar". the "return null" part is evident

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2725
chrome/browser/automation/automation_provider.cc:2725:
Details<std::string>(&language));
On 2010/07/24 00:02:49, Nirnimesh wrote:
> Function returns without calling either SendError or SendSuccess.
> Will cause DCHECK crash in AutomationJSONReply's destructor.
> 
> Fix: replace reply.SendError(..) above with AutomationJSONReply(this,
> reply_message).SendError(..)

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2758
chrome/browser/automation/automation_provider.cc:2758: if (option == "never
translate language") {
On 2010/07/24 00:02:49, Nirnimesh wrote:
> maybe we should underscore these strings
> never_translate_language, never_translate_site, ...

Done.

http://codereview.chromium.org/3026016/diff/30001/31001#newcode2778
chrome/browser/automation/automation_provider.cc:2778:
translate_bar->Translate();
On 2010/07/24 00:02:49, Nirnimesh wrote:
> this success condition will cause DCHECK crash for the same reason as above

Done.

http://codereview.chromium.org/3026016/diff/30001/31006
File chrome/test/functional/translate.py (right):

http://codereview.chromium.org/3026016/diff/30001/31006#newcode131
chrome/test/functional/translate.py:131:
self.assertTrue(translate_info['can_translate_page'])
On 2010/07/24 00:02:49, Nirnimesh wrote:
> match something in page contents too

Page contents (as retrieved by "view page source") don't change - they always
stay in the original language.