Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533953003:
[translate] Fix enforcement vs logging for TranslateRanker. (Closed)
|
Created:
4 years ago by Roger McFarlane (Chromium) Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[translate] Fix enforcement vs logging for TranslateRanker.
The translate ranker enforcement flag was redundantly enforced
within the ranker itself, thwarting the TranslateManagers attempts
to log the ranker result and enforc (or not) the result. The
check in the TranslateRanker should have been removed in the
last refactor.
BUG=669528
Committed: https://crrev.com/131e0dc96c8c88fc1fc222442a61fb2704b1795d
Cr-Commit-Position: refs/heads/master@{#435061}
Patch Set 1 : remove enforcement check #
Total comments: 4
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 ========== to ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 ==========
rogerm@chromium.org changed reviewers: + groby@chromium.org, hamelphi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL?
https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... File components/translate/core/browser/translate_ranker.cc (left): https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... components/translate/core/browser/translate_ranker.cc:185: return IsEnforcementEnabled() ? result : kDefaultResponse; The TranslateManager code where this is enforced is at https://cs.chromium.org/chromium/src/components/translate/core/browser/transl...
groby@google.com changed reviewers: + groby@google.com
Based on the CL description, the behavior difference I called out in the remark is intentional. If that's the case - LGTM. (If it isn't, obviously not so much :) https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... File components/translate/core/browser/translate_ranker.cc (left): https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... components/translate/core/browser/translate_ranker.cc:185: return IsEnforcementEnabled() ? result : kDefaultResponse; On 2016/11/29 18:43:30, Roger McFarlane (Chromium) wrote: > The TranslateManager code where this is enforced is at > https://cs.chromium.org/chromium/src/components/translate/core/browser/transl... The behavior in translate manager is subtly different, in that the ranker response is always logged as the actual result, even when enforcement is disabled. Intentionally, or bug?
The CQ bit was checked by hamelphi@chromium.org
lgtm https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... File components/translate/core/browser/translate_ranker.cc (left): https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... components/translate/core/browser/translate_ranker.cc:185: return IsEnforcementEnabled() ? result : kDefaultResponse; On 2016/11/29 19:03:43, groby1 wrote: > On 2016/11/29 18:43:30, Roger McFarlane (Chromium) wrote: > > The TranslateManager code where this is enforced is at > > > https://cs.chromium.org/chromium/src/components/translate/core/browser/transl... > > The behavior in translate manager is subtly different, in that the ranker > response is always logged as the actual result, even when enforcement is > disabled. Intentionally, or bug? It is intentional. We want to log the response the ranker would have given even if we don't enforce it. See b/33176772.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/11/29 19:37:25, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. GAH. Sorry. Accidentally used my @google account. LGTM again. HOORAY FOR TOOLS!
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480448755855940,
"parent_rev": "2254c07522f2bd2db0903f5d50704f47ab45d030", "commit_rev":
"02b178a85c685fa4bf5ea26419085acfa906cb10"}
Message was sent while issue was closed.
Description was changed from ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 ========== to ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 ========== to ========== [translate] Fix enforcement vs logging for TranslateRanker. The translate ranker enforcement flag was redundantly enforced within the ranker itself, thwarting the TranslateManagers attempts to log the ranker result and enforc (or not) the result. The check in the TranslateRanker should have been removed in the last refactor. BUG=669528 Committed: https://crrev.com/131e0dc96c8c88fc1fc222442a61fb2704b1795d Cr-Commit-Position: refs/heads/master@{#435061} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/131e0dc96c8c88fc1fc222442a61fb2704b1795d Cr-Commit-Position: refs/heads/master@{#435061}
Message was sent while issue was closed.
https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... File components/translate/core/browser/translate_ranker.cc (left): https://codereview.chromium.org/2533953003/diff/20001/components/translate/co... components/translate/core/browser/translate_ranker.cc:185: return IsEnforcementEnabled() ? result : kDefaultResponse; On 2016/11/29 19:37:06, hamelphi wrote: > On 2016/11/29 19:03:43, groby1 wrote: > > On 2016/11/29 18:43:30, Roger McFarlane (Chromium) wrote: > > > The TranslateManager code where this is enforced is at > > > > > > https://cs.chromium.org/chromium/src/components/translate/core/browser/transl... > > > > The behavior in translate manager is subtly different, in that the ranker > > response is always logged as the actual result, even when enforcement is > > disabled. Intentionally, or bug? > > It is intentional. We want to log the response the ranker would have given even > if we don't enforce it. See b/33176772. Precisely. :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
