check network connectivity in onPageTranslated callback.

Check network connectivity in onPageTranslated callback.

When attempting second translation keeping the WiFi/Data OFF, Chrome
shows "Translated to <desired language>" message though web page remains
untranslated.

Current fix is to check whether user is online before showing up the
translate UI in OnPageTranslated callback. 

BUG=653499
Review-Url: https://codereview.chromium.org/2822383002
Cr-Commit-Position: refs/heads/master@{#468569}
Committed: https://chromium.googlesource.com/chromium/src/+/97e2e2a80810368ef54b61ddfe1f529cdb801cf2

[renjieliu1, 2017-04-19 04:31:31]

renjieliu@chromium.org changed reviewers:
+ napper@chromium.org

[renjieliu1, 2017-04-19 06:16:15]

The CQ bit was checked by renjieliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 06:16:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 06:56:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 06:56:07]

Dry run: This issue passed the CQ dry run.

[napper, 2017-04-20 04:31:37]

Please fix CL comment to match Chrome format.

[renjieliu1, 2017-04-20 06:38:39]

Description was changed from

==========
check network connectivity in onPageTranslated callback.

BUG=653499
==========

to

==========
When attempting second translation keeping the WiFi/Data OFF, Chrome shows
"Translated to <desired language>" message though web page remains untranslated.

Current fix is to check whether user is online before showing up the translate
UI 

BUG=653499
==========

[renjieliu1, 2017-04-20 06:39:24]

On 2017/04/20 04:31:37, napper wrote:
> Please fix CL comment to match Chrome format.

