Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894553002:
Replace OnPageTranslate Obsever by a responder of delegate. (Closed)
DescriptionReplace OnPageTranslate Observer by a responder of delegate.
In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver.
But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all.
In order to fix the loose relationship, we create a responder inside the delegate.
So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate.
What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it.
BUG=720164, 703887, 723426, 724428
TBR=dfalcantara@chromium.org
Review-Url: https://codereview.chromium.org/2894553002
Cr-Commit-Position: refs/heads/master@{#473799}
Committed: https://chromium.googlesource.com/chromium/src/+/e65dd51e10accc80aa2be79129912f47357cb958
Patch Set 1 #
Total comments: 14
Patch Set 2 : fix #
Total comments: 20
Patch Set 3 : Fix Comments #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== Replace OnPageTransalte Obsever by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Transalate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. BUG=720164,703887 ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. BUG=720164,703887 ==========
googleo@chromium.org changed reviewers: + groby@chromium.org
The CQ bit was checked by googleo@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...
This CL is used to fix a no-connection bug. PLTA. Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
googleo@chromium.org changed reviewers: + napper@chromium.org
+ napper and cc clingon
https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:23: TranslateCompactInfoBar::TranslateCompactInfoBar( Dont you need to initialize action_flags_ here? https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:161: JNIEnv* env = base::android::AttachCurrentThread(); Can this be: const JNIEnv* const env ... or at least: JNIEnv* const env ... https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:168: return action_flags == 0; It might be clearer if you had: FLAG_NONE = 0 then return action_flags == FLAG_NONE; https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags; action_flags_; https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:67: // Action flags Add '.' at end of comment https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:68: const int FLAG_TRANSLATE = 1 << 0; Consider making these an enum https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... components/translate/core/browser/translate_infobar_delegate.h:240: Responder* responder_; Responder* const responder_;
Description was changed from ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. BUG=720164,703887 ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. BUG=720164,703887,723426 ==========
Thanks John. PTAL. Great to learn the C++ technique from your comments. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:23: TranslateCompactInfoBar::TranslateCompactInfoBar( On 2017/05/19 00:17:50, napper wrote: > Dont you need to initialize action_flags_ here? Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:161: JNIEnv* env = base::android::AttachCurrentThread(); On 2017/05/19 00:17:50, napper wrote: > Can this be: > > const JNIEnv* const env ... > > or at least: > > JNIEnv* const env ... Not sure whether JNI framework has treated the env args as const in method Java_Class*_Method* But I checked all usages. None of them set const for the env. https://cs.chromium.org/search/?q=AttachCurrentThread&type=cs https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:168: return action_flags == 0; On 2017/05/19 00:17:50, napper wrote: > It might be clearer if you had: > > FLAG_NONE = 0 > > then > > return action_flags == FLAG_NONE; Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags; On 2017/05/19 00:17:50, napper wrote: > action_flags_; Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:67: // Action flags On 2017/05/19 00:17:50, napper wrote: > Add '.' at end of comment Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:68: const int FLAG_TRANSLATE = 1 << 0; On 2017/05/19 00:17:50, napper wrote: > Consider making these an enum Done. https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... components/translate/core/browser/translate_infobar_delegate.h:240: Responder* responder_; On 2017/05/19 00:17:50, napper wrote: > Responder* const responder_; We can talk this offline.
Thanks John. PTAL. Great to learn the C++ technique from your comments. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:23: TranslateCompactInfoBar::TranslateCompactInfoBar( On 2017/05/19 00:17:50, napper wrote: > Dont you need to initialize action_flags_ here? Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:161: JNIEnv* env = base::android::AttachCurrentThread(); On 2017/05/19 00:17:50, napper wrote: > Can this be: > > const JNIEnv* const env ... > > or at least: > > JNIEnv* const env ... Not sure whether JNI framework has treated the env args as const in method Java_Class*_Method* But I checked all usages. None of them set const for the env. https://cs.chromium.org/search/?q=AttachCurrentThread&type=cs https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:168: return action_flags == 0; On 2017/05/19 00:17:50, napper wrote: > It might be clearer if you had: > > FLAG_NONE = 0 > > then > > return action_flags == FLAG_NONE; Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags; On 2017/05/19 00:17:50, napper wrote: > action_flags_; Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:67: // Action flags On 2017/05/19 00:17:50, napper wrote: > Add '.' at end of comment Acknowledged. https://codereview.chromium.org/2894553002/diff/1/chrome/browser/ui/android/i... chrome/browser/ui/android/infobars/translate_compact_infobar.h:68: const int FLAG_TRANSLATE = 1 << 0; On 2017/05/19 00:17:50, napper wrote: > Consider making these an enum Done. https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/1/components/translate/core/b... components/translate/core/browser/translate_infobar_delegate.h:240: Responder* responder_; On 2017/05/19 00:17:50, napper wrote: > Responder* const responder_; We can talk this offline.
Description was changed from ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. BUG=720164,703887,723426 ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426 ==========
Description was changed from ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426 ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426, 724428 ==========
https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:65: action_flags_ |= FLAG_TRANSLATE; SHould this flag be traced on the delegate instead? https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (left): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:48: void OnPageTranslated(const std::string& original_lang, Who is notifying the ContentTranslateDriver now? https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags_; This should be of type ActionFlag if you use the enum. As a _personal_ preference, I'd prefer a bitfield. But ultimately, either is fine. struct { bool did_translate_ : 1; bool did_revert_ : 1; ... } flags; https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.cc (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.cc:359: TranslateInfoBarDelegate::TranslateInfoBarDelegate( You do want to initialize |responder_| https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:40: // A responder to handle different translate steps' UI changes. If I remember correctly, you don't need this class at all. ChromeTranslateClient::CreateInfobar is called every single time the TranslateStep changes. You can query that value from the delegate. (See chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm for an example) If I read the Android code correctly, that means you're rebulding the Infobar every time TranslateStep changes anyways, with a new delegate. Can you use that fact instead of adding an additional object? (Or is this subtly different on Android, and I am misunderstanding?) https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:41: class Responder { We usually call those "Observer" https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:44: virtual void ActionOnStep(translate::TranslateStep step, Suggested name: OnTranslateStepChanged() That's closer to the usual Chrome naming scheme. https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:46: // Return whether user declined translate service. Does "declined" include dismissing the infobar, or is it an active decline only?
Thanks a lot for the reviewing. I replied on some crucial comments first. Hope these explanations are clear and make sense. If you agree with adding responder (observer) in the translate delegate, I will fix the other comments. https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.cc (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.cc:65: action_flags_ |= FLAG_TRANSLATE; On 2017/05/20 21:30:39, groby wrote: > SHould this flag be traced on the delegate instead? The actions traced here are mostly frontend-specific behaviors. Since the new user flow is very different from the original one, it's hard to integrate the different flow in the same delegate used by all original clients. What's more, the tracing is only used in new UX, tracing them here are easier and straightforward. https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (left): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:48: void OnPageTranslated(const std::string& original_lang, On 2017/05/20 21:30:39, groby wrote: > Who is notifying the ContentTranslateDriver now? This method is used when ContentTranslateDriver finished translation and notify the observer to update its state. With or without a observer, translate manager will be called when page translation finished. https://cs.chromium.org/chromium/src/components/translate/content/browser/con... So the following things will always happen with or without observer. ContentTranslateDriver:OnPageTranslated >> TranslateManager::PageTranslated >> TranslateClient::ShowTranslateUI >> TranslateInfoBarDelegate::Create So I remove this observer stuff in compact infobar but respond the onPageTranslated event inside TranslateInfoBarDelegate https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:40: // A responder to handle different translate steps' UI changes. On 2017/05/20 21:30:39, groby wrote: > If I remember correctly, you don't need this class at all. > > ChromeTranslateClient::CreateInfobar is called every single time the > TranslateStep changes. You can query that value from the delegate. (See > chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm for an example) > > If I read the Android code correctly, that means you're rebulding the Infobar > every time TranslateStep changes anyways, with a new delegate. > > Can you use that fact instead of adding an additional object? (Or is this subtly > different on Android, and I am misunderstanding?) > > I think the fact you described is the original logic for handling each step of the translation. In our new design, translate infobar will be created only once then it will be reused for responding all the possible steps later. Eg. after showing the initialized UI, when user asks for translating. Old one will create a "Translating" UI to replace initialized UI but in the new design, we will modify part of the current UI to indicate that it's being translating. Since the rule of creating its own UI for each step is defined in very common level and followed by its hierarchical logics, which means breaking the rule will take a lots of cost(eg. in bubble UI, translateDelegate is abandoned and they have to create quite a few modules to make it work...) In our case, in order to reuse the current code as much as possible, adding a responder(or observer) inside translateDelegate is the smallest change we can do.
https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags_; On 2017/05/20 at 21:30:39, groby wrote: > This should be of type ActionFlag if you use the enum. I think the int type is correct here, since the enum specifies bit flags, not values. The following would not be correct for an enum but is correct for an int type: action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; But I agree, a bitfield might be cleaner.
Hey Rachel, Sorry for my long sentences but just want to explain my idea clearer. There were 2 P1 bugs filed due to the Driver's observer stuff, which blocked us to launch this new feature. So hopefully we can land this fix ASAP. If there is anything which doesn't make sense to you, we can have a VC meeting to discuss them. WDYT. https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags_; On 2017/05/21 23:24:16, napper wrote: > On 2017/05/20 at 21:30:39, groby wrote: > > This should be of type ActionFlag if you use the enum. > > I think the int type is correct here, since the enum specifies bit flags, not > values. The following would not be correct for an enum but is correct for an int > type: > > action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; > > But I agree, a bitfield might be cleaner. After a little discuss offline with Jon. Seems the initialization of the struct is not as simple as int and it may introduce other considerations. How about let me keep this way and refactor it in another CL. https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.cc (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.cc:359: TranslateInfoBarDelegate::TranslateInfoBarDelegate( On 2017/05/20 21:30:39, groby wrote: > You do want to initialize |responder_| Thanks Done https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:41: class Responder { On 2017/05/20 21:30:39, groby wrote: > We usually call those "Observer" Thanks for questioning the class name, I have struggled quite a while to name it. "Observer" was one of my options and "Responder" was closer. The role of this class here is kind of A commitment. If Any delegate was assigned with this class, it means this delegate will let it (the class assigned) to handle all the possible steps without recreating another delegate or infobar. Now I am persuaded, so I Changed the name to "Observer". https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:44: virtual void ActionOnStep(translate::TranslateStep step, On 2017/05/20 21:30:39, groby wrote: > Suggested name: OnTranslateStepChanged() > > That's closer to the usual Chrome naming scheme. Thanks, Done. https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:46: // Return whether user declined translate service. On 2017/05/20 21:30:39, groby wrote: > Does "declined" include dismissing the infobar, or is it an active decline only? "declined" here is just a flag used to evaluate user's behaviors. PM and UX of the new design, want to understand users better, in our user flow, "declined" means things below 1, user closed the infobar without using it at all. 2, user didn't touch the infobar at all until the page got navigated 3, user kept the infobar for a few seconds (eg. 20s) without touching anything... All these behaviors are recorded in the front-end and before user closing infobar, we need a clear decision about whether this infobar server is declined by user or not. If yes, we will commit a "Declined" logging. So the method itself is just a few checking, and doesn't introduce any operation of the infobar. I will rename it to IsDeclinedByUser()
The code itself is lgtm I would very much appreciate it if you could add a unit test or two - but given that you're fixing a crasher, I'm OK with postponing that to another CL if that turns out complicated. (If it's easy and you can add it here, please do, and feel free to just reuse the LGTM after local review from Jon) https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags_; On 2017/05/21 23:24:16, napper wrote: > On 2017/05/20 at 21:30:39, groby wrote: > > This should be of type ActionFlag if you use the enum. > > I think the int type is correct here, since the enum specifies bit flags, not > values. The following would not be correct for an enum but is correct for an int > type: > > action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; > > But I agree, a bitfield might be cleaner. You are of course right about the int - sorry about the confusion. https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int action_flags_; On 2017/05/22 09:08:10, Leo wrote: > On 2017/05/21 23:24:16, napper wrote: > > On 2017/05/20 at 21:30:39, groby wrote: > > > This should be of type ActionFlag if you use the enum. > > > > I think the int type is correct here, since the enum specifies bit flags, not > > values. The following would not be correct for an enum but is correct for an > int > > type: > > > > action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; > > > > But I agree, a bitfield might be cleaner. > > After a little discuss offline with Jon. Seems the initialization of the struct > is not as simple as int and it may introduce other considerations. How about let > me keep this way and refactor it in another CL. No need to refactor if it introduces many other issues. (As I said, it's a personal preference of mine - but just because I like something a bit better aesthethically doesn't mean you should have a lot of extra work :) https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... File components/translate/core/browser/translate_infobar_delegate.h (right): https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... components/translate/core/browser/translate_infobar_delegate.h:46: // Return whether user declined translate service. On 2017/05/22 09:08:10, Leo wrote: > On 2017/05/20 21:30:39, groby wrote: > > Does "declined" include dismissing the infobar, or is it an active decline > only? > > "declined" here is just a flag used to evaluate user's behaviors. PM and UX of > the new design, want to understand users better, in our user flow, "declined" > means things below > 1, user closed the infobar without using it at all. > 2, user didn't touch the infobar at all until the page got navigated > 3, user kept the infobar for a few seconds (eg. 20s) without touching > anything... > > All these behaviors are recorded in the front-end and before user closing > infobar, we need a clear decision about whether this infobar server is declined > by user or not. If yes, we will commit a "Declined" logging. > > So the method itself is just a few checking, and doesn't introduce any operation > of the infobar. > > I will rename it to IsDeclinedByUser() Thank you - the reason I asked was that we use "Declined" for many different things in the translate system. It's very helpful to have a name that explains!
On 2017/05/22 14:34:11, groby wrote: > The code itself is lgtm > > I would very much appreciate it if you could add a unit test or two - but given > that you're fixing a crasher, I'm OK with postponing that to another CL if that > turns out complicated. (If it's easy and you can add it here, please do, and > feel free to just reuse the LGTM after local review from Jon) > > https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... > File chrome/browser/ui/android/infobars/translate_compact_infobar.h (right): > > https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... > chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int > action_flags_; > On 2017/05/21 23:24:16, napper wrote: > > On 2017/05/20 at 21:30:39, groby wrote: > > > This should be of type ActionFlag if you use the enum. > > > > I think the int type is correct here, since the enum specifies bit flags, not > > values. The following would not be correct for an enum but is correct for an > int > > type: > > > > action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; > > > > But I agree, a bitfield might be cleaner. > > You are of course right about the int - sorry about the confusion. > > https://codereview.chromium.org/2894553002/diff/20001/chrome/browser/ui/andro... > chrome/browser/ui/android/infobars/translate_compact_infobar.h:65: unsigned int > action_flags_; > On 2017/05/22 09:08:10, Leo wrote: > > On 2017/05/21 23:24:16, napper wrote: > > > On 2017/05/20 at 21:30:39, groby wrote: > > > > This should be of type ActionFlag if you use the enum. > > > > > > I think the int type is correct here, since the enum specifies bit flags, > not > > > values. The following would not be correct for an enum but is correct for an > > int > > > type: > > > > > > action_flags_ = FLAG_TRANSLATE | FLAG_NEVER_LANGUAGE; > > > > > > But I agree, a bitfield might be cleaner. > > > > After a little discuss offline with Jon. Seems the initialization of the > struct > > is not as simple as int and it may introduce other considerations. How about > let > > me keep this way and refactor it in another CL. > > No need to refactor if it introduces many other issues. (As I said, it's a > personal preference of mine - but just because I like something a bit better > aesthethically doesn't mean you should have a lot of extra work :) > > https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... > File components/translate/core/browser/translate_infobar_delegate.h (right): > > https://codereview.chromium.org/2894553002/diff/20001/components/translate/co... > components/translate/core/browser/translate_infobar_delegate.h:46: // Return > whether user declined translate service. > On 2017/05/22 09:08:10, Leo wrote: > > On 2017/05/20 21:30:39, groby wrote: > > > Does "declined" include dismissing the infobar, or is it an active decline > > only? > > > > "declined" here is just a flag used to evaluate user's behaviors. PM and UX of > > the new design, want to understand users better, in our user flow, "declined" > > means things below > > 1, user closed the infobar without using it at all. > > 2, user didn't touch the infobar at all until the page got navigated > > 3, user kept the infobar for a few seconds (eg. 20s) without touching > > anything... > > > > All these behaviors are recorded in the front-end and before user closing > > infobar, we need a clear decision about whether this infobar server is > declined > > by user or not. If yes, we will commit a "Declined" logging. > > > > So the method itself is just a few checking, and doesn't introduce any > operation > > of the infobar. > > > > I will rename it to IsDeclinedByUser() > > Thank you - the reason I asked was that we use "Declined" for many different > things in the translate system. It's very helpful to have a name that explains! Thanks for understanding Rachel. I did some research on test case of translate_infobar_delegate and I think it's better to add the test case in another CL. Mostly because InfobarManager (a very important part of delegate testing) missing mock stuff and so as its dependency, which may take more time than I thought. But it should be fixed this week. Thanks again.
Description was changed from ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426, 724428 ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426, 724428 TBR=dfalcantara@chromium.org ==========
The CQ bit was checked by googleo@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...
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 googleo@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": 40001, "attempt_start_ts": 1495509445288820,
"parent_rev": "489ea56faf3d58a0c940cec510a1dc514747d64c", "commit_rev":
"e65dd51e10accc80aa2be79129912f47357cb958"}
Message was sent while issue was closed.
Description was changed from ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426, 724428 TBR=dfalcantara@chromium.org ========== to ========== Replace OnPageTranslate Observer by a responder of delegate. In order to be notified that page is translated, we simply implement CompactInfobar as observer of Translate Driver. But now we found from the translate button is clicked to the page is translated, there are many steps. If any step is returned, we are not be notified at all. In order to fix the loose relationship, we create a responder inside the delegate. So no matter what delegate is going to do, we will be notified, which keep infobar always consistent with delegate. What's more, on some corner cases listed in bug/723426, the relationship between infobar and translate driver causes crash. And the new strong relationship will fix it. BUG=720164,703887,723426, 724428 TBR=dfalcantara@chromium.org Review-Url: https://codereview.chromium.org/2894553002 Cr-Commit-Position: refs/heads/master@{#473799} Committed: https://chromium.googlesource.com/chromium/src/+/e65dd51e10accc80aa2be7912991... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e65dd51e10accc80aa2be7912991... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
