New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 653881 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Merging into M54 a CL switching the new language detector with the old one

Project Member Reported by abakalov@chromium.org, Oct 7 2016

Issue description

We'd like to merge the following CL into M54:
https://codereview.chromium.org/2396183002/

It switches the new language detector (CLD3) with the old one (CLD2). The reason is that we need to spend some more time testing the new language detector before launching it to stable.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 7 2016

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

commit 13cff7269b9e76b0f77ecb94eade1dfee02e6a32
Author: abakalov <abakalov@chromium.org>
Date: Fri Oct 07 14:38:03 2016

Switching from CLD3 to CLD2

This is accomplished by:
- flipping the flag in third_party/cld/BUILD.gn
- listing explicitly source files in third_party/cld_2/BUILD.gn that
were originally retrieved from the following gyp file which got
removed during the transition away from GYP:
https://chromium.googlesource.com/chromium/src/+/4b56905d6ab61e9e14f991a7458d8968635cfef2/third_party/cld_2/cld_2.gyp

BUG= 653881 

Review-Url: https://codereview.chromium.org/2396183002
Cr-Commit-Position: refs/heads/master@{#423855}

[modify] https://crrev.com/13cff7269b9e76b0f77ecb94eade1dfee02e6a32/third_party/cld/BUILD.gn
[modify] https://crrev.com/13cff7269b9e76b0f77ecb94eade1dfee02e6a32/third_party/cld_2/BUILD.gn

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 7 2016

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

commit ac2066d0dd39fd8d282617a8ab9416b91c773e61
Author: abakalov <abakalov@chromium.org>
Date: Fri Oct 07 17:25:01 2016

Revert of Switching from CLD3 to CLD2 (patchset #1 id:1 of https://codereview.chromium.org/2396183002/ )

Reason for revert:
We created this CL, so that we can merge it into M54 and switch from CLD3 to CLD2. I am reverting it now to continue testing CLD3 in M55.

Original issue's description:
> Switching from CLD3 to CLD2
>
> This is accomplished by:
> - flipping the flag in third_party/cld/BUILD.gn
> - listing explicitly source files in third_party/cld_2/BUILD.gn that
> were originally retrieved from the following gyp file which got
> removed during the transition away from GYP:
> https://chromium.googlesource.com/chromium/src/+/4b56905d6ab61e9e14f991a7458d8968635cfef2/third_party/cld_2/cld_2.gyp
>
> BUG= 653881 
>
> Committed: https://crrev.com/13cff7269b9e76b0f77ecb94eade1dfee02e6a32
> Cr-Commit-Position: refs/heads/master@{#423855}

TBR=brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653881 

Review-Url: https://codereview.chromium.org/2400273002
Cr-Commit-Position: refs/heads/master@{#423895}

[modify] https://crrev.com/ac2066d0dd39fd8d282617a8ab9416b91c773e61/third_party/cld/BUILD.gn
[modify] https://crrev.com/ac2066d0dd39fd8d282617a8ab9416b91c773e61/third_party/cld_2/BUILD.gn

Are there an dependencies changes here?  I'm not familiar with CLD in general, but I'm concerned there could be code that calls an API supported in CLD3 that doesn't exist in CLD2 for example.

If it's just updating the internal language detection logic, I'll approve it.
There is a compile-time flag defined in //third_party/cld/BUILD.gn:
https://codereview.chromium.org/2396183002/patch/1/10001

This flag determines whether to execute the Chrome code calling CLD2 or the separate Chrome code calling CLD3. For example:
https://cs.corp.google.com/clankium/src/components/translate/core/language_detection/language_detection_util.cc?rcl=0&l=90

As a result, the API scenario in your message cannot happen.

Please let me know if you need further clarifications.

Comment 5 by dimu@chromium.org, Oct 8 2016

Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M54, manual review required.
Labels: -Merge-Review-54 Merge-Approved-54
Sounds good to me, thanks!  Approved merge into M54
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 11 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fafcc418d3e49f2d99f246effdb39499e100e84f

commit fafcc418d3e49f2d99f246effdb39499e100e84f
Author: Rouslan Solomakhin <rouslan@chromium.org>
Date: Tue Oct 11 20:06:15 2016

[Merge M54] Switching from CLD3 to CLD2

This is accomplished by:
- flipping the flag in third_party/cld/BUILD.gn
- listing explicitly source files in third_party/cld_2/BUILD.gn that
were originally retrieved from the following gyp file which got
removed during the transition away from GYP:
https://chromium.googlesource.com/chromium/src/+/4b56905d6ab61e9e14f991a7458d8968635cfef2/third_party/cld_2/cld_2.gyp

BUG= 653881 

Review-Url: https://codereview.chromium.org/2396183002
Cr-Commit-Position: refs/heads/master@{#423855}
(cherry picked from commit 13cff7269b9e76b0f77ecb94eade1dfee02e6a32)

Review URL: https://codereview.chromium.org/2406423005 .

Cr-Commit-Position: refs/branch-heads/2840@{#719}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/fafcc418d3e49f2d99f246effdb39499e100e84f/third_party/cld/BUILD.gn
[modify] https://crrev.com/fafcc418d3e49f2d99f246effdb39499e100e84f/third_party/cld_2/BUILD.gn

Thanks, Rouslan!
Status: Fixed (was: Untriaged)
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 27 2016

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

commit fafcc418d3e49f2d99f246effdb39499e100e84f
Author: Rouslan Solomakhin <rouslan@chromium.org>
Date: Tue Oct 11 20:06:15 2016

[Merge M54] Switching from CLD3 to CLD2

This is accomplished by:
- flipping the flag in third_party/cld/BUILD.gn
- listing explicitly source files in third_party/cld_2/BUILD.gn that
were originally retrieved from the following gyp file which got
removed during the transition away from GYP:
https://chromium.googlesource.com/chromium/src/+/4b56905d6ab61e9e14f991a7458d8968635cfef2/third_party/cld_2/cld_2.gyp

BUG= 653881 

Review-Url: https://codereview.chromium.org/2396183002
Cr-Commit-Position: refs/heads/master@{#423855}
(cherry picked from commit 13cff7269b9e76b0f77ecb94eade1dfee02e6a32)

Review URL: https://codereview.chromium.org/2406423005 .

Cr-Commit-Position: refs/branch-heads/2840@{#719}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/fafcc418d3e49f2d99f246effdb39499e100e84f/third_party/cld/BUILD.gn
[modify] https://crrev.com/fafcc418d3e49f2d99f246effdb39499e100e84f/third_party/cld_2/BUILD.gn

Sign in to add a comment