Couldn't find the standard chrome cl description format but I added more details
:(

[renjieliu1, 2017-04-20 06:39:27]

On 2017/04/20 04:31:37, napper wrote:
> Please fix CL comment to match Chrome format.

Couldn't find the standard chrome cl description format but I added more details
:(

[napper, 2017-04-20 06:51:57]

On 2017/04/20 at 06:39:27, renjieliu wrote:
> On 2017/04/20 04:31:37, napper wrote:
> > Please fix CL comment to match Chrome format.
> 
> Couldn't find the standard chrome cl description format but I added more
details :(

See here for an example: https://codereview.chromium.org/2622623003

Basically the first line should be a quick summary, then a line break, then the
longer description with lines wrapped at 80 chars.

[renjieliu1, 2017-04-21 00:06:44]

Description was changed from

==========
When attempting second translation keeping the WiFi/Data OFF, Chrome shows
"Translated to <desired language>" message though web page remains untranslated.

Current fix is to check whether user is online before showing up the translate
UI 

BUG=653499
==========

to

==========
Check network connectivity in onPageTranslated callback.

When attempting second translation keeping the WiFi/Data OFF, Chrome
shows "Translated to <desired language>" message though web page remains
untranslated.

Current fix is to check whether user is online before showing up the
translate UI in OnPageTranslated callback. 

BUG=653499
==========

[renjieliu1, 2017-04-21 00:06:52]

On 2017/04/20 06:51:57, napper wrote:
> On 2017/04/20 at 06:39:27, renjieliu wrote:
> > On 2017/04/20 04:31:37, napper wrote:
> > > Please fix CL comment to match Chrome format.
> > 
> > Couldn't find the standard chrome cl description format but I added more
> details :(
> 
> See here for an example: https://codereview.chromium.org/2622623003
> 
> Basically the first line should be a quick summary, then a line break, then
the
> longer description with lines wrapped at 80 chars.

thanks!

[napper, 2017-04-23 23:31:28]

napper@chromium.org changed reviewers:
+ groby@chromium.org

[napper, 2017-04-23 23:31:29]

lgtm

Adding Rachel for a quick review. Note we have tested this on desktop Linux and
Android.

[groby-ooo-7-16, 2017-04-26 22:55:41]

On 2017/04/23 23:31:29, napper wrote:
> lgtm
> 
> Adding Rachel for a quick review. Note we have tested this on desktop Linux
and
> Android.

Hm. Shouldn't we prevent even the attempt to translate, as opposed to handle
this when it's done "translating"? We're already checking this in
TranslateManager::InitiateTranslation, - maybe TranslateManager::DoTranslatePage
would be a better spot. (The previous instance of this problem was bug #612973 -
that addressed reloading *after* the network was turned off)

[groby-ooo-7-16, 2017-04-26 22:57:32]

On 2017/04/26 22:55:41, groby wrote:
> On 2017/04/23 23:31:29, napper wrote:
> > lgtm
> > 
> > Adding Rachel for a quick review. Note we have tested this on desktop Linux
> and
> > Android.
> 
> Hm. Shouldn't we prevent even the attempt to translate, as opposed to handle
> this when it's done "translating"? We're already checking this in
> TranslateManager::InitiateTranslation, - maybe
TranslateManager::DoTranslatePage
> would be a better spot. (The previous instance of this problem was bug #612973
-
> that addressed reloading *after* the network was turned off)

Also note: If I don't answer do a CL within <24h, I'm clearly in a failure state
- send IM to reset ;)

(In general, I suggest IMing me directly for any CL - there's a good chance we
can avoid 24h roundtrip that way.)

[napper, 2017-04-26 23:39:05]

On 2017/04/26 at 22:57:32, groby wrote:
> On 2017/04/26 22:55:41, groby wrote:
> > On 2017/04/23 23:31:29, napper wrote:
> > > lgtm
> > > 
> > > Adding Rachel for a quick review. Note we have tested this on desktop
Linux
> > and
> > > Android.
> > 
> > Hm. Shouldn't we prevent even the attempt to translate, as opposed to handle
> > this when it's done "translating"? We're already checking this in
> > TranslateManager::InitiateTranslation, - maybe
TranslateManager::DoTranslatePage
> > would be a better spot. (The previous instance of this problem was bug
#612973 -
> > that addressed reloading *after* the network was turned off)
> 
> Also note: If I don't answer do a CL within <24h, I'm clearly in a failure
state - send IM to reset ;)
> 
> (In general, I suggest IMing me directly for any CL - there's a good chance we
can avoid 24h roundtrip that way.)

Got it, thanks.

Renjie, can you look into detecting the network connectivity issue prior to
calling StartTranslate() as discussed above?

[renjieliu1, 2017-04-27 01:43:46]

On 2017/04/26 23:39:05, napper wrote:
> On 2017/04/26 at 22:57:32, groby wrote:
> > On 2017/04/26 22:55:41, groby wrote:
> > > On 2017/04/23 23:31:29, napper wrote:
> > > > lgtm
> > > > 
> > > > Adding Rachel for a quick review. Note we have tested this on desktop
> Linux
> > > and
> > > > Android.
> > > 
> > > Hm. Shouldn't we prevent even the attempt to translate, as opposed to
handle
> > > this when it's done "translating"? We're already checking this in
> > > TranslateManager::InitiateTranslation, - maybe
> TranslateManager::DoTranslatePage
> > > would be a better spot. (The previous instance of this problem was bug
> #612973 -
> > > that addressed reloading *after* the network was turned off)
> > 
> > Also note: If I don't answer do a CL within <24h, I'm clearly in a failure
> state - send IM to reset ;)
> > 
> > (In general, I suggest IMing me directly for any CL - there's a good chance
we
> can avoid 24h roundtrip that way.)
> 
> Got it, thanks.
> 
> Renjie, can you look into detecting the network connectivity issue prior to
> calling StartTranslate() as discussed above?

thanks for the review, detecting network error and pop up UI before
DoTranslatePage

[groby1, 2017-04-28 04:55:13]

On 2017/04/27 01:43:46, renjieliu1 wrote:
> On 2017/04/26 23:39:05, napper wrote:
> > On 2017/04/26 at 22:57:32, groby wrote:
> > > On 2017/04/26 22:55:41, groby wrote:
> > > > On 2017/04/23 23:31:29, napper wrote:
> > > > > lgtm
> > > > > 
> > > > > Adding Rachel for a quick review. Note we have tested this on desktop
> > Linux
> > > > and
> > > > > Android.
> > > > 
> > > > Hm. Shouldn't we prevent even the attempt to translate, as opposed to
> handle
> > > > this when it's done "translating"? We're already checking this in
> > > > TranslateManager::InitiateTranslation, - maybe
> > TranslateManager::DoTranslatePage
> > > > would be a better spot. (The previous instance of this problem was bug
> > #612973 -
> > > > that addressed reloading *after* the network was turned off)
> > > 
> > > Also note: If I don't answer do a CL within <24h, I'm clearly in a failure
> > state - send IM to reset ;)
> > > 
> > > (In general, I suggest IMing me directly for any CL - there's a good
chance
> we
> > can avoid 24h roundtrip that way.)
> > 
> > Got it, thanks.
> > 
> > Renjie, can you look into detecting the network connectivity issue prior to
> > calling StartTranslate() as discussed above?
> 
> thanks for the review, detecting network error and pop up UI before
> DoTranslatePage

