Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971703004:
add log to mac translate bubble implementation (Closed)
|
Created:
4 years, 7 months ago by ftang Modified:
4 years, 7 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd log to mac translate bubble implementation
BUG=611164
Committed: https://crrev.com/ab22e8b1a34d2eaacb713fb2e48c68edb4d95466
Cr-Commit-Position: refs/heads/master@{#394274}
Patch Set 1 #
Total comments: 4
Patch Set 2 : change to use translate::ReportUiAction #Messages
Total messages: 18 (7 generated)
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971703004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971703004/1
groby@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:611: ? translate::ALWAYS_TRANSLATE_CHECKED Does that match views semantics? (I.e. are we recording every state change there as well? Or do we record when leaving the bubble?) In general, I'd love to move those actions up to the model (or in a separate controller) - we're duplicating work between Views and OSX https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:682: - (void)reportUiAction:(translate::TranslateBubbleUiEvent)action { Please make this shared between Views and OSX - no reason to duplicate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
change to use translate::ReportUiAction
The CQ bit was checked by ftang@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:611: ? translate::ALWAYS_TRANSLATE_CHECKED On 2016/05/11 20:32:45, groby wrote: > Does that match views semantics? (I.e. are we recording every state change there > as well? Or do we record when leaving the bubble?) > > In general, I'd love to move those actions up to the model (or in a separate > controller) - we're duplicating work between Views and OSX yes, this record the user's action, not their final decision. It is the same in the window/linux bubble. https://codereview.chromium.org/1971703004/diff/1/chrome/browser/ui/cocoa/tra... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:682: - (void)reportUiAction:(translate::TranslateBubbleUiEvent)action { On 2016/05/11 20:32:45, groby wrote: > Please make this shared between Views and OSX - no reason to duplicate. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971703004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971703004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by groby@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971703004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971703004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== add log to mac translate bubble implementation BUG=611164 ========== to ========== add log to mac translate bubble implementation BUG=611164 Committed: https://crrev.com/ab22e8b1a34d2eaacb713fb2e48c68edb4d95466 Cr-Commit-Position: refs/heads/master@{#394274} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ab22e8b1a34d2eaacb713fb2e48c68edb4d95466 Cr-Commit-Position: refs/heads/master@{#394274} |
