Separate ContentSettingsClient out from LocalFrameClient

third_party/WebKit/Source/web/WebLocalFrameImpl.h

Index: third_party/WebKit/Source/web/WebLocalFrameImpl.h
diff --git a/third_party/WebKit/Source/web/WebLocalFrameImpl.h b/third_party/WebKit/Source/web/WebLocalFrameImpl.h
index a57a15a315a8cac17f25fae0101c0bcc828b72de..fcb4c2c5e1aeb002b14b71a536a7bf2bb38f9aec 100644
--- a/third_party/WebKit/Source/web/WebLocalFrameImpl.h
+++ b/third_party/WebKit/Source/web/WebLocalFrameImpl.h
@@ -32,6 +32,7 @@
 #define WebLocalFrameImpl_h
 
 #include "core/editing/VisiblePosition.h"
+#include "core/frame/ContentSettingsClient.h"
 #include "core/frame/LocalFrame.h"
 #include "platform/geometry/FloatRect.h"
 #include "platform/heap/SelfKeepAlive.h"
@@ -91,7 +92,6 @@ class WEB_EXPORT WebLocalFrameImpl final
   WebString assignedName() const override;
   void setName(const WebString&) override;
   WebVector<WebIconURL> iconURLs(int iconTypesMask) const override;
-  void setContentSettingsClient(WebContentSettingsClient*) override;
   void setSharedWorkerRepositoryClient(
       WebSharedWorkerRepositoryClient*) override;
   WebSize getScrollOffset() const override;
@@ -182,6 +182,7 @@ class WEB_EXPORT WebLocalFrameImpl final
   bool isSpellCheckingEnabled() const override;
   void replaceMisspelledRange(const WebString&) override;
   void removeSpellingMarkers() override;
+  void setContentSettingsClient(WebContentSettingsClient*) override;
   bool hasSelection() const override;
   WebRange selectionRange() const override;
   WebString selectionAsText() const override;
@@ -389,9 +390,10 @@ class WEB_EXPORT WebLocalFrameImpl final
   WebFrameClient* client() const { return m_client; }
   void setClient(WebFrameClient* client) { m_client = client; }
 
-  WebContentSettingsClient* contentSettingsClient() {
+  ContentSettingsClient& contentSettingsClient() {
     return m_contentSettingsClient;
   }
+
   SharedWorkerRepositoryClientImpl* sharedWorkerRepositoryClient() const {
     return m_sharedWorkerRepositoryClient.get();
   }
@@ -471,7 +473,7 @@ class WEB_EXPORT WebLocalFrameImpl final
 
   WebFrameClient* m_client;
   WebAutofillClient* m_autofillClient;
-  WebContentSettingsClient* m_contentSettingsClient;
+  ContentSettingsClient m_contentSettingsClient;

[haraken, 2017/04/04 11:25:32]

I'm wondering if WebLocalFrameImpl is a right object to own
ContentSettingsClient. Would you help me understand why you chose
WebLocalFrameImpl?

In the first place, do we really need ContentSettingsClient? Now that we have
WebContentSettingsClient on platform/, can we probably directly use
WebContentSettingsClient to configure content settings?

[kinuko, 2017/04/04 14:50:57]

Yep- I don't think this would be the best place either.  I tried several
alternatives but this looked least disruptive yet somewhat acceptable as an
incremental step.

One of the complications is that WebLocalFrameImpl's initialization order and
LocalFrame creation time is not consistent in different cases, so we anyway need
to keep something in this class to make things always work.  I assume all those
would need to be incrementally cleaned up.
(Fwiw I had wondered the same.)  What ContentSettingsClient does (and what
LocalFrameClient was doing in the aspect) is to return the given default value
if embedder doesn't install its own client, so it basically hides embedder's
implementation details (btw here embedder means chrome etc, not content).   We
can try removing the wrapping part too, but it'd make this change even bigger &
the callsite's code a little more complex.  I want to avoid making the change at
least in this CL.

Separately from that I could make ContentSettingsClient a subclass of
WebContentSettingsClient and name it WrappedWebContentSettingsClient or
something like that to make it a little clearer what it does, if it helps.

[haraken, 2017/04/05 02:31:07]

Yeah, I want to think about cleaning up these things in follow-up CLs.

   std::unique_ptr<SharedWorkerRepositoryClientImpl>
       m_sharedWorkerRepositoryClient;