[subresource_filter] Move OnReloadRequested to the ChromeClient

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/content_settings/content_setting_bubble_model.h"
 
 #include <stddef.h>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
@@ skipping 29 matching lines @@
 #include "chrome/common/render_messages.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/public/rappor_utils.h"
 #include "components/rappor/rappor_service_impl.h"
 #include "components/strings/grit/components_strings.h"
-#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 #include "components/url_formatter/elide_url.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/origin_util.h"
 #include "ppapi/features/features.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
@@ skipping 1199 matching lines @@
     bool is_checked) {
   if (is_checked)
     set_done_button_text(l10n_util::GetStringUTF16(IDS_APP_MENU_RELOAD));
   else
     set_done_button_text(base::string16());
   is_checked_ = is_checked;
 }
 
 void ContentSettingSubresourceFilterBubbleModel::OnDoneClicked() {
   if (is_checked_) {
-    auto* driver_factory = subresource_filter::
-        ContentSubresourceFilterDriverFactory::FromWebContents(web_contents());
-    driver_factory->OnReloadRequested();
+    ChromeSubresourceFilterClient::FromWebContents(web_contents())

[Bryan McQuade, 2017/05/19 13:24:21]

same

[Charlie Harrison, 2017/05/19 13:35:00]

Actually I'm less sure about this one, it's hard for me to track the lifetimes
here since all the pointers are raw ptrs.

pkasting, do you know if the bubble model is scoped to a WebContents lifetime?

[Peter Kasting, 2017/05/19 17:18:13]

I think you meant to ask about the filter client?  The bubble model is |this|
here and isn't really as connected to a WebContents.

I don't know about the filter client, but this patch isn't changing behavior, so
I'm OK with this code.

[Charlie Harrison, 2017/05/19 17:25:50]

The filter client is scoped to a WebContents, and is created with standard tab
helpers.
I'm worried about the following:
1. The bubble is shown
2. The underlying WebContents is destroyed (along with the filter client)
3. OnDoneClicked is run.

I looked through this class and it looks like web_contents_ can be reset to
nullptr here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/content_settings/conte...

So I think it might be possible for the filter client to be null here and
require a null check.

[Peter Kasting, 2017/05/19 17:31:52]

That listener is really confusing.  I'd look at who added it when and why.  It
doesn't seem offhand like it ought to be needed.

I would expect anything that destroys a WebContents to happen during tab
closure, which should also close open bubbles and the like.  In a UI sense it's
not reasonable to imagine an open bubble for no underlying page.

[Charlie Harrison, 2017/05/19 17:39:06]

Thanks for the clear explanation, that aligns with my expectations.
I'll go ahead and land this CL and follow up with some code archaeology to
figure out if we can remove that.

+        ->OnReloadRequested();
   }
 }
 
 ContentSettingSubresourceFilterBubbleModel*
 ContentSettingSubresourceFilterBubbleModel::AsSubresourceFilterBubbleModel() {
   return this;
 }
 
 // ContentSettingMidiSysExBubbleModel ------------------------------------------
 
@@ skipping 315 matching lines @@
 
 ContentSettingSubresourceFilterBubbleModel*
 ContentSettingBubbleModel::AsSubresourceFilterBubbleModel() {
   return nullptr;
 }
 
 ContentSettingDownloadsBubbleModel*
 ContentSettingBubbleModel::AsDownloadsBubbleModel() {
   return nullptr;
 }