Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 11644059: Change infobar creation to use a public static Create() method on the infobar delegate classes. (Closed)

Created:
8 years ago by Peter Kasting
Modified:
7 years, 11 months ago
Reviewers:
Elliot Glaysher, sky
CC:
chromium-reviews, nkostylev+watch_chromium.org, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, rdsmith+dwatch_chromium.org, stuartmorgan+watch_chromium.org, dmazzoni+watch_chromium.org, Albert Bodenhamer, markusheintz_, benjhayden+dwatch_chromium.org, Ilya Sherman, benquan, aboxhall+watch_chromium.org, stevenjb+watch_chromium.org, jam, dbeam+watch-autofill_chromium.org, darin-cc_chromium.org, rouslan+watch_chromium.org, Raghu Simha, gbillock+watch_chromium.org, chromium-apps-reviews_chromium.org, tim (not reviewing), groby+watch_chromium.org, dtseng+watch_chromium.org, yoshiki+watch_chromium.org, yuzo+watch_chromium.org, oshima+watch_chromium.org, smckay+watch_chromium.org, Raman Kakilate, haitaol1, ctguil+watch_chromium.org, zork+watch_chromium.org, akalin, hashimoto+watch_chromium.org, tfarina, davidbarr+watch_chromium.org, Aaron Boodman, Dane Wallinga, dyu1, estade+watch_chromium.org, James Su, davemoore+watch_chromium.org
Visibility:
Public.

Description

