[Translate] When reloading a page Wait until some content has actually been re-loaded before trigg…

[Translate] When reloading a page Wait until some content has actually been  re-loaded before triggering a translation.

BUG=90899
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199478

[Miguel Garcia, 2013-05-07 15:38:08]

I'll have a look at the failing tests of course.

[Miguel Garcia, 2013-05-09 13:09:11]

Tests fixed, can you have a look?

[MAD, 2013-05-09 14:05:24]

LGTM... I only have nit comments about... comment... :-)

Thanks!

BYE
MAD

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.cc:609: // During a reload we need
some content to be already loaded before the
The text seem to imply that as soon as "some" content is loaded we can run, but
we actually wait until loading is done... I would rephrase the comment.

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.h:89: // Used by unit-tests to
override some defaults
Comments text should fill the line as much as possible before starting a new
line.

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.h:90: // Delay after which the
translate
Delay -> delay.

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.h:97: // Number of attempts before
waiting for a page to be full reloaded.
full -> fully

https://codereview.chromium.org/14846026/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_manager.h:210: // Max number of attempts
before checking if a page has been
Should fit on a single line.

You could also add some text about the backoff strategy which is actually
influenced by this value.

[Miguel Garcia, 2013-05-10 12:00:02]

Thanks for the quick review!

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:609: // During a reload we need
some content to be already loaded before the
On 2013/05/09 14:05:24, MAD wrote:
> The text seem to imply that as soon as "some" content is loaded we can run,
but
> we actually wait until loading is done... I would rephrase the comment.

Done.

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.h (right):

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_manager.h:89: // Used by unit-tests to
override some defaults
On 2013/05/09 14:05:24, MAD wrote:
> Comments text should fill the line as much as possible before starting a new
> line.

Done.

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_manager.h:90: // Delay after which the
translate
On 2013/05/09 14:05:24, MAD wrote:
> Delay -> delay.

Done.

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_manager.h:97: // Number of attempts before
waiting for a page to be full reloaded.
On 2013/05/09 14:05:24, MAD wrote:
> full -> fully

Done.

[commit-bot: I haz the power, 2013-05-10 12:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/14846026/15001

[Miguel Garcia, 2013-05-10 12:00:53]

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.h (right):

https://chromiumcodereview.appspot.com/14846026/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_manager.h:210: // Max number of attempts
before checking if a page has been
Done, added a case in the reload test as well

On 2013/05/09 14:05:24, MAD wrote:
> Should fit on a single line.
> 
> You could also add some text about the backoff strategy which is actually
> influenced by this value.

[commit-bot: I haz the power, 2013-05-10 14:05:27]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-10 14:29:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/14846026/15001

[commit-bot: I haz the power, 2013-05-10 14:49:55]

Change committed as 199478