Separate ContentSettingsClient out from LocalFrameClient

third_party/WebKit/Source/web/WebLocalFrameImpl.h

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 14 matching lines @@
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef WebLocalFrameImpl_h
 #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"
 #include "public/platform/WebFileSystemType.h"
 #include "public/web/WebLocalFrame.h"
 #include "web/LocalFrameClientImpl.h"
 #include "web/UserMediaClientImpl.h"
 #include "web/WebExport.h"
 #include "web/WebFrameImplBase.h"
 #include "web/WebFrameWidgetBase.h"
@@ skipping 39 matching lines @@
     : public WebFrameImplBase,
       NON_EXPORTED_BASE(public WebLocalFrame) {
  public:
   // WebFrame methods:
   // TODO(dcheng): Fix sorting here; a number of method have been moved to
   // WebLocalFrame but not correctly updated here.
   void close() override;
   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;
   void setScrollOffset(const WebSize&) override;
   WebSize contentsSize() const override;
   bool hasVisibleContent() const override;
   WebRect visibleContentRect() const override;
   bool hasHorizontalScrollbar() const override;
   bool hasVerticalScrollbar() const override;
   WebView* view() const override;
@@ skipping 70 matching lines @@
                                   unsigned length,
                                   WebRect&) const override;
   size_t characterIndexForPoint(const WebPoint&) const override;
   bool executeCommand(const WebString&) override;
   bool executeCommand(const WebString&, const WebString& value) override;
   bool isCommandEnabled(const WebString&) const override;
   void enableSpellChecking(bool) override;
   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;
   WebString selectionAsMarkup() const override;
   bool selectWordAroundCaret() override;
   void selectRange(const WebPoint& base, const WebPoint& extent) override;
   void selectRange(const WebRange&) override;
   WebString rangeAsText(const WebRange&) override;
   void moveRangeSelectionExtent(const WebPoint&) override;
   void moveRangeSelection(
@@ skipping 187 matching lines @@
   void didFinish();
 
   // Sets whether the WebLocalFrameImpl allows its document to be scrolled.
   // If the parameter is true, allow the document to be scrolled.
   // Otherwise, disallow scrolling.
   void setCanHaveScrollbars(bool) override;
 
   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();
   }
 
   void setInputEventsTransformForEmulation(const IntSize&, float);
 
   static void selectWordAroundPosition(LocalFrame*, VisiblePosition);
 
   TextFinder* textFinder() const;
   // Returns the text finder object if it already exists.
@@ skipping 59 matching lines @@
   Member<LocalFrame> m_frame;
 
   Member<WebDevToolsAgentImpl> m_devToolsAgent;
 
   // This is set if the frame is the root of a local frame tree, and requires a
   // widget for layout.
   WebFrameWidgetBase* m_frameWidget;
 
   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;
 
   // Will be initialized after first call to ensureTextFinder().
   Member<TextFinder> m_textFinder;
 
   // Valid between calls to BeginPrint() and EndPrint(). Containts the print
   // information. Is used by PrintPage().
   Member<ChromePrintContext> m_printContext;
 
@@ skipping 20 matching lines @@
 
 DEFINE_TYPE_CASTS(WebLocalFrameImpl,
                   WebFrame,
                   frame,
                   frame->isWebLocalFrame(),
                   frame.isWebLocalFrame());
 
 }  // namespace blink
 
 #endif