Change infobar creation to use a public static Create() method on the infobar delegate classes. Make constructors as private as possible. This has several purposes: * By preventing direct instantiation, it prevents callers from leaking if they create an infobar and don't add it to an InfoBarService. * By moving decision-making about when to show infobars into these Create() functions, there's a pattern for where such code should go, and caller code becomes simpler and easier to read. * The two bullets above together mean that for infobars which should only be displayed in certain circumstances, code can't accidentally bypass the decision logic. * It enables us to eliminate a common InfoBarService* temp on the caller side since the caller no longer needs to both pass the pointer to the infobar _and_ call AddInfoBar() on the pointer. This was also a somewhat redundant-looking pattern. * It makes it easier to change the ownership model for infobars in the future by limiting the affected callsites to only the Create() functions. Note that right now, this still feels pretty redundant since we pass all the same args to Create() functions as constructors most times. In the new ownership model constructors will no longer need to take InfoBarService*s, which will make this better. Additionally, this makes AddInfoBar()/ReplaceInfoBar() take scoped_ptr<>s to indicate they're receiving ownership. This sort of change is easy to make since we only need change the create functions. This change also has a functional effect: it eliminates some cases where we tried to only show infobars when no other infobars were already showing (discussed and approved by Glen). BUG=none TEST=none Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175467

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1495 lines, -1392 lines) Patch
M chrome/browser/accessibility/accessibility_extension_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/browser/alternate_nav_url_fetcher.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/browser/api/infobars/confirm_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/api/infobars/confirm_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/api/infobars/infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/api/infobars/infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/api/infobars/infobar_service.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +9 lines, -7 lines 0 comments Download
M chrome/browser/api/infobars/simple_alert_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -3 lines 0 comments Download
M chrome/browser/api/infobars/simple_alert_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/autofill/autofill_cc_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +17 lines, -2 lines 0 comments Download
M chrome/browser/autofill/autofill_cc_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/autofill/autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -6 lines 0 comments Download
M chrome/browser/autofill/autofill_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +20 lines, -18 lines 0 comments Download
M chrome/browser/chrome_quota_permission_context.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +26 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/webui_login_view.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -7 lines 0 comments Download
M chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +10 lines, -5 lines 0 comments Download
M chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +31 lines, -10 lines 0 comments Download
M chrome/browser/download/download_request_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +16 lines, -4 lines 0 comments Download
M chrome/browser/download/download_request_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +13 lines, -5 lines 0 comments Download
M chrome/browser/download/download_request_infobar_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +10 lines, -19 lines 0 comments Download
M chrome/browser/download/download_request_limiter.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/extensions/api/debugger/debugger_api.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +28 lines, -12 lines 0 comments Download
M chrome/browser/extensions/app_notify_channel_ui_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +30 lines, -18 lines 0 comments Download
M chrome/browser/extensions/extension_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +13 lines, -6 lines 0 comments Download
M chrome/browser/extensions/extension_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +17 lines, -5 lines 0 comments Download
M chrome/browser/extensions/extension_install_ui_default.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -15 lines 0 comments Download
M chrome/browser/extensions/extension_install_ui_default.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +18 lines, -70 lines 0 comments Download
M chrome/browser/extensions/theme_installed_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/extensions/theme_installed_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +75 lines, -22 lines 0 comments Download
M chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 11 chunks +13 lines, -13 lines 0 comments Download
M chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +19 lines, -0 lines 0 comments Download
D chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -32 lines 0 comments Download
D chrome/browser/geolocation/geolocation_confirm_infobar_delegate_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -26 lines 0 comments Download
M chrome/browser/geolocation/geolocation_infobar_queue_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +4 lines, -8 lines 0 comments Download
M chrome/browser/google/google_url_tracker.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -19 lines 0 comments Download
M chrome/browser/google/google_url_tracker_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -3 lines 0 comments Download
M chrome/browser/google/google_url_tracker_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +17 lines, -6 lines 0 comments Download
M chrome/browser/google/google_url_tracker_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +29 lines, -8 lines 0 comments Download
M chrome/browser/infobars/alternate_nav_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -3 lines 0 comments Download
M chrome/browser/infobars/alternate_nav_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/infobars/infobar.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/infobars/infobar_extension_api.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -5 lines 0 comments Download
M chrome/browser/infobars/infobar_tab_helper.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/infobars/infobar_tab_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +24 lines, -40 lines 0 comments Download
M chrome/browser/infobars/insecure_content_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -5 lines 0 comments Download
M chrome/browser/infobars/insecure_content_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +26 lines, -3 lines 0 comments Download
D chrome/browser/intents/register_intent_handler_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -42 lines 0 comments Download
M chrome/browser/intents/register_intent_handler_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +35 lines, -15 lines 0 comments Download
M chrome/browser/intents/register_intent_handler_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +48 lines, -37 lines 0 comments Download
M chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -7 lines 0 comments Download
M chrome/browser/managed_mode/managed_mode_navigation_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +13 lines, -6 lines 0 comments Download
M chrome/browser/nacl_host/nacl_infobar.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/notifications/desktop_notification_service.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +28 lines, -4 lines 0 comments Download
M chrome/browser/omnibox_search_hint.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -9 lines 0 comments Download
M chrome/browser/omnibox_search_hint.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +51 lines, -38 lines 0 comments Download
M chrome/browser/password_manager/password_manager_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +33 lines, -22 lines 0 comments Download
M chrome/browser/pepper_broker_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -7 lines 0 comments Download
M chrome/browser/pepper_broker_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +23 lines, -23 lines 0 comments Download
M chrome/browser/plugins/plugin_infobar_delegates.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +31 lines, -14 lines 0 comments Download
M chrome/browser/plugins/plugin_infobar_delegates.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +82 lines, -40 lines 0 comments Download
M chrome/browser/plugins/plugin_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +24 lines, -55 lines 0 comments Download
M chrome/browser/ssl/ssl_tab_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +64 lines, -53 lines 0 comments Download
M chrome/browser/three_d_api_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +16 lines, -4 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +21 lines, -21 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +42 lines, -39 lines 0 comments Download
M chrome/browser/translate/translate_manager.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +0 lines, -12 lines 0 comments Download
M chrome/browser/translate/translate_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +29 lines, -90 lines 0 comments Download
M chrome/browser/ui/auto_login_info_bar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/auto_login_info_bar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +23 lines, -16 lines 0 comments Download
M chrome/browser/ui/auto_login_prompter.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/browser.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -14 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +11 lines, -70 lines 0 comments Download
M chrome/browser/ui/chrome_select_file_policy.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -8 lines 0 comments Download
M chrome/browser/ui/cocoa/content_settings/collected_cookies_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/infobars/infobar_container_controller.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/infobars/infobar_controller.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/keystone_infobar_delegate.mm View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +24 lines, -20 lines 0 comments Download
M chrome/browser/ui/collected_cookies_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/ui/collected_cookies_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/content_settings/content_setting_bubble_model.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/gtk/collected_cookies_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/hung_plugin_tab_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +26 lines, -8 lines 0 comments Download
M chrome/browser/ui/media_stream_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -5 lines 0 comments Download
M chrome/browser/ui/media_stream_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +42 lines, -9 lines 0 comments Download
M chrome/browser/ui/startup/autolaunch_prompt_win.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +17 lines, -11 lines 0 comments Download
M chrome/browser/ui/startup/bad_flags_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -9 lines 0 comments Download
M chrome/browser/ui/startup/default_browser_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +18 lines, -12 lines 0 comments Download
M chrome/browser/ui/startup/obsolete_os_info_bar.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/ui/startup/obsolete_os_info_bar.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +39 lines, -8 lines 0 comments Download
D chrome/browser/ui/startup/obsolete_os_prompt.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -21 lines 0 comments Download
D chrome/browser/ui/startup/obsolete_os_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -15 lines 0 comments Download
D chrome/browser/ui/startup/obsolete_os_prompt_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -41 lines 0 comments Download
M chrome/browser/ui/startup/session_crashed_prompt.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +34 lines, -4 lines 0 comments Download
M chrome/browser/ui/startup/session_crashed_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +16 lines, -58 lines 0 comments Download
M chrome/browser/ui/startup/startup_browser_creator_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +23 lines, -9 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/sync/one_click_signin_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/collected_cookies_views.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/external_tab_container_win.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/ui/website_settings/website_settings_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +0 lines, -3 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +2 lines, -10 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Peter Kasting
7 years, 12 months ago (2012-12-26 20:18:57 UTC) #1
Peter Kasting
Patchset 9 also includes a change to remove InfoBarDelegate::InfoBarClosed() and instead have callers just delete ...
7 years, 12 months ago (2012-12-28 02:36:19 UTC) #2
Elliot Glaysher
my eyes have rolled over this entire change. nothing jumped out as wrong. lgtm.
7 years, 11 months ago (2013-01-02 20:23:25 UTC) #3
Peter Kasting
Nico, any chance you can rubber-stamp this as a chrome/ OWNER? I'd send this to ...
7 years, 11 months ago (2013-01-02 21:59:39 UTC) #4
Nico
On 2013/01/02 21:59:39, Peter Kasting wrote: > Nico, any chance you can rubber-stamp this as ...
7 years, 11 months ago (2013-01-02 23:24:19 UTC) #5
Peter Kasting
sky: chrome/ OWNERS
7 years, 11 months ago (2013-01-03 19:17:16 UTC) #6
sky
7 years, 11 months ago (2013-01-07 17:17:22 UTC) #7
LGTM

Powered by Google App Engine
This is Rietveld 408576698