Issue 882003: Implement chrome.experimental.clipboard extension API.

Issue 882003: Implement chrome.experimental.clipboard extension API. (Closed)

Created:
10 years, 9 months ago by dcheng1

Modified:
9 years, 7 months ago

Reviewers:
Erik does not do reviews

CC:
chromium-reviews, Aaron Boodman, pam+watch_chromium.org, ben+cc_chromium.org

Base URL:
http://src.chromium.org/svn/trunk/src/

Visibility:
Public.

Description

Implement chrome.experimental.clipboard extension API. BUG=38038 TEST=browser_tests.exe --gtest_filter=ExtensionApiTest.Clipboard

Patch Set 14 : '' #

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1075 lines, -1 line)			Patch
A	chrome/browser/extensions/extension_clipboard_api.h	View	4 5 10 11	1 chunk	+38 lines, -0 lines	0 comments	Download
A	chrome/browser/extensions/extension_clipboard_api.cc	View	4 5 6 7 8 9 10 11	1 chunk	+53 lines, -0 lines	0 comments	Download
A	chrome/browser/extensions/extension_clipboard_apitest.cc	View	4 5	1 chunk	+16 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/extension_function_dispatcher.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View	4 5 6 7 8 9 10 11 12 13	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/extensions/api/extension_api.json	View	2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+34 lines, -0 lines	0 comments	Download
	chrome/common/extensions/docs/experimental.html	View	6 7 8 9 10 11 12 13	1 chunk	+1 line, -1 line	0 comments	Download
	chrome/common/extensions/docs/experimental.clipboard.html	View	6 7 8 9 10 11 12 13	1 chunk	+819 lines, -0 lines	0 comments	Download
A	chrome/common/extensions/docs/static/experimental.clipboard.html	View	6 7 8 9	1 chunk	+16 lines, -0 lines	0 comments	Download
M	chrome/renderer/resources/renderer_extension_bindings.js	View	2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+1 line, -0 lines	0 comments	Download
A	chrome/test/data/extensions/api_test/clipboard/manifest.json	View	4 5	1 chunk	+13 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/api_test/clipboard/test.html	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	chrome/test/data/extensions/api_test/clipboard/test.js	View	4 5 6 7 8 9 10 11 12 13	1 chunk	+64 lines, -0 lines	0 comments	Download
A	chrome/test/data/extensions/api_test/clipboard/test_helper.js	View		1 chunk	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Erik does not do reviews

let me know when you'd like a full review. one drive-by comment before we get ...

10 years, 9 months ago (2010-03-12 22:08:35 UTC) #1

dcheng1

I've implemented the tests and updated the patch to place the new APIs under experimental. ...

10 years, 9 months ago (2010-03-12 23:09:57 UTC) #2

Erik does not do reviews

When clipboard is enabled for an extension, I think you should enable DOM paste for ...

10 years, 9 months ago (2010-03-13 00:11:05 UTC) #3

When clipboard is enabled for an extension, I think you should enable DOM paste
for the extension.  Look in ExtensionHost.cc::GetWebkitPrefs and add
setDOMPasteAllowed(true) if the extension supports it.

Since there isn't a clipboard permission yet, just add a method on Extension
that returns true if experimental is valid.  We can change it to use the
clipboard permission later (add a TODO).

http://codereview.chromium.org/882003/diff/41001/28012
File chrome/browser/extensions/extension_clipboard_api.cc (right):

