From 1c31e29c9a6f257ee3130fae93ae21968d71c881 Mon Sep 17 00:00:00 2001 From: "pkasting@chromium.org" Date: Fri, 22 Nov 2013 00:02:56 +0000 Subject: [PATCH] Android infobar cleanup: * Don't return values callers don't use * Rename a temp in anticipation of refactor patch * Use Pass() where PassAs<>() is not necessary * Minor comment/whitespace changes * Don't NULL-check web_contents() when we're owned (should never be NULL) * Don't bother storing delegate pointers on infobar subclasses, just downcast the base class' pointer (matches other platforms) * Remove unnecessary DCHECKs (either other code performs them or they will no longer be necesssary in the refactored world) * Remove unnecessary temps * Speed up vector manipulation by not creating a bunch of objects we then overwrite * DCHECK(expected, actual) BUG=none TEST=none R=miguelg@chromium.org Review URL: https://codereview.chromium.org/77773005 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236640 0039d316-1c4b-4281-b951-d872f2087c98 --- .../popup_blocked_infobar_delegate.cc | 17 +++--- .../popup_blocked_infobar_delegate.h | 4 +- .../auto_login_infobar_delegate_android.cc | 46 +++++++------- .../browser/ui/android/infobars/confirm_infobar.cc | 22 ++++--- .../browser/ui/android/infobars/confirm_infobar.h | 2 +- .../browser/ui/android/infobars/infobar_android.cc | 11 +--- .../browser/ui/android/infobars/infobar_android.h | 2 - .../android/infobars/infobar_container_android.cc | 6 +- .../ui/android/infobars/translate_infobar.cc | 70 ++++++++++++---------- .../ui/android/infobars/translate_infobar.h | 3 +- 10 files changed, 93 insertions(+), 90 deletions(-) diff --git a/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.cc b/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.cc index 8aaa7d7d3957..6b76433e7ce1 100644 --- a/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.cc +++ b/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.cc @@ -16,10 +16,9 @@ // static -InfoBarDelegate* PopupBlockedInfoBarDelegate::Create( - InfoBarService* infobar_service, - int num_popups) { - scoped_ptr new_delegate( +void PopupBlockedInfoBarDelegate::Create(InfoBarService* infobar_service, + int num_popups) { + scoped_ptr infobar( new PopupBlockedInfoBarDelegate(infobar_service, num_popups)); // See if there is an existing popup infobar already. @@ -27,14 +26,14 @@ InfoBarDelegate* PopupBlockedInfoBarDelegate::Create( // will be shown once, then hide then be shown again. // This will be fixed once we have an in place replace infobar mechanism. for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { - if (infobar_service->infobar_at(i)->AsPopupBlockedInfoBarDelegate()) { - return infobar_service->ReplaceInfoBar( - infobar_service->infobar_at(i), - new_delegate.PassAs()); + InfoBarDelegate* existing_infobar = infobar_service->infobar_at(i); + if (existing_infobar->AsPopupBlockedInfoBarDelegate()) { + infobar_service->ReplaceInfoBar(existing_infobar, infobar.Pass()); + return; } } - return infobar_service->AddInfoBar(new_delegate.PassAs()); + infobar_service->AddInfoBar(infobar.Pass()); } PopupBlockedInfoBarDelegate::~PopupBlockedInfoBarDelegate() { diff --git a/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.h b/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.h index 1000955e0f30..004146e90730 100644 --- a/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.h +++ b/chrome/browser/ui/android/content_settings/popup_blocked_infobar_delegate.h @@ -11,8 +11,8 @@ class InfoBarService; class PopupBlockedInfoBarDelegate : public ConfirmInfoBarDelegate { public: - static InfoBarDelegate* Create(InfoBarService* infobar_service, - int num_popups); + // Creates a popup blocked infobar delegate and adds it to |infobar_service|. + static void Create(InfoBarService* infobar_service, int num_popups); virtual ~PopupBlockedInfoBarDelegate(); private: diff --git a/chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc b/chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc index 47b9695e66f8..c6df272490b9 100644 --- a/chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc +++ b/chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc @@ -19,6 +19,7 @@ using base::android::ConvertUTF8ToJavaString; using base::android::ScopedJavaLocalRef; + AutoLoginInfoBarDelegateAndroid::AutoLoginInfoBarDelegateAndroid( InfoBarService* owner, const Params& params) @@ -61,10 +62,8 @@ bool AutoLoginInfoBarDelegateAndroid::Accept() { ScopedJavaLocalRef delegate = weak_java_auto_login_delegate_.get(env); DCHECK(delegate.obj()); - Java_AutoLoginDelegate_logIn(env, delegate.obj(), reinterpret_cast(this)); - // Do not close the infobar on accept, it will be closed as part // of the log in callback. return false; @@ -83,29 +82,32 @@ bool AutoLoginInfoBarDelegateAndroid::Cancel() { void AutoLoginInfoBarDelegateAndroid::LoginSuccess(JNIEnv* env, jobject obj, jstring result) { - if (owner()) { - content::WebContents* web_contents = owner()->web_contents(); - if (web_contents) { - web_contents->Stop(); - web_contents->OpenURL(content::OpenURLParams( - GURL(base::android::ConvertJavaStringToUTF8(env, result)), - content::Referrer(), CURRENT_TAB, - content::PAGE_TRANSITION_AUTO_BOOKMARK, false)); - } - owner()->RemoveInfoBar(this); - } + if (!owner()) + return; // We're closing; don't call anything, it might access the owner. + + // TODO(miguelg): Test whether the Stop() and RemoveInfoBar() calls here are + // necessary, or whether OpenURL() will do this for us. + content::WebContents* web_contents = owner()->web_contents(); + web_contents->Stop(); + owner()->RemoveInfoBar(this); + // WARNING: |this| may be deleted at this point! Do not access any members! + web_contents->OpenURL(content::OpenURLParams( + GURL(base::android::ConvertJavaStringToUTF8(env, result)), + content::Referrer(), CURRENT_TAB, content::PAGE_TRANSITION_AUTO_BOOKMARK, + false)); } void AutoLoginInfoBarDelegateAndroid::LoginFailed(JNIEnv* env, jobject obj) { - ScopedJavaLocalRef delegate = - weak_java_auto_login_delegate_.get(env); - DCHECK(delegate.obj()); - if (owner()) { - SimpleAlertInfoBarDelegate::Create( - owner(), IDR_INFOBAR_WARNING, - l10n_util::GetStringUTF16(IDS_AUTO_LOGIN_FAILED), false); - owner()->RemoveInfoBar(this); - } + if (!owner()) + return; // We're closing; don't call anything, it might access the owner. + + // TODO(miguelg): Using SimpleAlertInfoBarDelegate::Create() animates in a new + // infobar while we animate the current one closed. It would be better to use + // ReplaceInfoBar(). + SimpleAlertInfoBarDelegate::Create( + owner(), IDR_INFOBAR_WARNING, + l10n_util::GetStringUTF16(IDS_AUTO_LOGIN_FAILED), false); + owner()->RemoveInfoBar(this); } void AutoLoginInfoBarDelegateAndroid::LoginDismiss(JNIEnv* env, jobject obj) { diff --git a/chrome/browser/ui/android/infobars/confirm_infobar.cc b/chrome/browser/ui/android/infobars/confirm_infobar.cc index 702f99023bb3..59e6bffaab18 100644 --- a/chrome/browser/ui/android/infobars/confirm_infobar.cc +++ b/chrome/browser/ui/android/infobars/confirm_infobar.cc @@ -24,7 +24,6 @@ InfoBar* ConfirmInfoBarDelegate::CreateInfoBar(InfoBarService* owner) { ConfirmInfoBar::ConfirmInfoBar(InfoBarService* owner, InfoBarDelegate* delegate) : InfoBarAndroid(owner, delegate), - delegate_(delegate->AsConfirmInfoBarDelegate()), java_confirm_delegate_() { } @@ -40,12 +39,13 @@ base::android::ScopedJavaLocalRef ConfirmInfoBar::CreateRenderInfoBar( base::android::ScopedJavaLocalRef cancel_button_text = base::android::ConvertUTF16ToJavaString( env, GetTextFor(ConfirmInfoBarDelegate::BUTTON_CANCEL)); + ConfirmInfoBarDelegate* delegate = GetDelegate(); base::android::ScopedJavaLocalRef message_text = base::android::ConvertUTF16ToJavaString( - env, delegate_->GetMessageText()); + env, delegate->GetMessageText()); base::android::ScopedJavaLocalRef link_text = base::android::ConvertUTF16ToJavaString( - env, delegate_->GetLinkText()); + env, delegate->GetLinkText()); return Java_ConfirmInfoBarDelegate_showConfirmInfoBar( env, java_confirm_delegate_.obj(), reinterpret_cast(this), @@ -54,11 +54,10 @@ base::android::ScopedJavaLocalRef ConfirmInfoBar::CreateRenderInfoBar( } void ConfirmInfoBar::OnLinkClicked(JNIEnv* env, jobject obj) { - DCHECK(delegate_); if (!owner()) return; // We're closing; don't call anything, it might access the owner. - if (delegate_->LinkClicked(NEW_FOREGROUND_TAB)) + if (GetDelegate()->LinkClicked(NEW_FOREGROUND_TAB)) RemoveSelf(); } @@ -66,17 +65,24 @@ void ConfirmInfoBar::ProcessButton(int action, const std::string& action_value) { if (!owner()) return; // We're closing; don't call anything, it might access the owner. + DCHECK((action == InfoBarAndroid::ACTION_OK) || (action == InfoBarAndroid::ACTION_CANCEL)); + ConfirmInfoBarDelegate* delegate = GetDelegate(); if ((action == InfoBarAndroid::ACTION_OK) ? - delegate_->Accept() : delegate_->Cancel()) + delegate->Accept() : delegate->Cancel()) RemoveSelf(); } +ConfirmInfoBarDelegate* ConfirmInfoBar::GetDelegate() { + return delegate()->AsConfirmInfoBarDelegate(); +} + string16 ConfirmInfoBar::GetTextFor( ConfirmInfoBarDelegate::InfoBarButton button) { - return (delegate_->GetButtons() & button) ? - delegate_->GetButtonLabel(button) : string16(); + ConfirmInfoBarDelegate* delegate = GetDelegate(); + return (delegate->GetButtons() & button) ? + delegate->GetButtonLabel(button) : string16(); } diff --git a/chrome/browser/ui/android/infobars/confirm_infobar.h b/chrome/browser/ui/android/infobars/confirm_infobar.h index 7ee9559dad2d..f8e47b7d21d0 100644 --- a/chrome/browser/ui/android/infobars/confirm_infobar.h +++ b/chrome/browser/ui/android/infobars/confirm_infobar.h @@ -23,9 +23,9 @@ class ConfirmInfoBar : public InfoBarAndroid { virtual void ProcessButton(int action, const std::string& action_value) OVERRIDE; + ConfirmInfoBarDelegate* GetDelegate(); string16 GetTextFor(ConfirmInfoBarDelegate::InfoBarButton button); - ConfirmInfoBarDelegate* delegate_; base::android::ScopedJavaGlobalRef java_confirm_delegate_; DISALLOW_COPY_AND_ASSIGN(ConfirmInfoBar); diff --git a/chrome/browser/ui/android/infobars/infobar_android.cc b/chrome/browser/ui/android/infobars/infobar_android.cc index 2f08d849f3a0..f21841d1b978 100644 --- a/chrome/browser/ui/android/infobars/infobar_android.cc +++ b/chrome/browser/ui/android/infobars/infobar_android.cc @@ -29,10 +29,7 @@ const int InfoBar::kDefaultBarTargetHeight = 36; // InfoBarAndroid ------------------------------------------------------------- InfoBarAndroid::InfoBarAndroid(InfoBarService* owner, InfoBarDelegate* delegate) - : InfoBar(owner, delegate), - delegate_(delegate) { - DCHECK(delegate_); - DCHECK(delegate_->owner()); + : InfoBar(owner, delegate) { } InfoBarAndroid::~InfoBarAndroid() { @@ -60,13 +57,12 @@ void InfoBarAndroid::OnButtonClicked(JNIEnv* env, jobject obj, jint action, jstring action_value) { - DCHECK(delegate_); std::string value = base::android::ConvertJavaStringToUTF8(env, action_value); ProcessButton(action, value); } void InfoBarAndroid::OnCloseButtonClicked(JNIEnv* env, jobject obj) { - delegate_->InfoBarDismissed(); + delegate()->InfoBarDismissed(); RemoveSelf(); } @@ -78,8 +74,7 @@ void InfoBarAndroid::CloseJavaInfoBar() { } int InfoBarAndroid::GetEnumeratedIconId() { - DCHECK(delegate_); - return ResourceMapper::MapFromChromiumId(delegate_->GetIconID()); + return ResourceMapper::MapFromChromiumId(delegate()->GetIconID()); } diff --git a/chrome/browser/ui/android/infobars/infobar_android.h b/chrome/browser/ui/android/infobars/infobar_android.h index 654f480f3837..2359c51158e5 100644 --- a/chrome/browser/ui/android/infobars/infobar_android.h +++ b/chrome/browser/ui/android/infobars/infobar_android.h @@ -70,8 +70,6 @@ class InfoBarAndroid : public InfoBar { void CloseInfoBar(); private: - // The InfoBar's delegate. - InfoBarDelegate* delegate_; base::android::ScopedJavaGlobalRef java_info_bar_; DISALLOW_COPY_AND_ASSIGN(InfoBarAndroid); diff --git a/chrome/browser/ui/android/infobars/infobar_container_android.cc b/chrome/browser/ui/android/infobars/infobar_container_android.cc index be77ac8420bc..b81c1a2200b5 100644 --- a/chrome/browser/ui/android/infobars/infobar_container_android.cc +++ b/chrome/browser/ui/android/infobars/infobar_container_android.cc @@ -72,10 +72,8 @@ void InfoBarContainerAndroid::AttachJavaInfoBar(InfoBarAndroid* android_bar) { void InfoBarContainerAndroid::PlatformSpecificReplaceInfoBar( InfoBar* old_infobar, InfoBar* new_infobar) { - InfoBarAndroid* new_android_bar = static_cast(new_infobar); - InfoBarAndroid* old_android_bar = (old_infobar == NULL) ? - NULL : static_cast(old_infobar); - new_android_bar->PassJavaInfoBar(old_android_bar); + static_cast(new_infobar)->PassJavaInfoBar( + static_cast(old_infobar)); } void InfoBarContainerAndroid::PlatformSpecificRemoveInfoBar(InfoBar* infobar) { diff --git a/chrome/browser/ui/android/infobars/translate_infobar.cc b/chrome/browser/ui/android/infobars/translate_infobar.cc index 868cd3d2b4f3..f5e47b6a96a5 100644 --- a/chrome/browser/ui/android/infobars/translate_infobar.cc +++ b/chrome/browser/ui/android/infobars/translate_infobar.cc @@ -13,7 +13,7 @@ #include "ui/base/l10n/l10n_util.h" -// InfoBarDelegateAndroid ----------------------------------------------------- +// TranslateInfoBarDelegate --------------------------------------------------- // static InfoBar* TranslateInfoBarDelegate::CreateInfoBar(InfoBarService* owner) { @@ -26,7 +26,6 @@ InfoBar* TranslateInfoBarDelegate::CreateInfoBar(InfoBarService* owner) { TranslateInfoBar::TranslateInfoBar(InfoBarService* owner, TranslateInfoBarDelegate* delegate) : InfoBarAndroid(owner, delegate), - delegate_(delegate), java_translate_delegate_() { } @@ -35,38 +34,36 @@ TranslateInfoBar::~TranslateInfoBar() { ScopedJavaLocalRef TranslateInfoBar::CreateRenderInfoBar(JNIEnv* env) { java_translate_delegate_.Reset(Java_TranslateInfoBarDelegate_create(env)); - std::vector languages(delegate_->num_languages()); - for (size_t i = 0; i < delegate_->num_languages(); ++i) - languages[i] = delegate_->language_name_at(i); + TranslateInfoBarDelegate* delegate = GetDelegate(); + std::vector languages; + languages.reserve(delegate->num_languages()); + for (size_t i = 0; i < delegate->num_languages(); ++i) + languages.push_back(delegate->language_name_at(i)); base::android::ScopedJavaLocalRef java_languages = base::android::ToJavaArrayOfStrings(env, languages); return Java_TranslateInfoBarDelegate_showTranslateInfoBar( - env, - java_translate_delegate_.obj(), - reinterpret_cast(this), - delegate_->infobar_type(), - delegate_->original_language_index(), - delegate_->target_language_index(), - delegate_->ShouldAlwaysTranslate(), - ShouldDisplayNeverTranslateInfoBarOnCancel(), - java_languages.obj()); + env, java_translate_delegate_.obj(), reinterpret_cast(this), + delegate->infobar_type(), delegate->original_language_index(), + delegate->target_language_index(), delegate->ShouldAlwaysTranslate(), + ShouldDisplayNeverTranslateInfoBarOnCancel(), java_languages.obj()); } -void TranslateInfoBar::ProcessButton( - int action, const std::string& action_value) { +void TranslateInfoBar::ProcessButton(int action, + const std::string& action_value) { if (!owner()) - return; // We're closing; don't call anything, it might access the owner. + return; // We're closing; don't call anything, it might access the owner. + TranslateInfoBarDelegate* delegate = GetDelegate(); if (action == InfoBarAndroid::ACTION_TRANSLATE) { - delegate_->Translate(); + delegate->Translate(); return; } if (action == InfoBarAndroid::ACTION_CANCEL) - delegate_->TranslationDeclined(); + delegate->TranslationDeclined(); else if (action == InfoBarAndroid::ACTION_TRANSLATE_SHOW_ORIGINAL) - delegate_->RevertTranslation(); + delegate->RevertTranslation(); else DCHECK_EQ(InfoBarAndroid::ACTION_NONE, action); @@ -74,13 +71,14 @@ void TranslateInfoBar::ProcessButton( } void TranslateInfoBar::PassJavaInfoBar(InfoBarAndroid* source) { - DCHECK_NE( - delegate_->infobar_type(), TranslateInfoBarDelegate::BEFORE_TRANSLATE); + TranslateInfoBarDelegate* delegate = GetDelegate(); + DCHECK_NE(TranslateInfoBarDelegate::BEFORE_TRANSLATE, + delegate->infobar_type()); // Ask the former bar to transfer ownership to us. DCHECK(source != NULL); static_cast(source)->TransferOwnership( - this, delegate_->infobar_type()); + this, delegate->infobar_type()); } void TranslateInfoBar::ApplyTranslateOptions(JNIEnv* env, @@ -90,17 +88,18 @@ void TranslateInfoBar::ApplyTranslateOptions(JNIEnv* env, bool always_translate, bool never_translate_language, bool never_translate_site) { - delegate_->UpdateOriginalLanguageIndex(source_language_index); - delegate_->UpdateTargetLanguageIndex(target_language_index); + TranslateInfoBarDelegate* delegate = GetDelegate(); + delegate->UpdateOriginalLanguageIndex(source_language_index); + delegate->UpdateTargetLanguageIndex(target_language_index); - if (delegate_->ShouldAlwaysTranslate() != always_translate) - delegate_->ToggleAlwaysTranslate(); + if (delegate->ShouldAlwaysTranslate() != always_translate) + delegate->ToggleAlwaysTranslate(); - if (never_translate_language && delegate_->IsTranslatableLanguageByPrefs()) - delegate_->ToggleTranslatableLanguageByPrefs(); + if (never_translate_language && delegate->IsTranslatableLanguageByPrefs()) + delegate->ToggleTranslatableLanguageByPrefs(); - if (never_translate_site && !delegate_->IsSiteBlacklisted()) - delegate_->ToggleSiteBlacklist(); + if (never_translate_site && !delegate->IsSiteBlacklisted()) + delegate->ToggleSiteBlacklist(); } void TranslateInfoBar::TransferOwnership( @@ -121,10 +120,15 @@ void TranslateInfoBar::SetJavaDelegate(jobject delegate) { } bool TranslateInfoBar::ShouldDisplayNeverTranslateInfoBarOnCancel() { + TranslateInfoBarDelegate* delegate = GetDelegate(); return - (delegate_->infobar_type() == + (delegate->infobar_type() == TranslateInfoBarDelegate::BEFORE_TRANSLATE) && - (delegate_->ShouldShowNeverTranslateShortcut()); + (delegate->ShouldShowNeverTranslateShortcut()); +} + +TranslateInfoBarDelegate* TranslateInfoBar::GetDelegate() { + return delegate()->AsTranslateInfoBarDelegate(); } diff --git a/chrome/browser/ui/android/infobars/translate_infobar.h b/chrome/browser/ui/android/infobars/translate_infobar.h index 027ec28c9647..96899f0f0150 100644 --- a/chrome/browser/ui/android/infobars/translate_infobar.h +++ b/chrome/browser/ui/android/infobars/translate_infobar.h @@ -37,7 +37,8 @@ class TranslateInfoBar : public InfoBarAndroid { void SetJavaDelegate(jobject delegate); bool ShouldDisplayNeverTranslateInfoBarOnCancel(); - TranslateInfoBarDelegate* delegate_; + TranslateInfoBarDelegate* GetDelegate(); + base::android::ScopedJavaGlobalRef java_translate_delegate_; DISALLOW_COPY_AND_ASSIGN(TranslateInfoBar); -- 2.11.4.GIT