Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1632953002:
Change the TranslateDeclined() to only count decline if the explicit_closed is set to true (Closed)
DescriptionFix the problem that currently chrome count user action
which not explicit decline translation as decline and
only count those explicit action. The following actions
will not be counted as a decline action after this fix.
a. User open a new tab/window before confirm the translation
b. User switch to a new tab/window to load page before confirm the
translation
BUG=575023
Committed: https://crrev.com/54d1294f337c5d7fe0bc474e9ae34d38913dc1f3
Cr-Commit-Position: refs/heads/master@{#376016}
Patch Set 1 #
Total comments: 2
Patch Set 2 : move the fix to translate_ui_delegate.cc instead #Patch Set 3 : add unit test for TranslateUIDelegate::TranslateDeclined() function #Patch Set 4 : fix ios/macos unit test breakage #Patch Set 5 : fix ng build breakage #
Total comments: 37
Patch Set 6 : address review comment #Patch Set 7 : refactor out MockTranslteDriver to .h file #Patch Set 8 : Make MacOS compiled by split the MockTranslateDriver implementation into .cc file #Patch Set 9 : rebase #Patch Set 10 : fix gn build breakage for unittests #Patch Set 11 : fix mac unit test breakage #
Total comments: 1
Messages
Total messages: 57 (18 generated)
groby@chromium.org changed reviewers: + groby@chromium.org
Also: * Please add a test case * have you tested the behavior on OSX? https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void TranslateBubbleView::WindowClosing() { This means we would lose all metrics that are due to e.g. page navigation. Do we want a separate metric for that? Is it OK to do nothing with the model if things are closed e.g. due to Navigation?
Please point out how to add test case OSX does not use TranslateBubbleView yet, right? On 25 January 2016 at 17:07, <groby@chromium.org> wrote: > Also: > > * Please add a test case > * have you tested the behavior on OSX? > > > > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): > > > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void > TranslateBubbleView::WindowClosing() { > This means we would lose all metrics that are due to e.g. page > navigation. Do we want a separate metric for that? Is it OK to do > nothing with the model if things are closed e.g. due to Navigation? > > https://codereview.chromium.org/1632953002/ > -- Frank Yung-Fong Tang 譚永鋒 Sr. Software Engineer / *Google Ţråñšļåţé* -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Do you want me to extend chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc ? On 25 January 2016 at 17:09, Frank Tang (譚永鋒) <ftang@google.com> wrote: > Please point out how to add test case > OSX does not use TranslateBubbleView yet, right? > > On 25 January 2016 at 17:07, <groby@chromium.org> wrote: > >> Also: >> >> * Please add a test case >> * have you tested the behavior on OSX? >> >> >> >> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): >> >> >> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >> TranslateBubbleView::WindowClosing() { >> This means we would lose all metrics that are due to e.g. page >> navigation. Do we want a separate metric for that? Is it OK to do >> nothing with the model if things are closed e.g. due to Navigation? >> >> https://codereview.chromium.org/1632953002/ >> > > > > -- > Frank Yung-Fong Tang > 譚永鋒 > Sr. Software Engineer / *Google Ţråñšļåţé* > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/26 01:16:35, ftang wrote: > Do you want me to extend > chrome/browser/ui/views/translate/translate_bubble_view_unittest.cc > ? > > On 25 January 2016 at 17:09, Frank Tang (譚永鋒) <mailto:ftang@google.com> wrote: > > > Please point out how to add test case > > OSX does not use TranslateBubbleView yet, right? > > > > On 25 January 2016 at 17:07, <mailto:groby@chromium.org> wrote: > > > >> Also: > >> > >> * Please add a test case > >> * have you tested the behavior on OSX? > >> > >> > >> > >> > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > >> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): > >> > >> > >> > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > >> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void > >> TranslateBubbleView::WindowClosing() { > >> This means we would lose all metrics that are due to e.g. page > >> navigation. Do we want a separate metric for that? Is it OK to do > >> nothing with the model if things are closed e.g. due to Navigation? > >> > >> https://codereview.chromium.org/1632953002/ > >> > > > > > > > > -- > > Frank Yung-Fong Tang > > 譚永鋒 > > Sr. Software Engineer / *Google Ţråñšļåţé* > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Since this is related to slightly more complex behavior around page navigation, I'd suggest adding to translate_bubble_view_browsertest.cc instead. (If you're not sure how to do that, happy to chat with you) As for OSX, we're in the process of rolling it out, so it'd be helpful to very this behavior works cross-platform. You can enable it (if you have an OSX machine) at chrome://flags/#enable-translate-new-ux
There is already a chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc in the tree. Do you want me to extend that ? https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void TranslateBubbleView::WindowClosing() { On 2016/01/26 01:07:22, groby wrote: > This means we would lose all metrics that are due to e.g. page navigation. Do we > want a separate metric for that? Is it OK to do nothing with the model if things > are closed e.g. due to Navigation? The function called "TranslationDeclined()" so no information related to the real decline of translation will be lost because the function will still be called when the user really decline the translation. Only information which is not related to decline of translation will be lost, but such information should not be tracked inside a function called "TranslationDeclined() anyway. Page navigation has nothing to do with decline of translation, right?
Yes, that's what I meant - sorry for being unclear. As for page navigation/TranslationDeclined - it might be that the function has a bad name. Does the translate team need numbers on Translations that have been aborted due to page navigation or other issues? If they do, how can we still get them after this change? (I'm OK with the change per se, I just want to make sure we don't accidentally lose important data). On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: > There is already a > > chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc > in the tree. Do you want me to extend that ? > > > > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): > > > https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... > chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void > TranslateBubbleView::WindowClosing() { > On 2016/01/26 01:07:22, groby wrote: > >> This means we would lose all metrics that are due to e.g. page >> > navigation. Do we > >> want a separate metric for that? Is it OK to do nothing with the model >> > if things > >> are closed e.g. due to Navigation? >> > > The function called "TranslationDeclined()" so no information related to > the real decline of translation will be lost because the function will > still be called when the user really decline the translation. Only > information which is not related to decline of translation will be lost, > but such information should not be tracked inside a function called > "TranslationDeclined() anyway. Page navigation has nothing to do with > decline of translation, right? > > https://codereview.chromium.org/1632953002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Currently, the code inside TranslationDeclined will change the metrics Translate.DeclineTranslateDismissUI If it is not trigger by user explicit decline action. but what does this metrics really mean? What does "Dismiss UI" mean? On 25 January 2016 at 17:46, Rachel Blum <groby@chromium.org> wrote: > Yes, that's what I meant - sorry for being unclear. > > As for page navigation/TranslationDeclined - it might be that the function > has a bad name. Does the translate team need numbers on Translations that > have been aborted due to page navigation or other issues? If they do, how > can we still get them after this change? > > (I'm OK with the change per se, I just want to make sure we don't > accidentally lose important data). > > > On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: > >> There is already a >> >> chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc >> in the tree. Do you want me to extend that ? >> >> >> >> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): >> >> >> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >> TranslateBubbleView::WindowClosing() { >> On 2016/01/26 01:07:22, groby wrote: >> >>> This means we would lose all metrics that are due to e.g. page >>> >> navigation. Do we >> >>> want a separate metric for that? Is it OK to do nothing with the model >>> >> if things >> >>> are closed e.g. due to Navigation? >>> >> >> The function called "TranslationDeclined()" so no information related to >> the real decline of translation will be lost because the function will >> still be called when the user really decline the translation. Only >> information which is not related to decline of translation will be lost, >> but such information should not be tracked inside a function called >> "TranslationDeclined() anyway. Page navigation has nothing to do with >> decline of translation, right? >> >> https://codereview.chromium.org/1632953002/ >> > > -- Frank Yung-Fong Tang 譚永鋒 Sr. Software Engineer / *Google Ţråñšļåţé* -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
"Dismiss UI" means the UI has been closed for any reason that was not initiated by the user. If it was initiated by the user, we count it instead as Translate.DeclineTranslate - the distinction is made by |explicitly_closed| in TranslationDeclined. Since you are trying to prevent a language being marked as "never translate" only due to navigation, maybe that should be handled in TranslateDeclined() instead, then? I.e. by changing the first line there from if (!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { to if (*explicitly_closed && *!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { That would mean that only explicit closing of the translate bubble is counted against the 2 "decline" requests that lead to a block. As a bonus, TranslateUiDelegate is a platform-neutral class, so we don't need to fix any code on OSX when/if the bubble rolls out there. What do you think? On Mon, Jan 25, 2016 at 5:55 PM, Frank Tang (譚永鋒) <ftang@google.com> wrote: > Currently, the code inside TranslationDeclined will change the metrics > > Translate.DeclineTranslateDismissUI > > If it is not trigger by user explicit decline action. > > but what does this metrics really mean? > > What does "Dismiss UI" mean? > > > > On 25 January 2016 at 17:46, Rachel Blum <groby@chromium.org> wrote: > >> Yes, that's what I meant - sorry for being unclear. >> >> As for page navigation/TranslationDeclined - it might be that the >> function has a bad name. Does the translate team need numbers on >> Translations that have been aborted due to page navigation or other issues? >> If they do, how can we still get them after this change? >> >> (I'm OK with the change per se, I just want to make sure we don't >> accidentally lose important data). >> >> >> On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: >> >>> There is already a >>> >>> chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc >>> in the tree. Do you want me to extend that ? >>> >>> >>> >>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): >>> >>> >>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >>> TranslateBubbleView::WindowClosing() { >>> On 2016/01/26 01:07:22, groby wrote: >>> >>>> This means we would lose all metrics that are due to e.g. page >>>> >>> navigation. Do we >>> >>>> want a separate metric for that? Is it OK to do nothing with the model >>>> >>> if things >>> >>>> are closed e.g. due to Navigation? >>>> >>> >>> The function called "TranslationDeclined()" so no information related to >>> the real decline of translation will be lost because the function will >>> still be called when the user really decline the translation. Only >>> information which is not related to decline of translation will be lost, >>> but such information should not be tracked inside a function called >>> "TranslationDeclined() anyway. Page navigation has nothing to do with >>> decline of translation, right? >>> >>> https://codereview.chromium.org/1632953002/ >>> >> >> > > > -- > Frank Yung-Fong Tang > 譚永鋒 > Sr. Software Engineer / *Google Ţråñšļåţé* > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That is another way to solve it. The problem is really the name TranslationDeclined() imply the user has take some action the consciously decline the translation while what really happen is the translation prompt got closed for any reason. How about I change the implement of TranslationDeclined as you suggested but also add comment to the header of TranslationDeclined and later rename it to a better function name? On 25 January 2016 at 18:05, Rachel Blum <groby@chromium.org> wrote: > "Dismiss UI" means the UI has been closed for any reason that was not > initiated by the user. > > If it was initiated by the user, we count it instead as > Translate.DeclineTranslate - the distinction is made by |explicitly_closed| > in TranslationDeclined. > > Since you are trying to prevent a language being marked as "never > translate" only due to navigation, maybe that should be handled in > TranslateDeclined() instead, then? > > I.e. by changing the first line there from > > if (!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { > > to > > if (*explicitly_closed && *!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { > > > That would mean that only explicit closing of the translate bubble is > counted against the 2 "decline" requests that lead to a block. As a bonus, > TranslateUiDelegate is a platform-neutral class, so we don't need to fix > any code on OSX when/if the bubble rolls out there. > > What do you think? > > > On Mon, Jan 25, 2016 at 5:55 PM, Frank Tang (譚永鋒) <ftang@google.com> > wrote: > >> Currently, the code inside TranslationDeclined will change the metrics >> >> Translate.DeclineTranslateDismissUI >> >> If it is not trigger by user explicit decline action. >> >> but what does this metrics really mean? >> >> What does "Dismiss UI" mean? >> >> >> >> On 25 January 2016 at 17:46, Rachel Blum <groby@chromium.org> wrote: >> >>> Yes, that's what I meant - sorry for being unclear. >>> >>> As for page navigation/TranslationDeclined - it might be that the >>> function has a bad name. Does the translate team need numbers on >>> Translations that have been aborted due to page navigation or other issues? >>> If they do, how can we still get them after this change? >>> >>> (I'm OK with the change per se, I just want to make sure we don't >>> accidentally lose important data). >>> >>> >>> On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: >>> >>>> There is already a >>>> >>>> chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc >>>> in the tree. Do you want me to extend that ? >>>> >>>> >>>> >>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): >>>> >>>> >>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >>>> TranslateBubbleView::WindowClosing() { >>>> On 2016/01/26 01:07:22, groby wrote: >>>> >>>>> This means we would lose all metrics that are due to e.g. page >>>>> >>>> navigation. Do we >>>> >>>>> want a separate metric for that? Is it OK to do nothing with the model >>>>> >>>> if things >>>> >>>>> are closed e.g. due to Navigation? >>>>> >>>> >>>> The function called "TranslationDeclined()" so no information related to >>>> the real decline of translation will be lost because the function will >>>> still be called when the user really decline the translation. Only >>>> information which is not related to decline of translation will be lost, >>>> but such information should not be tracked inside a function called >>>> "TranslationDeclined() anyway. Page navigation has nothing to do with >>>> decline of translation, right? >>>> >>>> https://codereview.chromium.org/1632953002/ >>>> >>> >>> >> >> >> -- >> Frank Yung-Fong Tang >> 譚永鋒 >> Sr. Software Engineer / *Google Ţråñšļåţé* >> >> > -- Frank Yung-Fong Tang 譚永鋒 Sr. Software Engineer / *Google Ţråñšļåţé* -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
SGTM On Tue, Jan 26, 2016 at 10:23 AM, Frank Tang (譚永鋒) <ftang@google.com> wrote: > That is another way to solve it. The problem is really the name > TranslationDeclined() imply the user has take some action the consciously > decline the translation while what really happen is the translation prompt > got closed for any reason. > How about I change the implement of TranslationDeclined as you suggested > but also add comment to the header of > TranslationDeclined and later rename it to a better function name? > > On 25 January 2016 at 18:05, Rachel Blum <groby@chromium.org> wrote: > >> "Dismiss UI" means the UI has been closed for any reason that was not >> initiated by the user. >> >> If it was initiated by the user, we count it instead as >> Translate.DeclineTranslate - the distinction is made by |explicitly_closed| >> in TranslationDeclined. >> >> Since you are trying to prevent a language being marked as "never >> translate" only due to navigation, maybe that should be handled in >> TranslateDeclined() instead, then? >> >> I.e. by changing the first line there from >> >> if (!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { >> >> to >> >> if (*explicitly_closed && *!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { >> >> >> That would mean that only explicit closing of the translate bubble is >> counted against the 2 "decline" requests that lead to a block. As a bonus, >> TranslateUiDelegate is a platform-neutral class, so we don't need to fix >> any code on OSX when/if the bubble rolls out there. >> >> What do you think? >> >> >> On Mon, Jan 25, 2016 at 5:55 PM, Frank Tang (譚永鋒) <ftang@google.com> >> wrote: >> >>> Currently, the code inside TranslationDeclined will change the metrics >>> >>> Translate.DeclineTranslateDismissUI >>> >>> If it is not trigger by user explicit decline action. >>> >>> but what does this metrics really mean? >>> >>> What does "Dismiss UI" mean? >>> >>> >>> >>> On 25 January 2016 at 17:46, Rachel Blum <groby@chromium.org> wrote: >>> >>>> Yes, that's what I meant - sorry for being unclear. >>>> >>>> As for page navigation/TranslationDeclined - it might be that the >>>> function has a bad name. Does the translate team need numbers on >>>> Translations that have been aborted due to page navigation or other issues? >>>> If they do, how can we still get them after this change? >>>> >>>> (I'm OK with the change per se, I just want to make sure we don't >>>> accidentally lose important data). >>>> >>>> >>>> On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: >>>> >>>>> There is already a >>>>> >>>>> chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc >>>>> in the tree. Do you want me to extend that ? >>>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>>> File chrome/browser/ui/views/translate/translate_bubble_view.cc (left): >>>>> >>>>> >>>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>>> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >>>>> TranslateBubbleView::WindowClosing() { >>>>> On 2016/01/26 01:07:22, groby wrote: >>>>> >>>>>> This means we would lose all metrics that are due to e.g. page >>>>>> >>>>> navigation. Do we >>>>> >>>>>> want a separate metric for that? Is it OK to do nothing with the model >>>>>> >>>>> if things >>>>> >>>>>> are closed e.g. due to Navigation? >>>>>> >>>>> >>>>> The function called "TranslationDeclined()" so no information related >>>>> to >>>>> the real decline of translation will be lost because the function will >>>>> still be called when the user really decline the translation. Only >>>>> information which is not related to decline of translation will be >>>>> lost, >>>>> but such information should not be tracked inside a function called >>>>> "TranslationDeclined() anyway. Page navigation has nothing to do with >>>>> decline of translation, right? >>>>> >>>>> https://codereview.chromium.org/1632953002/ >>>>> >>>> >>>> >>> >>> >>> -- >>> Frank Yung-Fong Tang >>> 譚永鋒 >>> Sr. Software Engineer / *Google Ţråñšļåţé* >>> >>> >> > > > -- > Frank Yung-Fong Tang > 譚永鋒 > Sr. Software Engineer / *Google Ţråñšļåţé* > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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/1632953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. 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.
Description was changed from ========== Move the calls to DeclineTranslation() from the WindowClosing() of TranslationBubbleView to the code handle the declien action to avoid unwanted calls while a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 ========== to ========== Fix the problem that currently chrome count user action which not explicit decline translation as decline and only count those explicit action. The following actions will not be counted as a decline action after this fix. a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 ==========
I add unit test and it now passed both gyp and ng build PTAL Is it enough? (I plan to add more unit test to cover other TranslateUIDelegate functions which I didn't change this time later) Or do you think I should add browser test also? Regards, Frank On 26 January 2016 at 11:08, Rachel Blum <groby@chromium.org> wrote: > SGTM > > On Tue, Jan 26, 2016 at 10:23 AM, Frank Tang (譚永鋒) <ftang@google.com> > wrote: > >> That is another way to solve it. The problem is really the name >> TranslationDeclined() imply the user has take some action the consciously >> decline the translation while what really happen is the translation prompt >> got closed for any reason. >> How about I change the implement of TranslationDeclined as you suggested >> but also add comment to the header of >> TranslationDeclined and later rename it to a better function name? >> >> On 25 January 2016 at 18:05, Rachel Blum <groby@chromium.org> wrote: >> >>> "Dismiss UI" means the UI has been closed for any reason that was not >>> initiated by the user. >>> >>> If it was initiated by the user, we count it instead as >>> Translate.DeclineTranslate - the distinction is made by |explicitly_closed| >>> in TranslationDeclined. >>> >>> Since you are trying to prevent a language being marked as "never >>> translate" only due to navigation, maybe that should be handled in >>> TranslateDeclined() instead, then? >>> >>> I.e. by changing the first line there from >>> >>> if (!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { >>> >>> to >>> >>> if (*explicitly_closed && *!translate_driver_ <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla... <https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...) { >>> >>> >>> That would mean that only explicit closing of the translate bubble is >>> counted against the 2 "decline" requests that lead to a block. As a bonus, >>> TranslateUiDelegate is a platform-neutral class, so we don't need to fix >>> any code on OSX when/if the bubble rolls out there. >>> >>> What do you think? >>> >>> >>> On Mon, Jan 25, 2016 at 5:55 PM, Frank Tang (譚永鋒) <ftang@google.com> >>> wrote: >>> >>>> Currently, the code inside TranslationDeclined will change the metrics >>>> >>>> Translate.DeclineTranslateDismissUI >>>> >>>> If it is not trigger by user explicit decline action. >>>> >>>> but what does this metrics really mean? >>>> >>>> What does "Dismiss UI" mean? >>>> >>>> >>>> >>>> On 25 January 2016 at 17:46, Rachel Blum <groby@chromium.org> wrote: >>>> >>>>> Yes, that's what I meant - sorry for being unclear. >>>>> >>>>> As for page navigation/TranslationDeclined - it might be that the >>>>> function has a bad name. Does the translate team need numbers on >>>>> Translations that have been aborted due to page navigation or other issues? >>>>> If they do, how can we still get them after this change? >>>>> >>>>> (I'm OK with the change per se, I just want to make sure we don't >>>>> accidentally lose important data). >>>>> >>>>> >>>>> On Mon, Jan 25, 2016 at 5:30 PM, <ftang@chromium.org> wrote: >>>>> >>>>>> There is already a >>>>>> >>>>>> chromium/src/chrome/browser/ui/views/translate/translate_bubble_view_browsertest.cc >>>>>> in the tree. Do you want me to extend that ? >>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>>>> File chrome/browser/ui/views/translate/translate_bubble_view.cc >>>>>> (left): >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/1632953002/diff/1/chrome/browser/ui/views/tra... >>>>>> chrome/browser/ui/views/translate/translate_bubble_view.cc:169: void >>>>>> TranslateBubbleView::WindowClosing() { >>>>>> On 2016/01/26 01:07:22, groby wrote: >>>>>> >>>>>>> This means we would lose all metrics that are due to e.g. page >>>>>>> >>>>>> navigation. Do we >>>>>> >>>>>>> want a separate metric for that? Is it OK to do nothing with the >>>>>>> model >>>>>>> >>>>>> if things >>>>>> >>>>>>> are closed e.g. due to Navigation? >>>>>>> >>>>>> >>>>>> The function called "TranslationDeclined()" so no information related >>>>>> to >>>>>> the real decline of translation will be lost because the function will >>>>>> still be called when the user really decline the translation. Only >>>>>> information which is not related to decline of translation will be >>>>>> lost, >>>>>> but such information should not be tracked inside a function called >>>>>> "TranslationDeclined() anyway. Page navigation has nothing to do with >>>>>> decline of translation, right? >>>>>> >>>>>> https://codereview.chromium.org/1632953002/ >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Frank Yung-Fong Tang >>>> 譚永鋒 >>>> Sr. Software Engineer / *Google Ţråñšļåţé* >>>> >>>> >>> >> >> >> -- >> Frank Yung-Fong Tang >> 譚永鋒 >> Sr. Software Engineer / *Google Ţråñšļåţé* >> >> > -- Frank Yung-Fong Tang 譚永鋒 Sr. Software Engineer / *Google Ţråñšļåţé* -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (left): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:225: if (!translate_driver_->IsOffTheRecord()) { Overall, I'd prefer it if you'd just add && explicitly_closed here - large if blocks decrease readability, IMHO https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:238: if (explicitly_closed && translate_manager_) { If you keep this structure, no need to check |explicitly_closed| here - see larger if block https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:94: // Pass explicitly_closed as true if user explicityly decline the nit: |explicitly_closed|, please - here and elsewhere. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:28: class MockTranslateDriver : public TranslateDriver { There's already a MockTranslateDriver in components/translate/core/browser/language_state_unittest.cc I'd suggest factoring that out. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:46: MOCK_METHOD0(GetTranslateDriver, TranslateDriver*()); I'd have the mock take a driver in its ctor - and then just return that all the time. Saves clients to do the EXPECT_CALL dance. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:47: MOCK_METHOD0(GetPrefs, PrefService*()); If you rely on prefs in any way, shouldn't this return |pref_service_| from the test fixture? Either via a member passed in at ctor, or via EXPECT_CALL? https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:49: MOCK_METHOD0(GetTranslatePrefsMock, TranslatePrefs*()); I'm not sure why you're using a MOCK_METHOD here - doesn't it just return NULL? https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:61: CreateInfoBarMock(delegate.get())); You want to do new TranslateInfoBar(std::move(delegate)) - otherwise, the delegate will be deleted when you leave function scope. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:83: const char* preferred_languages_prefs = NULL; I think you can always just assume NULL for this. You're not testing CrOS-specific behavior. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:93: .WillRepeatedly(Return(prefs_)); That won't work - when the first return value is out of scope, it will delete |prefs_|. (scoped_ptr is similar to std::unique_ptr) GetTranslatePrefs is actually a factory method for scoped_ptr<TranslatePrefs> objects, so you'll need to allocate a new one on every call. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:105: prefs_->IncrementTranslationDeniedCount("ar"); Are you sure this should be in SetUp? It seems setup for your particular tests, not UiDelegate tests in general. (And I don't think you need the Reset, because each test case will receive new - and empty - preferences) https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:110: void TearDown() override { No need to override if it does nothing. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:116: TranslatePrefs* prefs_; You want this a scoped_ptr, I think. Otherwise, you'd leak it. Or you could just have TranslatePrefs as a scoped object in SetUp - they're designed to be short-lived. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:131: ASSERT_EQ(accepted_count, prefs_->GetTranslationAcceptedCount("ar")); You probably want EXPECT_EQ/EXPECT_FALSE. No need to abort the tests if one expectation fails. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:150: // TODO(ftang) Currently this file only test TranslationDeclined(), we You sure you want to take this on as your own TODO? :)
thanks for your comment. I will work on it. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (left): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:225: if (!translate_driver_->IsOffTheRecord()) { On 2016/01/28 21:17:28, groby wrote: > Overall, I'd prefer it if you'd just add && explicitly_closed here - large if > blocks decrease readability, IMHO Done. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:225: if (!translate_driver_->IsOffTheRecord()) { On 2016/01/28 21:17:28, groby wrote: > Overall, I'd prefer it if you'd just add && explicitly_closed here - large if > blocks decrease readability, IMHO Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:238: if (explicitly_closed && translate_manager_) { On 2016/01/28 21:17:28, groby wrote: > If you keep this structure, no need to check |explicitly_closed| here - see > larger if block Done. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.h (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.h:94: // Pass explicitly_closed as true if user explicityly decline the On 2016/01/28 21:17:28, groby wrote: > nit: |explicitly_closed|, please - here and elsewhere. Done. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:28: class MockTranslateDriver : public TranslateDriver { On 2016/01/28 21:17:28, groby wrote: > There's already a MockTranslateDriver in > components/translate/core/browser/language_state_unittest.cc > > I'd suggest factoring that out. Those are not using gmock. Do we have a directory to all the mock? If I factoring that out, where should I place it? https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:46: MOCK_METHOD0(GetTranslateDriver, TranslateDriver*()); On 2016/01/28 21:17:28, groby wrote: > I'd have the mock take a driver in its ctor - and then just return that all the > time. Saves clients to do the EXPECT_CALL dance. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:47: MOCK_METHOD0(GetPrefs, PrefService*()); On 2016/01/28 21:17:28, groby wrote: > If you rely on prefs in any way, shouldn't this return |pref_service_| from the > test fixture? Either via a member passed in at ctor, or via EXPECT_CALL? Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:49: MOCK_METHOD0(GetTranslatePrefsMock, TranslatePrefs*()); On 2016/01/28 21:17:28, groby wrote: > I'm not sure why you're using a MOCK_METHOD here - doesn't it just return NULL? gmock cannot mock scoped_ptr<class> so I follow some suggest pattern to deal the scoped_ptr<> in parameters and return. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:61: CreateInfoBarMock(delegate.get())); On 2016/01/28 21:17:28, groby wrote: > You want to do new TranslateInfoBar(std::move(delegate)) - otherwise, the > delegate will be deleted when you leave function scope. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:83: const char* preferred_languages_prefs = NULL; On 2016/01/28 21:17:28, groby wrote: > I think you can always just assume NULL for this. You're not testing > CrOS-specific behavior. but if I always assume NULL here. Won't the test break when running the test under OS_CHROMEOS? https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:93: .WillRepeatedly(Return(prefs_)); On 2016/01/28 21:17:28, groby wrote: > That won't work - when the first return value is out of scope, it will delete > |prefs_|. (scoped_ptr is similar to std::unique_ptr) > > GetTranslatePrefs is actually a factory method for scoped_ptr<TranslatePrefs> > objects, so you'll need to allocate a new one on every call. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:105: prefs_->IncrementTranslationDeniedCount("ar"); On 2016/01/28 21:17:28, groby wrote: > Are you sure this should be in SetUp? It seems setup for your particular tests, > not UiDelegate tests in general. (And I don't think you need the Reset, because > each test case will receive new - and empty - preferences) Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:110: void TearDown() override { On 2016/01/28 21:17:28, groby wrote: > No need to override if it does nothing. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:116: TranslatePrefs* prefs_; On 2016/01/28 21:17:28, groby wrote: > You want this a scoped_ptr, I think. Otherwise, you'd leak it. Or you could just > have TranslatePrefs as a scoped object in SetUp - they're designed to be > short-lived. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:131: ASSERT_EQ(accepted_count, prefs_->GetTranslationAcceptedCount("ar")); On 2016/01/28 21:17:28, groby wrote: > You probably want EXPECT_EQ/EXPECT_FALSE. No need to abort the tests if one > expectation fails. Acknowledged. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:150: // TODO(ftang) Currently this file only test TranslationDeclined(), we On 2016/01/28 21:17:28, groby wrote: > You sure you want to take this on as your own TODO? :) yes
address review comment
groby- please advise if I factoring out the mock class where should I place it. I haven't do that part yet.
Ah, sorry - that was stuck in the draft stage. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:28: class MockTranslateDriver : public TranslateDriver { On 2016/01/28 22:37:21, ftang wrote: > On 2016/01/28 21:17:28, groby wrote: > > There's already a MockTranslateDriver in > > components/translate/core/browser/language_state_unittest.cc > > > > I'd suggest factoring that out. > > Those are not using gmock. Do we have a directory to all the mock? If I > factoring that out, where should I place it? We usually place them right next to the header. So here, in components/translate/core/browser. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:49: MOCK_METHOD0(GetTranslatePrefsMock, TranslatePrefs*()); On 2016/01/28 22:37:21, ftang wrote: > On 2016/01/28 21:17:28, groby wrote: > > I'm not sure why you're using a MOCK_METHOD here - doesn't it just return > NULL? > > gmock cannot mock scoped_ptr<class> so I follow some suggest pattern to deal the > scoped_ptr<> in parameters and return. Hm - is that suggested in Chrome, or by gmock? We usually just write stub methods if it can't be created via MOCK_METHOD. I.e. here scoped_ptr<TranslatePrefs> GetTranslatePrefs() { return make_scoped_ptr(new TranslatePrefs(prefs_)); } (As said below, you'll need to create a new object every time GetTranslatePrefs is called) https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:83: const char* preferred_languages_prefs = NULL; On 2016/01/28 22:37:21, ftang wrote: > On 2016/01/28 21:17:28, groby wrote: > > I think you can always just assume NULL for this. You're not testing > > CrOS-specific behavior. > > but if I always assume NULL here. Won't the test break when running the test > under OS_CHROMEOS? You're right, it will. I forgot TranslatePrefs DCHECKs the value. :( https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:150: // TODO(ftang) Currently this file only test TranslationDeclined(), we On 2016/01/28 22:37:22, ftang wrote: > On 2016/01/28 21:17:28, groby wrote: > > You sure you want to take this on as your own TODO? :) > > yes Thank you for volunteering, then!
refactor out MockTranslteDriver to .h file
groby PTAL https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate_unittest.cc (right): https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:49: MOCK_METHOD0(GetTranslatePrefsMock, TranslatePrefs*()); On 2016/01/29 20:09:16, groby wrote: > On 2016/01/28 22:37:21, ftang wrote: > > On 2016/01/28 21:17:28, groby wrote: > > > I'm not sure why you're using a MOCK_METHOD here - doesn't it just return > > NULL? > > > > gmock cannot mock scoped_ptr<class> so I follow some suggest pattern to deal > the > > scoped_ptr<> in parameters and return. > > Hm - is that suggested in Chrome, or by gmock? We usually just write stub > methods if it can't be created via MOCK_METHOD. > > I.e. here > > scoped_ptr<TranslatePrefs> GetTranslatePrefs() { > return make_scoped_ptr(new TranslatePrefs(prefs_)); > } > > (As said below, you'll need to create a new object every time GetTranslatePrefs > is called) Done. https://codereview.chromium.org/1632953002/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate_unittest.cc:150: // TODO(ftang) Currently this file only test TranslationDeclined(), we On 2016/01/29 20:09:16, groby wrote: > On 2016/01/28 22:37:22, ftang wrote: > > On 2016/01/28 21:17:28, groby wrote: > > > You sure you want to take this on as your own TODO? :) > > > > yes > > Thank you for volunteering, then! Acknowledged.
Make MacOS compiled by split the MockTranslateDriver implementation into .cc file
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/1632953002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. 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.
PTAL
lgtm
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/1632953002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/180001
The CQ bit was unchecked by ftang@chromium.org
The CQ bit was checked by ftang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1632953002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ftang@chromium.org changed reviewers: + hajimehoshi@chromium.org
hajimehoshi- I need your review since you are in the OWNERS file but not groby.
Sorry for the delayed review. lgtm
The CQ bit was checked by ftang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1632953002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
fix mac unit test breakage
The CQ bit was checked by ftang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org, hajimehoshi@chromium.org Link to the patchset: https://codereview.chromium.org/1632953002/#ps200001 (title: "fix mac unit test breakage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1632953002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632953002/200001
Message was sent while issue was closed.
Description was changed from ========== Fix the problem that currently chrome count user action which not explicit decline translation as decline and only count those explicit action. The following actions will not be counted as a decline action after this fix. a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 ========== to ========== Fix the problem that currently chrome count user action which not explicit decline translation as decline and only count those explicit action. The following actions will not be counted as a decline action after this fix. a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fix the problem that currently chrome count user action which not explicit decline translation as decline and only count those explicit action. The following actions will not be counted as a decline action after this fix. a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 ========== to ========== Fix the problem that currently chrome count user action which not explicit decline translation as decline and only count those explicit action. The following actions will not be counted as a decline action after this fix. a. User open a new tab/window before confirm the translation b. User switch to a new tab/window to load page before confirm the translation BUG=575023 Committed: https://crrev.com/54d1294f337c5d7fe0bc474e9ae34d38913dc1f3 Cr-Commit-Position: refs/heads/master@{#376016} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/54d1294f337c5d7fe0bc474e9ae34d38913dc1f3 Cr-Commit-Position: refs/heads/master@{#376016}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1632953002/diff/200001/components/translate/c... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/1632953002/diff/200001/components/translate/c... components/translate/core/browser/BUILD.gn:96: sources += [ "translate_ui_delegate_unittest.cc" ] This file is built unconditionally in the gyp build but also if !use_aura in the gn build. Which one is right? (I'm trying to make sure that every test present in the gyp build is also present in the gn build, and this is one of the differences.)
Message was sent while issue was closed.
Ping! We need to know whether the discrepancy between the gyp and gn builds is intentional or not, in order to see whether having translate_ui_delegate_unittest.cc missing from most gn builds is normal and expected or a bug.
Message was sent while issue was closed.
On 2016/05/20 18:49:38, brucedawson wrote: > Ping! We need to know whether the discrepancy between the gyp and gn builds is > intentional or not, in order to see whether having > translate_ui_delegate_unittest.cc missing from most gn builds is normal and > expected or a bug. see https://codereview.chromium.org/1987073003/ |