That definitely works - LGTM (but please add a test if at all possible)

However, in the long run I'm not sure this is enough. Technically, we need to
observe online status and take action on status change. Otherwise, you'll always
have a race condition. (I.e. what if the network goes down right after the
check?)

I'd still land this, because it addresses a fairly common failure case, but in
the long run I'd suggest investigating a solution that handles status change
properly.

[napper, 2017-04-28 05:52:04]

On 2017/04/28 at 04:55:13, groby wrote:
> On 2017/04/27 01:43:46, renjieliu1 wrote:
> > On 2017/04/26 23:39:05, napper wrote:
> > > On 2017/04/26 at 22:57:32, groby wrote:
> > > > On 2017/04/26 22:55:41, groby wrote:
> > > > > On 2017/04/23 23:31:29, napper wrote:
> > > > > > lgtm
> > > > > > 
> > > > > > Adding Rachel for a quick review. Note we have tested this on
desktop
> > > Linux
> > > > > and
> > > > > > Android.
> > > > > 
> > > > > Hm. Shouldn't we prevent even the attempt to translate, as opposed to
> > handle
> > > > > this when it's done "translating"? We're already checking this in
> > > > > TranslateManager::InitiateTranslation, - maybe
> > > TranslateManager::DoTranslatePage
> > > > > would be a better spot. (The previous instance of this problem was bug
> > > #612973 -
> > > > > that addressed reloading *after* the network was turned off)
> > > > 
> > > > Also note: If I don't answer do a CL within <24h, I'm clearly in a
failure
> > > state - send IM to reset ;)
> > > > 
> > > > (In general, I suggest IMing me directly for any CL - there's a good
chance
> > we
> > > can avoid 24h roundtrip that way.)
> > > 
> > > Got it, thanks.
> > > 
> > > Renjie, can you look into detecting the network connectivity issue prior
to
> > > calling StartTranslate() as discussed above?
> > 
> > thanks for the review, detecting network error and pop up UI before
> > DoTranslatePage
> 
> That definitely works - LGTM (but please add a test if at all possible)
> 
> However, in the long run I'm not sure this is enough. Technically, we need to
observe online status and take action on status change. Otherwise, you'll always
have a race condition. (I.e. what if the network goes down right after the
check?)
> 
> I'd still land this, because it addresses a fairly common failure case, but in
the long run I'd suggest investigating a solution that handles status change
properly.

Is it worth keeping the old fix as well (i.e., the check that happens after the
translate completes or fails)? That would handle the case where network
connectivity is lost after this check.

[renjieliu1, 2017-04-28 06:47:55]

On 2017/04/28 05:52:04, napper wrote:
> On 2017/04/28 at 04:55:13, groby wrote:
> > On 2017/04/27 01:43:46, renjieliu1 wrote:
> > > On 2017/04/26 23:39:05, napper wrote:
> > > > On 2017/04/26 at 22:57:32, groby wrote:
> > > > > On 2017/04/26 22:55:41, groby wrote:
> > > > > > On 2017/04/23 23:31:29, napper wrote:
> > > > > > > lgtm
> > > > > > > 
> > > > > > > Adding Rachel for a quick review. Note we have tested this on
> desktop
> > > > Linux
> > > > > > and
> > > > > > > Android.
> > > > > > 
> > > > > > Hm. Shouldn't we prevent even the attempt to translate, as opposed
to
> > > handle
> > > > > > this when it's done "translating"? We're already checking this in
> > > > > > TranslateManager::InitiateTranslation, - maybe
> > > > TranslateManager::DoTranslatePage
> > > > > > would be a better spot. (The previous instance of this problem was
bug
> > > > #612973 -
> > > > > > that addressed reloading *after* the network was turned off)
> > > > > 
> > > > > Also note: If I don't answer do a CL within <24h, I'm clearly in a
> failure
> > > > state - send IM to reset ;)
> > > > > 
> > > > > (In general, I suggest IMing me directly for any CL - there's a good
> chance
> > > we
> > > > can avoid 24h roundtrip that way.)
> > > > 
> > > > Got it, thanks.
> > > > 
> > > > Renjie, can you look into detecting the network connectivity issue prior
> to
> > > > calling StartTranslate() as discussed above?
> > > 
> > > thanks for the review, detecting network error and pop up UI before
> > > DoTranslatePage
> > 
> > That definitely works - LGTM (but please add a test if at all possible)
> > 
> > However, in the long run I'm not sure this is enough. Technically, we need
to
> observe online status and take action on status change. Otherwise, you'll
always
> have a race condition. (I.e. what if the network goes down right after the
> check?)
> > 
> > I'd still land this, because it addresses a fairly common failure case, but
in
> the long run I'd suggest investigating a solution that handles status change
> properly.
> 
> Is it worth keeping the old fix as well (i.e., the check that happens after
the
> translate completes or fails)? That would handle the case where network
> connectivity is lost after this check.

