Bug 59264 – [chromium] Implement EditorClient::canCopyCut and EditorClient::canPaste

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

This allows the browser to decide if copy/cut/paste operations should be allowed on a per-origin basis, e.g. for extensions.

[Daniel Cheng]

Created attachment 90822 [details]
Patch

[Daniel Cheng]

For reference, the relevant Chrome patch can be found at http://codereview.chromium.org/6480106/

[Darin Fisher (:fishd, Google)]

Comment on attachment 90822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90822&action=review

> Source/WebKit/chromium/public/WebViewClient.h:318
> +    virtual bool canTriggerClipboardRead(const WebURL& url) { return false; }

nit: webkit style is to drop the parameter name when the name is just a clone of the type name.

nit: these functions are listed in the "Zoom" section, but they don't seem to have anything
to do with zooming.  it looks like registerProtocolHandler was improperly placed here too!

[Daniel Cheng]

Created attachment 91024 [details]
Patch

[Daniel Cheng]

(In reply to comment #3)
> (From update of attachment 90822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90822&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:318
> > +    virtual bool canTriggerClipboardRead(const WebURL& url) { return false; }
> 
> nit: webkit style is to drop the parameter name when the name is just a clone of the type name.
> 

Done.

> nit: these functions are listed in the "Zoom" section, but they don't seem to have anything
> to do with zooming.  it looks like registerProtocolHandler was improperly placed here too!

I've picked (what I hope is) a more appropriate location.

[Eric Seidel (no email)]

Comment on attachment 91024 [details]
Patch

Since Darin sounded generally OK with this, this seems OK to me too.

[WebKit Commit Bot]

Comment on attachment 91024 [details]
Patch

Clearing flags on attachment: 91024

Committed r84967: <http://trac.webkit.org/changeset/84967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84967 might have broken SnowLeopard Intel Release (WebKit2 Tests)

[John Abd-El-Malek]

Darin: the chrome side of this change, http://codereview.chromium.org/6480106/, is related to extensions.  Since we'll want to have the IPCs be in chrome, not content layer, adding these APIs to WebViewClient means that we'll have to implement these virtual functions on RenderView, only to call out to the embedder using ContentRendererClient, and then implement this for real in ChromeContentRendererClient.

For autofill/spellcheck/devtools, we've created specific interfaces that the webkit layer calls, and implement those in the chrome layer directly.  Seems we should do the same here to avoid all the indirection.  sounds good?  if so, what name do you recommend?  WebClipboardPermissionClient?  (I suck with naming :) ).  thanks, and apologies to Daniel for all the run-around.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> Darin: the chrome side of this change, http://codereview.chromium.org/6480106/, is related to extensions.  Since we'll want to have the IPCs be in chrome, not content layer, adding these APIs to WebViewClient means that we'll have to implement these virtual functions on RenderView, only to call out to the embedder using ContentRendererClient, and then implement this for real in ChromeContentRendererClient.
> 
> For autofill/spellcheck/devtools, we've created specific interfaces that the webkit layer calls, and implement those in the chrome layer directly.  Seems we should do the same here to avoid all the indirection.  sounds good?  if so, what name do you recommend?  WebClipboardPermissionClient?  (I suck with naming :) ).  thanks, and apologies to Daniel for all the run-around.

Ah, good point.  A separate WebClipboard{Permission,Policy}Client interface SGTM.  We could maybe just call it WebClipboardClient.

  class WebClipboardClient {
  public:
      virtual bool canReadFromClipboard(WebFrame*) = 0;
      virtual bool canWriteToClipboard(WebFrame*) = 0;
  };

We could just have the methods be named canRead and canWrite, but that wouldn't scale well if we ever had a number of other read/write policy related client interfaces, and someone wanted to implement more than one of them.

Note: I think it would be better to pass the WebFrame instead of the URL, so that the caller can have access to more context if needed.

[John Abd-El-Malek]

Darin: it looks like it would be nice to move the following as well out of RenderView.  How about a more generic Web{Permission,Policy}Client?  If this sounds good, I can take care of doing this, since it would help with some content refactoring I'd like to do in the browser process.


  virtual bool allowImages(WebKit::WebFrame* frame, bool enabled_per_settings);
  virtual bool allowPlugins(WebKit::WebFrame* frame, bool enabled_per_settings);
  virtual bool allowScript(WebKit::WebFrame* frame, bool enabled_per_settings);
  virtual bool allowDatabase(WebKit::WebFrame* frame,
                             const WebKit::WebString& name,
                             const WebKit::WebString& display_name,
                             unsigned long estimated_size);
  virtual void didNotAllowScript(WebKit::WebFrame* frame);
  virtual void didNotAllowPlugins(WebKit::WebFrame* frame);
  virtual bool allowScriptExtension(WebKit::WebFrame*,
                                    const WebKit::WebString& extension_name,
                                    int extensionGroup);

[John Abd-El-Malek]

(In reply to comment #12)
> Darin: it looks like it would be nice to move the following as well out of RenderView.  How about a more generic Web{Permission,Policy}Client?  If this sounds good, I can take care of doing this, since it would help with some content refactoring I'd like to do in the browser process.

just to be clear, this isn't just for cleaning up the browser process (although it will).  most of the functions above go to ContentRendererClient, so it'd be good to get them to go directly to the class that deals with them.  I had added them to ContentRendererClient so that we can get rid of the remaining dependencies and turn on DEPS checking

[John Abd-El-Malek]

for anyone reading, this was committed in r85241.