New issue
Advanced search Search tips

Issue 877706 link

Starred by 1 user
Status: Fixed
Owner:
jzw@chromium.org
Closed: Oct 12
Cc:
droger@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

Insert translate_ios.js between translate.js and element.js.

Project Member Reported by jzw@chromium.org, Aug 24 
The current injection order is:
1. translate.js (bundled script for all platforms)
2. element.js (downloaded from translate server)
3. translate_ios.js (bundled script only for iOS)

translate_ios.js sets callbacks on translate.js and so must be injected after. However, element.js can trigger events in translate.js before translate_ios.js has had a chance to install callbacks on translate.js.

The injection order should be:
1. translate.js
2. translate_ios.js
3. element.js
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 30 

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

commit bcd316044f7126811958871c35f0b05ed49fc1bf
Author: John Z Wu <jzw@chromium.org>
Date: Thu Aug 30 18:56:40 2018

Add a way to install callbacks on translate.js before element.js.

This is needed to ensure that callbacks installed on
translate.js can be registered before element.js executes.

Change-Id: Idefe034e77879169b57185bcad88066d1917de82
Bug:  877706 
Reviewed-on: https://chromium-review.googlesource.com/1189060
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587681}
[modify] https://crrev.com/bcd316044f7126811958871c35f0b05ed49fc1bf/components/translate/core/browser/translate_script.cc
[modify] https://crrev.com/bcd316044f7126811958871c35f0b05ed49fc1bf/components/translate/core/browser/translate_script.h
[modify] https://crrev.com/bcd316044f7126811958871c35f0b05ed49fc1bf/components/translate/core/browser/translate_script_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 31 

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

commit 626cb9384e17018c37c65a15c21bf333234ab8df
Author: John Z Wu <jzw@chromium.org>
Date: Fri Aug 31 18:46:20 2018

Update translate_ios.js to define a function to install callbacks.

This ensures that callbacks can be installed before they are invoked.

Bug:  877706 
Change-Id: Iabaac2bdeda2347b6968f0c03ade3a71739e290f
Reviewed-on: https://chromium-review.googlesource.com/1192419
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588099}
[modify] https://crrev.com/626cb9384e17018c37c65a15c21bf333234ab8df/components/translate/core/browser/resources/translate.js
[modify] https://crrev.com/626cb9384e17018c37c65a15c21bf333234ab8df/components/translate/ios/browser/js_translate_manager.mm
[modify] https://crrev.com/626cb9384e17018c37c65a15c21bf333234ab8df/components/translate/ios/browser/resources/translate_ios.js
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 12

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

commit 59ed680b9fc2cc5fe0e2b1bf487d70df9608c8d6
Author: John Z Wu <jzw@chromium.org>
Date: Wed Sep 12 04:01:31 2018

Update translate_ios.js to define a function to install callbacks.

This ensures that callbacks can be installed before they are invoked.

Bug:  877706 
Change-Id: Iabaac2bdeda2347b6968f0c03ade3a71739e290f
Reviewed-on: https://chromium-review.googlesource.com/1192419
Commit-Queue: John Wu <jzw@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588099}(cherry picked from commit 626cb9384e17018c37c65a15c21bf333234ab8df)
Reviewed-on: https://chromium-review.googlesource.com/1220399
Reviewed-by: John Wu <jzw@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#309}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/59ed680b9fc2cc5fe0e2b1bf487d70df9608c8d6/components/translate/core/browser/resources/translate.js
[modify] https://crrev.com/59ed680b9fc2cc5fe0e2b1bf487d70df9608c8d6/components/translate/ios/browser/js_translate_manager.mm
[modify] https://crrev.com/59ed680b9fc2cc5fe0e2b1bf487d70df9608c8d6/components/translate/ios/browser/resources/translate_ios.js
Project Member

Comment 4 by cr-audit...@appspot.gserviceaccount.com, Sep 12

Labels: CommitLog-Audit-Violation Merge-Without-Approval M-70
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 59ed680b9fc2cc5fe0e2b1bf487d70df9608c8d6 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!

Comment 5 by kariahda@chromium.org, Sep 20 

Hi jzw, seems this CL was merged to branch without TPM approval. Was this related to a different bug that I gave approval on?

Comment 6 by jzw@chromium.org, Sep 20 

This was cherrypicked as part of https://bugs.chromium.org/p/chromium/issues/detail?id=881518#c13. It landed before that bug was created, sorry for the confusion.

Comment 7 by jzw@chromium.org, Oct 12

Status: Fixed (was: Available)
►

Sign in to add a comment