unit test added and keep the old fix as well since it's a workaround to handle
when network connectivity is lost after the check. 

But I totally agree in the long run we need an elegant solution to solve this
problem.

[renjieliu1, 2017-04-28 08:47:21]

The CQ bit was checked by renjieliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 08:47:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 09:40:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 09:40:45]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-05-02 03:14:59]

On 2017/04/28 06:47:55, renjieliu1 wrote:
> On 2017/04/28 05:52:04, napper wrote:
> > On 2017/04/28 at 04:55:13, groby wrote:
> > > On 2017/04/27 01:43:46, renjieliu1 wrote:
> > > > On 2017/04/26 23:39:05, napper wrote:
> > > > > On 2017/04/26 at 22:57:32, groby wrote:
> > > > > > On 2017/04/26 22:55:41, groby wrote:
> > > > > > > On 2017/04/23 23:31:29, napper wrote:
> > > > > > > > lgtm
> > > > > > > > 
> > > > > > > > Adding Rachel for a quick review. Note we have tested this on
> > desktop
> > > > > Linux
> > > > > > > and
> > > > > > > > Android.
> > > > > > > 
> > > > > > > Hm. Shouldn't we prevent even the attempt to translate, as opposed
> to
> > > > handle
> > > > > > > this when it's done "translating"? We're already checking this in
> > > > > > > TranslateManager::InitiateTranslation, - maybe
> > > > > TranslateManager::DoTranslatePage
> > > > > > > would be a better spot. (The previous instance of this problem was
> bug
> > > > > #612973 -
> > > > > > > that addressed reloading *after* the network was turned off)
> > > > > > 
> > > > > > Also note: If I don't answer do a CL within <24h, I'm clearly in a
> > failure
> > > > > state - send IM to reset ;)
> > > > > > 
> > > > > > (In general, I suggest IMing me directly for any CL - there's a good
> > chance
> > > > we
> > > > > can avoid 24h roundtrip that way.)
> > > > > 
> > > > > Got it, thanks.
> > > > > 
> > > > > Renjie, can you look into detecting the network connectivity issue
prior
> > to
> > > > > calling StartTranslate() as discussed above?
> > > > 
> > > > thanks for the review, detecting network error and pop up UI before
> > > > DoTranslatePage
> > > 
> > > That definitely works - LGTM (but please add a test if at all possible)
> > > 
> > > However, in the long run I'm not sure this is enough. Technically, we need
> to
> > observe online status and take action on status change. Otherwise, you'll
> always
> > have a race condition. (I.e. what if the network goes down right after the
> > check?)
> > > 
> > > I'd still land this, because it addresses a fairly common failure case,
but
> in
> > the long run I'd suggest investigating a solution that handles status change
> > properly.
> > 
> > Is it worth keeping the old fix as well (i.e., the check that happens after
> the
> > translate completes or fails)? That would handle the case where network
> > connectivity is lost after this check.
> 
> unit test added and keep the old fix as well since it's a workaround to handle
> when network connectivity is lost after the check. 
> 
> But I totally agree in the long run we need an elegant solution to solve this
> problem.

Still LGTM :)

(In general, I try to give LGTM ahead of the last round of minor fixes - if you
only address those, no need to ping me again, just feel free to commit. I try to
shorten the round-trip pain :)

[renjieliu1, 2017-05-02 04:45:44]

