New issue
Advanced search Search tips

Issue 879821 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

Translate retries are not "real" retries.

Project Member Reported by jzw@chromium.org, Sep 1

Issue description

When translate fails, the entire chain of scripts should be re-injected to ensure that translate can start with a clean slate.
 
In the old polling logic, retry on an error merely rechecks the translate ready state,
which doesn't actually rerun the scripts. It worked like this:

1. Inject translate script.
2. Poll for translate ready state, waiting only 750ms.
3. Translate did not become ready in time. Show error to user.
4. Translate becomes ready after 750ms.
4. User taps retry. We check ready status again.
5. Translate is now ready so translate succeeds.

Now that the polling logic has been replaced with callbacks, there is no error that 
can result from timing out. Instead all errors now are not really recoverable except
by reinjecting the scripts to restart the entire thing.

Note that CSP errors will never succeed even if retries are attempted. Bling should
handle this case properly instead of showing a retry button.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df9af169f911522de26fa442ecdd4ea47901b0fa

commit df9af169f911522de26fa442ecdd4ea47901b0fa
Author: John Z Wu <jzw@chromium.org>
Date: Thu Sep 06 21:59:21 2018

Fix translate retry logic by re-injecting the scripts.

Currently retry only re-checks the ready status of the scripts.
To properly retry, the scripts need to be reinjected so a new ready
status can be provided.

Bug:  879821 
Change-Id: I3f188a3358435304006c907b7dd3799d907c6a80
Reviewed-on: https://chromium-review.googlesource.com/1200573
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589320}
[modify] https://crrev.com/df9af169f911522de26fa442ecdd4ea47901b0fa/components/translate/core/browser/resources/translate.js
[modify] https://crrev.com/df9af169f911522de26fa442ecdd4ea47901b0fa/components/translate/ios/browser/js_translate_manager.mm

Labels: CommitLog-Audit-Violation Merge-Without-Approval M-70
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 0ab909d5bfaec1ec62709ed390baf3d6e814953d was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ab909d5bfaec1ec62709ed390baf3d6e814953d

commit 0ab909d5bfaec1ec62709ed390baf3d6e814953d
Author: John Z Wu <jzw@chromium.org>
Date: Wed Sep 12 04:02:53 2018

Fix translate retry logic by re-injecting the scripts.

Currently retry only re-checks the ready status of the scripts.
To properly retry, the scripts need to be reinjected so a new ready
status can be provided.

Bug:  879821 
Change-Id: I3f188a3358435304006c907b7dd3799d907c6a80
Reviewed-on: https://chromium-review.googlesource.com/1200573
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589320}(cherry picked from commit df9af169f911522de26fa442ecdd4ea47901b0fa)
Reviewed-on: https://chromium-review.googlesource.com/1220402
Reviewed-by: John Wu <jzw@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#310}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/0ab909d5bfaec1ec62709ed390baf3d6e814953d/components/translate/core/browser/resources/translate.js
[modify] https://crrev.com/0ab909d5bfaec1ec62709ed390baf3d6e814953d/components/translate/ios/browser/js_translate_manager.mm

Hi jzw, seems this CL was merged to branch without TPM approval. Why is that?
Status: Fixed (was: Assigned)

Sign in to add a comment