http://codereview.chromium.org/882003/diff/41001/28012#newcode17
chrome/browser/extensions/extension_clipboard_api.cc:17: NULL, NULL, &contents,
NULL)) {
you should set error_ when returning false

http://codereview.chromium.org/882003/diff/41001/28005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/882003/diff/41001/28005#newcode2430
chrome/common/extensions/api/extension_api.json:2430: "description": "Triggers a
copy operation in the specified tab.",
Is it possible that these commands could fail? (like when nothing is selected)
If so, maybe this should get a callback.

http://codereview.chromium.org/882003/diff/41001/28015
File chrome/common/extensions/docs/api_index.html (right):

http://codereview.chromium.org/882003/diff/41001/28015#newcode285
chrome/common/extensions/docs/api_index.html:285: <a
href="experimental.html">experimental APIs</a>,
Did you mean to do this?  This is supposed to explicitly point at the dev
location.

http://codereview.chromium.org/882003/diff/41001/28006
File chrome/common/extensions/docs/clipboard.html (right):

http://codereview.chromium.org/882003/diff/41001/28006#newcode1
chrome/common/extensions/docs/clipboard.html:1: <!DOCTYPE html><!-- This page is
a placeholder for generated extensions api doc. Note:
remove this file from the CL

http://codereview.chromium.org/882003/diff/41001/28014
File chrome/test/data/extensions/api_test/clipboard/test_helper.js (right):

http://codereview.chromium.org/882003/diff/41001/28014#newcode1
chrome/test/data/extensions/api_test/clipboard/test_helper.js:1:
document.addEventListener("copy", function() {
Have you tried this test out on the trybots?  They act a little different than
your desktop (they run without a desktop, as if your screen were locked), so I'm
a little concerned that the pasteboard events might not function properly.

dcheng1

If you don't mind, I'd like to implement your DOM paste idea in a separate ...

10 years, 9 months ago (2010-03-13 02:23:37 UTC) #4

If you don't mind, I'd like to implement your DOM paste idea in a separate
patch.

http://codereview.chromium.org/882003/diff/41001/28012
File chrome/browser/extensions/extension_clipboard_api.cc (right):

http://codereview.chromium.org/882003/diff/41001/28012#newcode17
chrome/browser/extensions/extension_clipboard_api.cc:17: NULL, NULL, &contents,
NULL)) {
On 2010/03/13 00:11:06, Erik Kay wrote:
> you should set error_ when returning false

Done.

http://codereview.chromium.org/882003/diff/41001/28005
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/882003/diff/41001/28005#newcode2430
chrome/common/extensions/api/extension_api.json:2430: "description": "Triggers a
copy operation in the specified tab.",
On 2010/03/13 00:11:06, Erik Kay wrote:
> Is it possible that these commands could fail? (like when nothing is selected)
> If so, maybe this should get a callback.

In the test, nothing is selected and the copy event is still dispatched. This is
intentional. There are several WebKit layout editing tests that work similarly.

http://codereview.chromium.org/882003/diff/41001/28015
File chrome/common/extensions/docs/api_index.html (right):

http://codereview.chromium.org/882003/diff/41001/28015#newcode285
chrome/common/extensions/docs/api_index.html:285: <a
href="experimental.html">experimental APIs</a>,
On 2010/03/13 00:11:06, Erik Kay wrote:
> Did you mean to do this?  This is supposed to explicitly point at the dev
> location.
> 

I intentionally changed it since it made it more annoying to navigate to the
generated experimental page. Oh well... reverted.

http://codereview.chromium.org/882003/diff/41001/28006
File chrome/common/extensions/docs/clipboard.html (right):

http://codereview.chromium.org/882003/diff/41001/28006#newcode1
chrome/common/extensions/docs/clipboard.html:1: <!DOCTYPE html><!-- This page is
a placeholder for generated extensions api doc. Note:
On 2010/03/13 00:11:06, Erik Kay wrote:
> remove this file from the CL

Done.

http://codereview.chromium.org/882003/diff/41001/28014
File chrome/test/data/extensions/api_test/clipboard/test_helper.js (right):

http://codereview.chromium.org/882003/diff/41001/28014#newcode1
chrome/test/data/extensions/api_test/clipboard/test_helper.js:1:
document.addEventListener("copy", function() {
On 2010/03/13 00:11:06, Erik Kay wrote:
> Have you tried this test out on the trybots?  They act a little different than
> your desktop (they run without a desktop, as if your screen were locked), so
I'm
> a little concerned that the pasteboard events might not function properly.

I thought only committers could use the trybots; it turns out this is not
strictly true. This patch passes on the trybots--several UI tests failed on
Windows, but I believe it's unrelated. See
http://build.chromium.org/buildbot/try-server/builders/win/builds/22767

Erik does not do reviews

after you address this last comment, LGTM I'm fine with the paste change coming in ...

10 years, 9 months ago (2010-03-13 04:05:11 UTC) #5

dcheng1

Let me know how the update looks. Thanks! http://codereview.chromium.org/882003/diff/41001/28005 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/882003/diff/41001/28005#newcode2430 chrome/common/extensions/api/extension_api.json:2430: "description": ...

10 years, 9 months ago (2010-03-15 21:36:42 UTC) #6

Erik does not do reviews

http://codereview.chromium.org/882003/diff/74001/75004 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/882003/diff/74001/75004#newcode2448 chrome/common/extensions/api/extension_api.json:2448: "name": "succeeded", ditch the parameter for this. Take a ...

10 years, 9 months ago (2010-03-15 21:43:46 UTC) #7

dcheng1

10 years, 9 months ago (2010-03-16 01:00:21 UTC) #8

Erik does not do reviews

after this last change, LGTM http://codereview.chromium.org/882003/diff/90001/91006 File chrome/test/data/extensions/api_test/clipboard/test.js (right): http://codereview.chromium.org/882003/diff/90001/91006#newcode21 chrome/test/data/extensions/api_test/clipboard/test.js:21: if (chrome.extension.lastError) { you ...

10 years, 9 months ago (2010-03-16 15:02:46 UTC) #9

dcheng1

10 years, 9 months ago (2010-03-16 20:45:38 UTC) #10

Thanks for the tip.

http://codereview.chromium.org/882003/diff/90001/91006
File chrome/test/data/extensions/api_test/clipboard/test.js (right):

http://codereview.chromium.org/882003/diff/90001/91006#newcode21
chrome/test/data/extensions/api_test/clipboard/test.js:21: if
(chrome.extension.lastError) {
On 2010/03/16 15:02:46, Erik Kay wrote:
> you can leave this out.  chrome.test.callbackPass does this for you.  You
don't
> even need to give it any arguments:
> 
> chrome.test.callbackPass()

Done.

Expand Messages | Collapse Messages