On 2017/05/02 03:14:59, groby wrote:
> On 2017/04/28 06:47:55, renjieliu1 wrote:
> > On 2017/04/28 05:52:04, napper wrote:
> > > On 2017/04/28 at 04:55:13, groby wrote:
> > > > On 2017/04/27 01:43:46, renjieliu1 wrote:
> > > > > On 2017/04/26 23:39:05, napper wrote:
> > > > > > On 2017/04/26 at 22:57:32, groby wrote:
> > > > > > > On 2017/04/26 22:55:41, groby wrote:
> > > > > > > > On 2017/04/23 23:31:29, napper wrote:
> > > > > > > > > lgtm
> > > > > > > > > 
> > > > > > > > > Adding Rachel for a quick review. Note we have tested this on
> > > desktop
> > > > > > Linux
> > > > > > > > and
> > > > > > > > > Android.
> > > > > > > > 
> > > > > > > > Hm. Shouldn't we prevent even the attempt to translate, as
opposed
> > to
> > > > > handle
> > > > > > > > this when it's done "translating"? We're already checking this
in
> > > > > > > > TranslateManager::InitiateTranslation, - maybe
> > > > > > TranslateManager::DoTranslatePage
> > > > > > > > would be a better spot. (The previous instance of this problem
was
> > bug
> > > > > > #612973 -
> > > > > > > > that addressed reloading *after* the network was turned off)
> > > > > > > 
> > > > > > > Also note: If I don't answer do a CL within <24h, I'm clearly in a
> > > failure
> > > > > > state - send IM to reset ;)
> > > > > > > 
> > > > > > > (In general, I suggest IMing me directly for any CL - there's a
good
> > > chance
> > > > > we
> > > > > > can avoid 24h roundtrip that way.)
> > > > > > 
> > > > > > Got it, thanks.
> > > > > > 
> > > > > > Renjie, can you look into detecting the network connectivity issue
> prior
> > > to
> > > > > > calling StartTranslate() as discussed above?
> > > > > 
> > > > > thanks for the review, detecting network error and pop up UI before
> > > > > DoTranslatePage
> > > > 
> > > > That definitely works - LGTM (but please add a test if at all possible)
> > > > 
> > > > However, in the long run I'm not sure this is enough. Technically, we
need
> > to
> > > observe online status and take action on status change. Otherwise, you'll
> > always
> > > have a race condition. (I.e. what if the network goes down right after the
> > > check?)
> > > > 
> > > > I'd still land this, because it addresses a fairly common failure case,
> but
> > in
> > > the long run I'd suggest investigating a solution that handles status
change
> > > properly.
> > > 
> > > Is it worth keeping the old fix as well (i.e., the check that happens
after
> > the
> > > translate completes or fails)? That would handle the case where network
> > > connectivity is lost after this check.
> > 
> > unit test added and keep the old fix as well since it's a workaround to
handle
> > when network connectivity is lost after the check. 
> > 
> > But I totally agree in the long run we need an elegant solution to solve
this
> > problem.
> 
> Still LGTM :)
> 
> (In general, I try to give LGTM ahead of the last round of minor fixes - if
you
> only address those, no need to ping me again, just feel free to commit. I try
to
> shorten the round-trip pain :)

got it! thanks!

[renjieliu1, 2017-05-02 04:48:06]

The CQ bit was checked by renjieliu@chromium.org

[renjieliu1, 2017-05-02 04:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org,
groby@google.com
Link to the patchset: https://codereview.chromium.org/2822383002/#ps60001
(title: "add unit tests")

[commit-bot: I haz the power, 2017-05-02 04:48:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 05:42:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493700486553020,
"parent_rev": "09eed0675cc8f316344caf5ac695f140608493f6", "commit_rev":
"97e2e2a80810368ef54b61ddfe1f529cdb801cf2"}

[commit-bot: I haz the power, 2017-05-02 05:42:59]

Description was changed from

==========
Check network connectivity in onPageTranslated callback.

When attempting second translation keeping the WiFi/Data OFF, Chrome
shows "Translated to <desired language>" message though web page remains
untranslated.

Current fix is to check whether user is online before showing up the
translate UI in OnPageTranslated callback. 

BUG=653499
==========

to

==========
Check network connectivity in onPageTranslated callback.

When attempting second translation keeping the WiFi/Data OFF, Chrome
shows "Translated to <desired language>" message though web page remains
untranslated.

Current fix is to check whether user is online before showing up the
translate UI in OnPageTranslated callback. 

BUG=653499

Review-Url: https://codereview.chromium.org/2822383002
Cr-Commit-Position: refs/heads/master@{#468569}
Committed:
https://chromium.googlesource.com/chromium/src/+/97e2e2a80810368ef54b61ddfe1f...
==========

[commit-bot: I haz the power, 2017-05-02 05:43:01]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/97e2e2a80810368ef54b61ddfe1f...