Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379573008:
Add SetImageData api to chrome.clipboard. (Closed)
DescriptionAdd SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard.
BUG=678794
TBR=isherman
Committed: https://crrev.com/6298fc54177c7f2919fc158a3b43e7a3e08f36fe
Cr-Commit-Position: refs/heads/master@{#441546}
Patch Set 1 #Patch Set 2 : Add a string for prompting clipboard write permission for installing chrome.clipboard. #
Total comments: 8
Patch Set 3 : Change ImageType to enum data type in SetImageData API. #Patch Set 4 : Add clipboardReadWrite permission. #Patch Set 5 : Fix compiler error. #Patch Set 6 : Fix interactive_ui_tests issue. #
Total comments: 46
Patch Set 7 : Address code review comments and add test cases. #
Total comments: 45
Patch Set 8 : More changes for addressing code review comments. #Patch Set 9 : Rebase. #Patch Set 10 : Fix compiler issue on non-chromeos platforms. #Patch Set 11 : Fix compiler errors. #Patch Set 12 : Add a warning in clipboard.idl about the future deprecation plan. #
Total comments: 27
Patch Set 13 : More improvements from code review comments. #Patch Set 14 : Rebase. #Patch Set 15 : Defer creating clipboard_extension_helper_ object until used. #Patch Set 16 : Remove previously added image files. #Patch Set 17 : Update idl for possible web alternative timeline. #
Total comments: 12
Patch Set 18 : Remvoe clipboardReadWrite permission, etc. #Patch Set 19 : Adjust clipboard api permissions to method level, etc. #
Total comments: 10
Patch Set 20 : Change permission strings and fix nits. #
Total comments: 4
Patch Set 21 : Fix nits. #Messages
Total messages: 71 (14 generated)
Description was changed from ========== Add SetImageData api to chrome.clipboard. BUG=150835 patch from issue 2347363002 at patchset 160001 (http://crrev.com/2347363002#ps160001) ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 patch from issue 2347363002 at patchset 160001 (http://crrev.com/2347363002#ps160001) ==========
jennyz@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, This cl implements an extension API setImageData in chrome.clipboard, that can accepts png or jpeg encoded image data in ArrayBuffer. It will decode the image data using the existing ImageDecoder and save the decoded bitmap data in chromeos clipboard data. I tested it with my testing js code, it works well. After executing the setImageData API, I can paste the image data. The existing ImageDecoder code will decode the encoded image data in a separate utility process, so I assume this should guard the malicious data possibly passed in from the ArrayBuffer. Please take a look at the cl and let me know if you have any concerns. Once it looks fine to you, I will proceed to add some testing cases.
Description was changed from ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 patch from issue 2347363002 at patchset 160001 (http://crrev.com/2347363002#ps160001) ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 ==========
Also, I am not sure how widely we want to expose this, since it would be good to remove this API once the new Clipboard API is implemented. +rdevlin.cronin for insight on this (and the IDL questions) https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... chrome/browser/extensions/clipboard_extension_helper.cc:41: ImageDecoder::StartWithOptions(this, image_data_str, codec, true); IIRC, ImageDecoder internally uses std::vector<uint8_t>. Is there a way for extensions bindings to give us that type, so we can avoid copying the data from std::vector<char> -> std::string -> std::vector<uint8_t>? https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_rules.cc (right): https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_rules.cc:575: {APIPermission::kClipboardWrite}, In the past, we intentionally didn't add a prompt for this, to avoid warning fatigue. https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:64: ExtensionsBrowserClient::Get()->SaveImageDataToClipboard( I might be missing something, but can the ExtensionFunction just use the helper directly instead of going through ContentBrowserClient? From a layering perspective, it would be nice not to have to expose extension-specific functionality there. https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... extensions/common/api/clipboard.idl:26: DOMString type, I'm not an extension IDL expert, but if we can somehow use an enum here, that would be nicer than a string.
Did this ever go through an API review?
On 2016/10/04 16:26:33, Devlin wrote: > Did this ever go through an API review? What kind API review process do we need to go through?
On 2016/10/04 05:13:28, dcheng wrote: > Also, I am not sure how widely we want to expose this, since it would be good to > remove this API once the new Clipboard API is implemented. +rdevlin.cronin for > insight on this (and the IDL questions) > > https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/clipboard_extension_helper.cc (right): > > https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/clipboard_extension_helper.cc:41: > ImageDecoder::StartWithOptions(this, image_data_str, codec, true); > IIRC, ImageDecoder internally uses std::vector<uint8_t>. Is there a way for > extensions bindings to give us that type, so we can avoid copying the data from > std::vector<char> -> std::string -> std::vector<uint8_t>? > > https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... > File chrome/common/extensions/permissions/chrome_permission_message_rules.cc > (right): > > https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... > chrome/common/extensions/permissions/chrome_permission_message_rules.cc:575: > {APIPermission::kClipboardWrite}, > In the past, we intentionally didn't add a prompt for this, to avoid warning > fatigue. > > https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... > File extensions/browser/api/clipboard/clipboard_api.cc (right): > > https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... > extensions/browser/api/clipboard/clipboard_api.cc:64: > ExtensionsBrowserClient::Get()->SaveImageDataToClipboard( > I might be missing something, but can the ExtensionFunction just use the helper > directly instead of going through ContentBrowserClient? From a layering > perspective, it would be nice not to have to expose extension-specific > functionality there. > > https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... > File extensions/common/api/clipboard.idl (right): > > https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... > extensions/common/api/clipboard.idl:26: DOMString type, > I'm not an extension IDL expert, but if we can somehow use an enum here, that > would be nicer than a string. chrome.clipboard APIs are only exposed on chromeos platform.
https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/20001/chrome/browser/extensio... chrome/browser/extensions/clipboard_extension_helper.cc:41: ImageDecoder::StartWithOptions(this, image_data_str, codec, true); On 2016/10/04 05:13:27, dcheng wrote: > IIRC, ImageDecoder internally uses std::vector<uint8_t>. Is there a way for > extensions bindings to give us that type, so we can avoid copying the data from > std::vector<char> -> std::string -> std::vector<uint8_t>? The automatically generated clipboard.h maps the ArrayBuffer data type from extension API to std::vector<char>. I may not have the freedom to change the extensions bindings to give use std::vector<uint8_t> type. namespace SetImageData { struct Params { static std::unique_ptr<Params> Create(const base::ListValue& args); ~Params(); // The encoded image data. std::vector<char> image_data; ... } ... https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... File chrome/common/extensions/permissions/chrome_permission_message_rules.cc (right): https://codereview.chromium.org/2379573008/diff/20001/chrome/common/extension... chrome/common/extensions/permissions/chrome_permission_message_rules.cc:575: {APIPermission::kClipboardWrite}, On 2016/10/04 05:13:27, dcheng wrote: > In the past, we intentionally didn't add a prompt for this, to avoid warning > fatigue. But is it OK not to warn user about the clipboard write? The current kClipboardRead warning prompt only warns about clipboard read, "Read data you copy and paste". Or should we make a new one that warn user about read/write clipboard in one message instead of, such as "Read and write data you copy and paste"? https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/20001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:64: ExtensionsBrowserClient::Get()->SaveImageDataToClipboard( On 2016/10/04 05:13:27, dcheng wrote: > I might be missing something, but can the ExtensionFunction just use the helper > directly instead of going through ContentBrowserClient? From a layering > perspective, it would be nice not to have to expose extension-specific > functionality there. The image decoding code we used is under chrome/browser, which extensions/browser/api code can not depends on, so we have to access it via ExtensionsBrowserClient interface. https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/20001/extensions/common/api/c... extensions/common/api/clipboard.idl:26: DOMString type, On 2016/10/04 05:13:27, dcheng wrote: > I'm not an extension IDL expert, but if we can somehow use an enum here, that > would be nicer than a string. Done.
On 2016/10/04 16:26:33, Devlin wrote: > Did this ever go through an API review? This cl adds a new function to the existing chrome.clipboard extension API chrome.clipboard, which is a platform app only available on chromeos. It was released in R54 dev channel. https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... I checked the document for creating new extension API. I think I have followed the steps in https://www.chromium.org/developers/design-documents/extensions/proposed-chan... There are some steps I haven't done in this cl like adding test and document. I would like the reviewer to have a quick review first, if there is no major issues, I would then proceed to add the test cases and document to complete the cl. Would you please take a look at the cl and let me know if you have any concerns? Thanks,
On 2016/10/11 17:53:48, jennyz wrote: > On 2016/10/04 16:26:33, Devlin wrote: > > Did this ever go through an API review? > > This cl adds a new function to the existing chrome.clipboard extension API > chrome.clipboard, which is a platform app only available on chromeos. It was > released in R54 dev channel. > https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... > > I checked the document for creating new extension API. I think I have followed > the steps in > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > There are some steps I haven't done in this cl like adding test and document. I > would like the reviewer to have a quick review first, if there is no major > issues, I would then proceed to add the test cases and document to complete the > cl. > > Would you please take a look at the cl and let me know if you have any concerns? > > Thanks, The first step in the process is the proposal: https://www.chromium.org/developers/design-documents/extensions/proposed-chan... Did one for this ever go out?
On 2016/10/12 21:14:15, Devlin wrote: > On 2016/10/11 17:53:48, jennyz wrote: > > On 2016/10/04 16:26:33, Devlin wrote: > > > Did this ever go through an API review? > > > > This cl adds a new function to the existing chrome.clipboard extension API > > chrome.clipboard, which is a platform app only available on chromeos. It was > > released in R54 dev channel. > > > https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... > > > > I checked the document for creating new extension API. I think I have followed > > the steps in > > > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > > > There are some steps I haven't done in this cl like adding test and document. > I > > would like the reviewer to have a quick review first, if there is no major > > issues, I would then proceed to add the test cases and document to complete > the > > cl. > > > > Would you please take a look at the cl and let me know if you have any > concerns? > > > > Thanks, > > The first step in the process is the proposal: > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > Did one for this ever go out? No, we didn't submit this one since I don't know the process. Now it is already out in R54. Do we need to make up?
I made some changes to the cl, the major change I made is too add a new clipboardReadWrite permission, so that the installation time warning will just show one string to show it can "Access your clipboard". This is suggested by UI designer. By the way, the API proposal is out and seems going well. I am looking forward to landing this cl soon.
devlin@, I have talked to dcheng, he is fine with the cl, and will bless it later. Would you please take a look at it?
On 2016/11/16 00:41:04, jennyz wrote: > devlin@, I have talked to dcheng, he is fine with the cl, and will bless it > later. Would you please take a look at it? FYI, it looks like I'm not on the reviewer or cc list, so I wasn't aware this was ready for another look. Be sure to check the reviewers if you're waiting on a response. :)
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Two additional high-level comments: - This needs tests - From the API proposal, it was implied that this was designed to be a temporary fix and would be removed once we had an open-web alternative. What's the plan for doing that? How are we going to ensure we can deprecate this? https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:60: private: private dtor? https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:71: : image_save_success_callback_(nullptr), are these initializations needed? https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:76: ClipboardExtensionHelper::~ClipboardExtensionHelper() {} What if the clipboard_image_data_decoder_ outlives the ClipboardExtensionHelper? https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_ = new ClipboardImageDataDecoder(this); Add a note that this manages its own lifetime. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:97: clipboard_image_data_decoder_ = NULL; nullptr in new code https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.h (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.h:21: namespace clipboard = api::clipboard; not allowed in .h files https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.h:52: std::unique_ptr<ui::ScopedClipboardWriter> clipboard_writer_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { Why do we need this permission? https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:53: EXTENSION_FUNCTION_VALIDATE(params.get()); s/params.get()/params https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:72: Respond(OneArgument(base::MakeUnique<base::FundamentalValue>(false))); Do we want to convey the error to the extension at all? https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:230: virtual void SaveImageDataToClipboard( Why does this need to be a method on ExtensionsBrowserClient? Why can't the API use the helper directly and own it? https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" This will disable this entire api for any existing extensions. https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/clipboard.idl:30: SetImageDataCallback callback); Why a boolean success callback? In which circumstances can this fail? Should we be returning an error instead?
I think it's a bit confusing that we now have four different clipboard permissions (and it's non-obvious if clipboardRead + clipboardWrite == clipboardReadWrite) https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_ = new ClipboardImageDataDecoder(this); On 2016/11/16 02:34:33, Devlin wrote: > Add a note that this manages its own lifetime. Maybe it'd be better to just have the lifetime managed here? We have to null out the pointer anyway, so it seems like we can just make it a std::unique_ptr here. https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:230: virtual void SaveImageDataToClipboard( On 2016/11/16 02:34:33, Devlin wrote: > Why does this need to be a method on ExtensionsBrowserClient? Why can't the API > use the helper directly and own it? The problem is ImageDecoder is a content API, so //extensions can't use it. (I guess the alternative is to move the clipboard API into chrome/*/extensions?) https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/tes... File extensions/browser/test_extensions_browser_client.cc (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/tes... extensions/browser/test_extensions_browser_client.cc:86: Nit: remove this newline.
https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:230: virtual void SaveImageDataToClipboard( On 2016/11/17 01:13:32, dcheng wrote: > On 2016/11/16 02:34:33, Devlin wrote: > > Why does this need to be a method on ExtensionsBrowserClient? Why can't the > API > > use the helper directly and own it? > > The problem is ImageDecoder is a content API, so //extensions can't use it. > > (I guess the alternative is to move the clipboard API into chrome/*/extensions?) //extensions already depends on //content
I added the test code and updated the patch addressing the comments. Please take another look. Thanks, https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:60: private: On 2016/11/16 02:34:33, Devlin wrote: > private dtor? Done. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:71: : image_save_success_callback_(nullptr), On 2016/11/16 02:34:33, Devlin wrote: > are these initializations needed? removed. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:76: ClipboardExtensionHelper::~ClipboardExtensionHelper() {} On 2016/11/16 02:34:33, Devlin wrote: > What if the clipboard_image_data_decoder_ outlives the ClipboardExtensionHelper? Good catch. Make ClipboardExtensionHelper a WeakPtr and pass to clipboard_image_data_decoder_, check if it is still valid before calling its function. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_ = new ClipboardImageDataDecoder(this); On 2016/11/16 02:34:33, Devlin wrote: > Add a note that this manages its own lifetime. Done. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_ = new ClipboardImageDataDecoder(this); On 2016/11/17 01:13:32, dcheng wrote: > On 2016/11/16 02:34:33, Devlin wrote: > > Add a note that this manages its own lifetime. > > Maybe it'd be better to just have the lifetime managed here? We have to null out > the pointer anyway, so it seems like we can just make it a std::unique_ptr here. Changed to pass WeakPtr into clipboard_image_data_decoder_ to take care of the case clipboard_image_data_decoder_ outlives ClipboardExtensionHelper object. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:97: clipboard_image_data_decoder_ = NULL; On 2016/11/16 02:34:33, Devlin wrote: > nullptr in new code Done. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.h (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.h:21: namespace clipboard = api::clipboard; On 2016/11/16 02:34:33, Devlin wrote: > not allowed in .h files Done. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.h:52: std::unique_ptr<ui::ScopedClipboardWriter> clipboard_writer_; On 2016/11/16 02:34:33, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/11/16 02:34:33, Devlin wrote: > Why do we need this permission? This permission is need to showing the warning string for "accessing clipboard data" at the installation time of the app/extension who use chrome.clipboard. https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:53: EXTENSION_FUNCTION_VALIDATE(params.get()); On 2016/11/16 02:34:33, Devlin wrote: > s/params.get()/params Done. https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:72: Respond(OneArgument(base::MakeUnique<base::FundamentalValue>(false))); On 2016/11/16 02:34:33, Devlin wrote: > Do we want to convey the error to the extension at all? Changed to use lastError to convey the error. https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:230: virtual void SaveImageDataToClipboard( On 2016/11/17 01:25:33, Devlin wrote: > On 2016/11/17 01:13:32, dcheng wrote: > > On 2016/11/16 02:34:33, Devlin wrote: > > > Why does this need to be a method on ExtensionsBrowserClient? Why can't the > > API > > > use the helper directly and own it? > > > > The problem is ImageDecoder is a content API, so //extensions can't use it. > > > > (I guess the alternative is to move the clipboard API into > chrome/*/extensions?) > > //extensions already depends on //content But the ImageDecoder we use is under chrome/browser, not in content. https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/tes... File extensions/browser/test_extensions_browser_client.cc (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/browser/tes... extensions/browser/test_extensions_browser_client.cc:86: On 2016/11/17 01:13:32, dcheng wrote: > Nit: remove this newline. Done. https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/11/16 02:34:33, Devlin wrote: > This will disable this entire api for any existing extensions. This extension is a new one we added for Citrix remote app, so far they are the only client who uses this API, no one has use this extension yet, so it should be ok to make change now. https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/clipboard.idl:30: SetImageDataCallback callback); On 2016/11/16 02:34:33, Devlin wrote: > Why a boolean success callback? In which circumstances can this fail? Should > we be returning an error instead? Removed the boolean argument and pass error with chrome.runtime.lastError instead.
On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > - From the API proposal, it was implied that this was designed to be a temporary > fix and would be removed once we had an open-web alternative. What's the plan > for doing that? How are we going to ensure we can deprecate this? Was this ever answered? Also, you have a bunch of red bots... https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/07 01:21:48, jennyz wrote: > On 2016/11/16 02:34:33, Devlin wrote: > > Why do we need this permission? > > This permission is need to showing the warning string for "accessing clipboard > data" at the installation time of the app/extension who use chrome.clipboard. But we have clipboardRead and clipboardWrite; can't we just require both of those instead of making a new permission? https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/07 01:21:48, jennyz wrote: > On 2016/11/16 02:34:33, Devlin wrote: > > This will disable this entire api for any existing extensions. > > This extension is a new one we added for Citrix remote app, so far they are the > only client who uses this API, no one has use this extension yet, so it should > be ok to make change now. Where is this whitelist enforced? https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { What's the reason this can't be a unittest? https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:30: #if defined(OS_CHROMEOS) Is this API only supported on CrOS? If so, can't we make this file CrOS-only? https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:31: if (type == clipboard::IMAGE_TYPE_PNG) { prefer a switch https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:44: void Cancel() { Shouldn't we also call ImageDecoder::Cancel()? https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:51: if (!cancel_flag_.IsSet() && owner_) If we *do* want this approach, instead of doing this, we could just null out owner_ in Cancel(). https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:64: base::WeakPtr<ClipboardExtensionHelper> owner_; What was wrong with dcheng's suggestion to have the lifetime managed? https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_->Cancel(); So only one item can be using the decode api at any time? Why is that? https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:102: image_save_error_callback_.Run(); prefer using base::ResetAndReturn or a OnceCallback. (ditto for other callbacks) https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) (all files) https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:8: console.log('onLaunched'); remove debugging logs https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:11: 'height': 500 }, Please follow JS style here. Also, if you don't do anything with the callback, don't pass one,. So, something like: chrome.app.window.create('app_main.html', {width: 500, height: 500}); https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:20: if (!chrome.runtime.lastError) { Can't this big chunk just be chrome.test.assertEq(expectSucceed, !!chrome.runtime.lastError); ? https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:32: chrome.test.sendMessage('test success ' + testSuccessCount); why do we need these messages? The test system will fail if any tests fail https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); Rietveld won't let me comment on the pictures, so commenting here. Are you sure that these images are not copyrighted/licenses, etc? Are free to use and distribute with the open source chromium license? Is there a reason we can't just use one of the pngs we already have in chrome? https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:72: Respond(Error("Failed to save image data on clipboard")); Do we want to indicate why? This isn't a very helpful error. https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.h (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.h:15: namespace clipboard = api::clipboard; See comment here. https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... Note: Often reviewers will mention a general comment once, and it should be applied to all files in a patch set. This helps us avoid having a patch where we have a bunch of comments addressing the same issue. :) https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:48: namespace clipboard = api::clipboard; ditto (https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi...) https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... extensions/common/api/clipboard.idl:24: // Sets image data to clipboard. Doesn't it also return the result? https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... extensions/common/api/clipboard.idl:27: // |type|: The image type, the supported types are: "png", "jpeg". The documentation will include a link (or inlining) the enum, so we don't need to specify supported types.
https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_ = new ClipboardImageDataDecoder(this); On 2016/11/17 01:13:32, dcheng wrote: > On 2016/11/16 02:34:33, Devlin wrote: > > Add a note that this manages its own lifetime. > > Maybe it'd be better to just have the lifetime managed here? We have to null out > the pointer anyway, so it seems like we can just make it a std::unique_ptr here. Yes, changed to make it td::unique_ptr. Thanks for advice. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { On 2016/12/09 15:23:43, Devlin wrote: > What's the reason this can't be a unittest? I made ClipboardExtensionApiTest into a interactive_ui_tests because the first clipboard API test for ClipboardDataChanged needs interactive UI action. The SetImageData test does not need interactive ui action, but I put it here for convenience reason. If you are really strong about it should not be here, I can change. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:30: #if defined(OS_CHROMEOS) On 2016/12/09 15:23:43, Devlin wrote: > Is this API only supported on CrOS? If so, can't we make this file CrOS-only? Yes, this API is Cros-only, I made a couple of changes to make the related files CrOS-only, including this file. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:31: if (type == clipboard::IMAGE_TYPE_PNG) { On 2016/12/09 15:23:43, Devlin (in MON - PST plus 3) wrote: > prefer a switch Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:44: void Cancel() { On 2016/12/09 15:23:48, Devlin (in MON - PST plus 3) wrote: > Shouldn't we also call ImageDecoder::Cancel()? Yes, good suggestion. Now I changed to call ImageDecoder::Cancel() and get rid of cancel_flag_. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:51: if (!cancel_flag_.IsSet() && owner_) On 2016/12/09 15:23:45, Devlin (in MON - PST plus 3) wrote: > If we *do* want this approach, instead of doing this, we could just null out > owner_ in Cancel(). Changed to use unique_ptr to manage the life cycle. No need to check owner_ on callback. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:64: base::WeakPtr<ClipboardExtensionHelper> owner_; On 2016/12/09 15:23:48, Devlin (in MON - PST plus 3) wrote: > What was wrong with dcheng's suggestion to have the lifetime managed? > > https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... Yes, changed to use unique_ptr to manage the lifetime. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:90: clipboard_image_data_decoder_->Cancel(); On 2016/12/09 15:23:47, Devlin (in MON - PST plus 3) wrote: > So only one item can be using the decode api at any time? Why is that? The clipboard can only save one image from the SetImageData call, which should be the most recent request in the multiple paste actions cases. Therefore, no need to get the results from the previous decode api calls. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper.cc:102: image_save_error_callback_.Run(); On 2016/12/09 15:23:46, Devlin (in MON - PST plus 3) wrote: > prefer using base::ResetAndReturn or a OnceCallback. (ditto for other callbacks) Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/12/09 15:23:48, Devlin wrote: > no (c) (all files) Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:8: console.log('onLaunched'); On 2016/12/09 15:23:49, Devlin wrote: > remove debugging logs Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/main.js:11: 'height': 500 }, On 2016/12/09 15:23:50, Devlin wrote: > Please follow JS style here. Also, if you don't do anything with the callback, > don't pass one,. > > So, something like: > chrome.app.window.create('app_main.html', {width: 500, height: 500}); Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:20: if (!chrome.runtime.lastError) { On 2016/12/09 15:23:51, Devlin wrote: > Can't this big chunk just be > chrome.test.assertEq(expectSucceed, !!chrome.runtime.lastError); > ? Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:32: chrome.test.sendMessage('test success ' + testSuccessCount); On 2016/12/09 15:23:51, Devlin wrote: > why do we need these messages? The test system will fail if any tests fail Done. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); On 2016/12/09 15:23:52, Devlin wrote: > Rietveld won't let me comment on the pictures, so commenting here. > > Are you sure that these images are not copyrighted/licenses, etc? Are free to > use and distribute with the open source chromium license? > > Is there a reason we can't just use one of the pngs we already have in chrome? Good point. Changed to use the existing image files in chrome. New image files used: chrome/test/data/extensions/icon1.png: existing file chrome/test/data/extensions/test.jpg: copied from chrome/test/data/extensions/api_test/wallpaper/test.jpg chrome/test/data/extensions/redirect_target.gif: copied from chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:72: Respond(Error("Failed to save image data on clipboard")); On 2016/12/09 15:23:53, Devlin wrote: > Do we want to indicate why? This isn't a very helpful error. Changed to accept an error string for detailed error message. https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.h (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.h:15: namespace clipboard = api::clipboard; On 2016/12/09 15:23:53, Devlin wrote: > See comment here. > https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi... > > Note: Often reviewers will mention a general comment once, and it should be > applied to all files in a patch set. This helps us avoid having a patch where > we have a bunch of comments addressing the same issue. :) Done. https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:48: namespace clipboard = api::clipboard; On 2016/12/09 15:23:54, Devlin wrote: > ditto > (https://codereview.chromium.org/2379573008/diff/100001/chrome/browser/extensi...) Done. https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... extensions/common/api/clipboard.idl:24: // Sets image data to clipboard. On 2016/12/09 15:23:54, Devlin wrote: > Doesn't it also return the result? Now I used chrome.runtime.lastError to pass the error, it won't return any result. https://codereview.chromium.org/2379573008/diff/120001/extensions/common/api/... extensions/common/api/clipboard.idl:27: // |type|: The image type, the supported types are: "png", "jpeg". On 2016/12/09 15:23:54, Devlin wrote: > The documentation will include a link (or inlining) the enum, so we don't need > to specify supported types. I update the comment here, but I am not exactly sure how to include a link (on inlining) the enum. Any example?
It seems like we still haven't addressed one of my original questions from ~4 weeks ago - before I take another look, can we answer this? On 2016/12/09 15:23:55, Devlin wrote: > On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > > - From the API proposal, it was implied that this was designed to be a > temporary > > fix and would be removed once we had an open-web alternative. What's the plan > > for doing that? How are we going to ensure we can deprecate this? > > Was this ever answered?
On 2016/12/15 00:10:44, Devlin wrote: > It seems like we still haven't addressed one of my original questions from ~4 > weeks ago - before I take another look, can we answer this? > > On 2016/12/09 15:23:55, Devlin wrote: > > On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > > > - From the API proposal, it was implied that this was designed to be a > > temporary > > > fix and would be removed once we had an open-web alternative. What's the > plan > > > for doing that? How are we going to ensure we can deprecate this? > > > > Was this ever answered? Currently, this API is going to be used by Citrix remote app, and it is marked as chromeos and platform app only. With R57 release, we are going to discourage user to write platform apps. With Citrix dev team, we have direct communication, we can coordinate to have them migrate to open-web alternative. With the deprecation of platform apps, it is less likely someone is going to write a new platform app to use clipboard API. Therefore, the major migration will be for citrix app, which we can work directly with. To make it more restrictive, we can even whitelist it for citrix app only, but our PM would not like to do this last time I talked to her.
On 2016/12/15 06:11:36, jennyz wrote: > On 2016/12/15 00:10:44, Devlin wrote: > > It seems like we still haven't addressed one of my original questions from ~4 > > weeks ago - before I take another look, can we answer this? > > > > On 2016/12/09 15:23:55, Devlin wrote: > > > On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > > > > - From the API proposal, it was implied that this was designed to be a > > > temporary > > > > fix and would be removed once we had an open-web alternative. What's the > > plan > > > > for doing that? How are we going to ensure we can deprecate this? > > > > > > Was this ever answered? > > Currently, this API is going to be used by Citrix remote app, and it is marked > as chromeos and platform app only. With R57 release, we are going to discourage > user to write platform apps. With Citrix dev team, we have direct communication, > we can coordinate to have them migrate to open-web alternative. With the > deprecation of platform apps, it is less likely someone is going to write a new > platform app to use clipboard API. Therefore, the major migration will be for > citrix app, which we can work directly with. To make it more restrictive, we can > even whitelist it for citrix app only, but our PM would not like to do this last > time I talked to her. If we don't whitelist this to only citrix, then other platform apps will use it (and why not? It's a public API). Then, when we remove support, those apps will break. We need to have a concrete (and hopefully public) plan for how to deprecate this, and make it clear from the beginning that this is a temporary solution until the open web alternative is available.
On 2016/12/15 17:19:59, Devlin wrote: > On 2016/12/15 06:11:36, jennyz wrote: > > On 2016/12/15 00:10:44, Devlin wrote: > > > It seems like we still haven't addressed one of my original questions from > ~4 > > > weeks ago - before I take another look, can we answer this? > > > > > > On 2016/12/09 15:23:55, Devlin wrote: > > > > On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > > > > > - From the API proposal, it was implied that this was designed to be a > > > > temporary > > > > > fix and would be removed once we had an open-web alternative. What's > the > > > plan > > > > > for doing that? How are we going to ensure we can deprecate this? > > > > > > > > Was this ever answered? > > > > Currently, this API is going to be used by Citrix remote app, and it is marked > > as chromeos and platform app only. With R57 release, we are going to > discourage > > user to write platform apps. With Citrix dev team, we have direct > communication, > > we can coordinate to have them migrate to open-web alternative. With the > > deprecation of platform apps, it is less likely someone is going to write a > new > > platform app to use clipboard API. Therefore, the major migration will be for > > citrix app, which we can work directly with. To make it more restrictive, we > can > > even whitelist it for citrix app only, but our PM would not like to do this > last > > time I talked to her. > > If we don't whitelist this to only citrix, then other platform apps will use it > (and why not? It's a public API). Then, when we remove support, those apps > will break. We need to have a concrete (and hopefully public) plan for how to > deprecate this, and make it clear from the beginning that this is a temporary > solution until the open web alternative is available. (To be clear, I'm not suggesting we whitelist this, I think open APIs are always preferable. However, we need to make sure that we don't break everyone without due warning.)
On 2016/12/15 17:20:39, Devlin wrote: > On 2016/12/15 17:19:59, Devlin wrote: > > On 2016/12/15 06:11:36, jennyz wrote: > > > On 2016/12/15 00:10:44, Devlin wrote: > > > > It seems like we still haven't addressed one of my original questions from > > ~4 > > > > weeks ago - before I take another look, can we answer this? > > > > > > > > On 2016/12/09 15:23:55, Devlin wrote: > > > > > On 2016/11/16 02:34:33, Devlin (in MON - PST plus 3) wrote: > > > > > > - From the API proposal, it was implied that this was designed to be a > > > > > temporary > > > > > > fix and would be removed once we had an open-web alternative. What's > > the > > > > plan > > > > > > for doing that? How are we going to ensure we can deprecate this? > > > > > > > > > > Was this ever answered? > > > > > > Currently, this API is going to be used by Citrix remote app, and it is > marked > > > as chromeos and platform app only. With R57 release, we are going to > > discourage > > > user to write platform apps. With Citrix dev team, we have direct > > communication, > > > we can coordinate to have them migrate to open-web alternative. With the > > > deprecation of platform apps, it is less likely someone is going to write a > > new > > > platform app to use clipboard API. Therefore, the major migration will be > for > > > citrix app, which we can work directly with. To make it more restrictive, we > > can > > > even whitelist it for citrix app only, but our PM would not like to do this > > last > > > time I talked to her. > > > > If we don't whitelist this to only citrix, then other platform apps will use > it > > (and why not? It's a public API). Then, when we remove support, those apps > > will break. We need to have a concrete (and hopefully public) plan for how to > > deprecate this, and make it clear from the beginning that this is a temporary > > solution until the open web alternative is available. > > (To be clear, I'm not suggesting we whitelist this, I think open APIs are always > preferable. However, we need to make sure that we don't break everyone without > due warning.) I have documented this in the clipboard.idl. Please see new patch uploaded.
https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/09 15:23:43, Devlin wrote: > On 2016/12/07 01:21:48, jennyz wrote: > > On 2016/11/16 02:34:33, Devlin wrote: > > > Why do we need this permission? > > > > This permission is need to showing the warning string for "accessing clipboard > > data" at the installation time of the app/extension who use chrome.clipboard. > > But we have clipboardRead and clipboardWrite; can't we just require both of > those instead of making a new permission? This still wasn't answered, either?
https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { On 2016/12/14 01:15:35, jennyz wrote: > On 2016/12/09 15:23:43, Devlin wrote: > > What's the reason this can't be a unittest? > > I made ClipboardExtensionApiTest into a interactive_ui_tests because the first > clipboard API test for ClipboardDataChanged needs interactive UI action. The > SetImageData test does not need interactive ui action, but I put it here for > convenience reason. If you are really strong about it should not be here, I can > change. If it can, let's make it a unit test. Unit tests are faster and more stable, and should always be preferred, all other things equal. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); On 2016/12/14 01:15:36, jennyz wrote: > On 2016/12/09 15:23:52, Devlin wrote: > > Rietveld won't let me comment on the pictures, so commenting here. > > > > Are you sure that these images are not copyrighted/licenses, etc? Are free to > > use and distribute with the open source chromium license? > > > > Is there a reason we can't just use one of the pngs we already have in chrome? > > Good point. Changed to use the existing image files in chrome. New image files > used: > chrome/test/data/extensions/icon1.png: existing file > chrome/test/data/extensions/test.jpg: copied from > chrome/test/data/extensions/api_test/wallpaper/test.jpg > chrome/test/data/extensions/redirect_target.gif: copied from > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif I see we're still adding the other images, though? https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extensions_browser_client.h:61: void SaveImageDataToClipboard(const std::vector<char>& image_data, If you're convinced we can't implement this in //extensions, this should go on the ChromeExtensionsAPIClient. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:12: #include "extensions/browser/api/clipboard/clipboard_api.h" needed? https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:25: : owner_(owner) {} this seems like it needs to be git cl format'd. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:66: ClipboardExtensionHelper* owner_; Document this (in particular, lifetime). https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:89: clipboard_image_data_decoder_.reset(new ClipboardImageDataDecoder(this)); Is there a reason we need to reset this each time? Could we in fact just have this be stack-allocated? https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:98: std::string("Image data decoding failed")); const char[] can be implicitly converted to std::string; no need to have the ctor here. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:107: clipboard_writer_.reset( here, too, why do we need to reset? https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:14: #include "ui/gfx/image/image_skia.h" needed? https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:38: class ClipboardImageDataDecoder; Why protected instead of private? https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:11: oReq.open("GET", imageUrl, true); single quotes in js. https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:112: typedef base::Callback<void(const std::string&)> ErrorCallback; prefer "using" syntax in new code. That said, for this, it might not be worth the alias. https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... extensions/common/api/clipboard.idl:7: // chromeos platform apps until open-web alternative is available. It will be links to the open web alternative planning stage? Rough timeline? https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... extensions/common/api/clipboard.idl:30: // |type|: Type of |image_data|. instead of saying "Type of |image_data|", let's say something like "The type of image being passed.". Note that these comments directly become our public facing documentation.
https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { On 2016/12/16 02:26:15, Devlin wrote: > On 2016/12/14 01:15:35, jennyz wrote: > > On 2016/12/09 15:23:43, Devlin wrote: > > > What's the reason this can't be a unittest? > > > > I made ClipboardExtensionApiTest into a interactive_ui_tests because the first > > clipboard API test for ClipboardDataChanged needs interactive UI action. The > > SetImageData test does not need interactive ui action, but I put it here for > > convenience reason. If you are really strong about it should not be here, I > can > > change. > > If it can, let's make it a unit test. Unit tests are faster and more stable, > and should always be preferred, all other things equal. I am not familiar with writing unit test for extension api. I will do some research and make a change later. You may review the rest of the changes first. https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); On 2016/12/16 02:26:15, Devlin wrote: > On 2016/12/14 01:15:36, jennyz wrote: > > On 2016/12/09 15:23:52, Devlin wrote: > > > Rietveld won't let me comment on the pictures, so commenting here. > > > > > > Are you sure that these images are not copyrighted/licenses, etc? Are free > to > > > use and distribute with the open source chromium license? > > > > > > Is there a reason we can't just use one of the pngs we already have in > chrome? > > > > Good point. Changed to use the existing image files in chrome. New image files > > used: > > chrome/test/data/extensions/icon1.png: existing file > > chrome/test/data/extensions/test.jpg: copied from > > chrome/test/data/extensions/api_test/wallpaper/test.jpg > > chrome/test/data/extensions/redirect_target.gif: copied from > > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif > > I see we're still adding the other images, though? The other images from copied from existing image files from other directory so that they are all sitting in the same directory. If you prefer to refer directly to existing file from other directly, I can make the change. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extensions_browser_client.h:61: void SaveImageDataToClipboard(const std::vector<char>& image_data, On 2016/12/16 02:26:16, Devlin wrote: > If you're convinced we can't implement this in //extensions, this should go on > the ChromeExtensionsAPIClient. Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:12: #include "extensions/browser/api/clipboard/clipboard_api.h" On 2016/12/16 02:26:16, Devlin wrote: > needed? Removed. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:25: : owner_(owner) {} On 2016/12/16 02:26:16, Devlin wrote: > this seems like it needs to be git cl format'd. Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:66: ClipboardExtensionHelper* owner_; On 2016/12/16 02:26:16, Devlin wrote: > Document this (in particular, lifetime). Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:89: clipboard_image_data_decoder_.reset(new ClipboardImageDataDecoder(this)); On 2016/12/16 02:26:16, Devlin wrote: > Is there a reason we need to reset this each time? Could we in fact just have > this be stack-allocated? Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:98: std::string("Image data decoding failed")); On 2016/12/16 02:26:16, Devlin wrote: > const char[] can be implicitly converted to std::string; no need to have the > ctor here. Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:107: clipboard_writer_.reset( On 2016/12/16 02:26:16, Devlin wrote: > here, too, why do we need to reset? Done. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:14: #include "ui/gfx/image/image_skia.h" On 2016/12/16 02:26:16, Devlin wrote: > needed? Removed. https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:38: class ClipboardImageDataDecoder; On 2016/12/16 02:26:16, Devlin wrote: > Why protected instead of private? Make it private. https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:11: oReq.open("GET", imageUrl, true); On 2016/12/16 02:26:16, Devlin wrote: > single quotes in js. Done. https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... File extensions/browser/extensions_browser_client.h (right): https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... extensions/browser/extensions_browser_client.h:112: typedef base::Callback<void(const std::string&)> ErrorCallback; On 2016/12/16 02:26:16, Devlin wrote: > prefer "using" syntax in new code. That said, for this, it might not be worth > the alias. Done. https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... extensions/common/api/clipboard.idl:7: // chromeos platform apps until open-web alternative is available. It will be On 2016/12/16 02:26:16, Devlin wrote: > links to the open web alternative planning stage? Rough timeline? I am asking dcheng to help me find the document about open-web alternative and rough timeline. I will update this part once I get it. https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... extensions/common/api/clipboard.idl:30: // |type|: Type of |image_data|. On 2016/12/16 02:26:16, Devlin wrote: > instead of saying "Type of |image_data|", let's say something like "The type of > image being passed.". Note that these comments directly become our public > facing documentation. Done.
On 2016/12/19 07:01:36, jennyz wrote: > https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... > File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): > > https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: > IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { > On 2016/12/16 02:26:15, Devlin wrote: > > On 2016/12/14 01:15:35, jennyz wrote: > > > On 2016/12/09 15:23:43, Devlin wrote: > > > > What's the reason this can't be a unittest? > > > > > > I made ClipboardExtensionApiTest into a interactive_ui_tests because the > first > > > clipboard API test for ClipboardDataChanged needs interactive UI action. The > > > SetImageData test does not need interactive ui action, but I put it here for > > > convenience reason. If you are really strong about it should not be here, I > > can > > > change. > > > > If it can, let's make it a unit test. Unit tests are faster and more stable, > > and should always be preferred, all other things equal. > > I am not familiar with writing unit test for extension api. I will do some > research and make a change later. You may review the rest of the changes first. > > https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js > (right): > > https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... > chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: > testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); > On 2016/12/16 02:26:15, Devlin wrote: > > On 2016/12/14 01:15:36, jennyz wrote: > > > On 2016/12/09 15:23:52, Devlin wrote: > > > > Rietveld won't let me comment on the pictures, so commenting here. > > > > > > > > Are you sure that these images are not copyrighted/licenses, etc? Are > free > > to > > > > use and distribute with the open source chromium license? > > > > > > > > Is there a reason we can't just use one of the pngs we already have in > > chrome? > > > > > > Good point. Changed to use the existing image files in chrome. New image > files > > > used: > > > chrome/test/data/extensions/icon1.png: existing file > > > chrome/test/data/extensions/test.jpg: copied from > > > chrome/test/data/extensions/api_test/wallpaper/test.jpg > > > chrome/test/data/extensions/redirect_target.gif: copied from > > > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif > > > > I see we're still adding the other images, though? > > The other images from copied from existing image files from other directory so > that they are all sitting in the same directory. If you prefer to refer directly > to existing file from other directly, I can make the change. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > File chrome/browser/extensions/chrome_extensions_browser_client.h (right): > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/chrome_extensions_browser_client.h:61: void > SaveImageDataToClipboard(const std::vector<char>& image_data, > On 2016/12/16 02:26:16, Devlin wrote: > > If you're convinced we can't implement this in //extensions, this should go on > > the ChromeExtensionsAPIClient. > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:12: #include > "extensions/browser/api/clipboard/clipboard_api.h" > On 2016/12/16 02:26:16, Devlin wrote: > > needed? > > Removed. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:25: : > owner_(owner) {} > On 2016/12/16 02:26:16, Devlin wrote: > > this seems like it needs to be git cl format'd. > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:66: > ClipboardExtensionHelper* owner_; > On 2016/12/16 02:26:16, Devlin wrote: > > Document this (in particular, lifetime). > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:89: > clipboard_image_data_decoder_.reset(new ClipboardImageDataDecoder(this)); > On 2016/12/16 02:26:16, Devlin wrote: > > Is there a reason we need to reset this each time? Could we in fact just have > > this be stack-allocated? > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:98: > std::string("Image data decoding failed")); > On 2016/12/16 02:26:16, Devlin wrote: > > const char[] can be implicitly converted to std::string; no need to have the > > ctor here. > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:107: > clipboard_writer_.reset( > On 2016/12/16 02:26:16, Devlin wrote: > > here, too, why do we need to reset? > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.h:14: #include > "ui/gfx/image/image_skia.h" > On 2016/12/16 02:26:16, Devlin wrote: > > needed? > > Removed. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/clipboard_extension_helper_chromeos.h:38: class > ClipboardImageDataDecoder; > On 2016/12/16 02:26:16, Devlin wrote: > > Why protected instead of private? > > Make it private. > > https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js > (right): > > https://codereview.chromium.org/2379573008/diff/220001/chrome/test/data/exten... > chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:11: > oReq.open("GET", imageUrl, true); > On 2016/12/16 02:26:16, Devlin wrote: > > single quotes in js. > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... > File extensions/browser/extensions_browser_client.h (right): > > https://codereview.chromium.org/2379573008/diff/220001/extensions/browser/ext... > extensions/browser/extensions_browser_client.h:112: typedef > base::Callback<void(const std::string&)> ErrorCallback; > On 2016/12/16 02:26:16, Devlin wrote: > > prefer "using" syntax in new code. That said, for this, it might not be worth > > the alias. > > Done. > > https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... > File extensions/common/api/clipboard.idl (right): > > https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... > extensions/common/api/clipboard.idl:7: // chromeos platform apps until open-web > alternative is available. It will be > On 2016/12/16 02:26:16, Devlin wrote: > > links to the open web alternative planning stage? Rough timeline? > > I am asking dcheng to help me find the document about open-web alternative and > rough timeline. I will update this part once I get it. > > https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... > extensions/common/api/clipboard.idl:30: // |type|: Type of |image_data|. > On 2016/12/16 02:26:16, Devlin wrote: > > instead of saying "Type of |image_data|", let's say something like "The type > of > > image being passed.". Note that these comments directly become our public > > facing documentation. > > Done. For the unit test, I took a brief look, it may not be easy to make ImageDecoder work in the unit test, since it is running in a utility process, which seems not available in unit test. We will need to do lots of plumbing to make it a unit test, can we leave it as it for now?
https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); On 2016/12/19 07:01:36, jennyz wrote: > On 2016/12/16 02:26:15, Devlin wrote: > > On 2016/12/14 01:15:36, jennyz wrote: > > > On 2016/12/09 15:23:52, Devlin wrote: > > > > Rietveld won't let me comment on the pictures, so commenting here. > > > > > > > > Are you sure that these images are not copyrighted/licenses, etc? Are > free > > to > > > > use and distribute with the open source chromium license? > > > > > > > > Is there a reason we can't just use one of the pngs we already have in > > chrome? > > > > > > Good point. Changed to use the existing image files in chrome. New image > files > > > used: > > > chrome/test/data/extensions/icon1.png: existing file > > > chrome/test/data/extensions/test.jpg: copied from > > > chrome/test/data/extensions/api_test/wallpaper/test.jpg > > > chrome/test/data/extensions/redirect_target.gif: copied from > > > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif > > > > I see we're still adding the other images, though? > > The other images from copied from existing image files from other directory so > that they are all sitting in the same directory. If you prefer to refer directly > to existing file from other directly, I can make the change. Latest patch set: https://codereview.chromium.org/2379573008/diff/280001/chrome/test/data/exten... That's a new image being added to the source tree. We shouldn't need to do that anymore, right?
On 2016/12/19 23:08:23, Devlin wrote: > https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... > File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js > (right): > > https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... > chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: > testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); > On 2016/12/19 07:01:36, jennyz wrote: > > On 2016/12/16 02:26:15, Devlin wrote: > > > On 2016/12/14 01:15:36, jennyz wrote: > > > > On 2016/12/09 15:23:52, Devlin wrote: > > > > > Rietveld won't let me comment on the pictures, so commenting here. > > > > > > > > > > Are you sure that these images are not copyrighted/licenses, etc? Are > > free > > > to > > > > > use and distribute with the open source chromium license? > > > > > > > > > > Is there a reason we can't just use one of the pngs we already have in > > > chrome? > > > > > > > > Good point. Changed to use the existing image files in chrome. New image > > files > > > > used: > > > > chrome/test/data/extensions/icon1.png: existing file > > > > chrome/test/data/extensions/test.jpg: copied from > > > > chrome/test/data/extensions/api_test/wallpaper/test.jpg > > > > chrome/test/data/extensions/redirect_target.gif: copied from > > > > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif > > > > > > I see we're still adding the other images, though? > > > > The other images from copied from existing image files from other directory so > > that they are all sitting in the same directory. If you prefer to refer > directly > > to existing file from other directly, I can make the change. > > Latest patch set: > https://codereview.chromium.org/2379573008/diff/280001/chrome/test/data/exten... > > That's a new image being added to the source tree. We shouldn't need to do that > anymore, right? Woops, forgot to remove the image files I added previously. Removed now.
https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2379573008/diff/220001/extensions/common/api/... extensions/common/api/clipboard.idl:7: // chromeos platform apps until open-web alternative is available. It will be On 2016/12/19 07:01:36, jennyz wrote: > On 2016/12/16 02:26:16, Devlin wrote: > > links to the open web alternative planning stage? Rough timeline? > > I am asking dcheng to help me find the document about open-web alternative and > rough timeline. I will update this part once I get it. According to chrome team(Gary), they are going to start work on it in Q1, and hope to have something by Q2. There is no document link so far. I added the possible web alternative timeline here.
https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/16 02:05:08, Devlin wrote: > On 2016/12/09 15:23:43, Devlin wrote: > > On 2016/12/07 01:21:48, jennyz wrote: > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > Why do we need this permission? > > > > > > This permission is need to showing the warning string for "accessing > clipboard > > > data" at the installation time of the app/extension who use > chrome.clipboard. > > > > But we have clipboardRead and clipboardWrite; can't we just require both of > > those instead of making a new permission? > > This still wasn't answered, either? Again, this doesn't seem to have been answered.
Since there are several patches and comments, I missed some comments in the earlier patches, and I may answered some comments by reply a different patch so it didn't match at the original comment of the patch, which causes the confusion. Very sorry about the inconvenience. I checked again and answered the ones I missed. Hope this is good now. https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/09 15:23:43, Devlin wrote: > On 2016/12/07 01:21:48, jennyz wrote: > > On 2016/11/16 02:34:33, Devlin wrote: > > > Why do we need this permission? > > > > This permission is need to showing the warning string for "accessing clipboard > > data" at the installation time of the app/extension who use chrome.clipboard. > > But we have clipboardRead and clipboardWrite; can't we just require both of > those instead of making a new permission? Yes, that was what I originally did. But UI designer does not like it since it shows two separate strings instead one on the installation warning dialog, which violates one of the guideline in "CRX API Security Checklist" (https://docs.google.com/document/u/1/d/1RamP4-HJ7GAJY3yv2ju2cK50K9GhOsydJN6KI...) as follows: Collapse multiple warnings into a single warning when possible.[2] Example: Combine “Read files” and “Write files” into “Read and write files”. The string is provided by our UI designer. Therefore, I added this combined permission instead of requiring clipboardRead and clipboardWrite. https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/09 15:23:43, Devlin wrote: > On 2016/12/07 01:21:48, jennyz wrote: > > On 2016/11/16 02:34:33, Devlin wrote: > > > This will disable this entire api for any existing extensions. > > > > This extension is a new one we added for Citrix remote app, so far they are > the > > only client who uses this API, no one has use this extension yet, so it should > > be ok to make change now. > > Where is this whitelist enforced? No, I didn't enforce the whitelist. According to your comment later, I thought it is OK for me to document the deprecation plan in the clipboard.idl, instead of enforcing the whitelist. https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_apitest_chromeos.cc:21: IN_PROC_BROWSER_TEST_F(ClipboardExtensionApiTest, SetImageData) { On 2016/12/19 07:01:36, jennyz wrote: > On 2016/12/16 02:26:15, Devlin wrote: > > On 2016/12/14 01:15:35, jennyz wrote: > > > On 2016/12/09 15:23:43, Devlin wrote: > > > > What's the reason this can't be a unittest? > > > > > > I made ClipboardExtensionApiTest into a interactive_ui_tests because the > first > > > clipboard API test for ClipboardDataChanged needs interactive UI action. The > > > SetImageData test does not need interactive ui action, but I put it here for > > > convenience reason. If you are really strong about it should not be here, I > > can > > > change. > > > > If it can, let's make it a unit test. Unit tests are faster and more stable, > > and should always be preferred, all other things equal. > > I am not familiar with writing unit test for extension api. I will do some > research and make a change later. You may review the rest of the changes first. I took a look at converting it to a unit test, it looks like the ImageDecoder I used needs to run in a utility process, which is not available in unit test infrastructure. So it is not straightforward to make it a unit test. Can we leave this as a browser test? https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2379573008/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:43: testSetImageDataClipboard(baseUrl + '/penguin.png', 'png', true); On 2016/12/19 23:08:22, Devlin wrote: > On 2016/12/19 07:01:36, jennyz wrote: > > On 2016/12/16 02:26:15, Devlin wrote: > > > On 2016/12/14 01:15:36, jennyz wrote: > > > > On 2016/12/09 15:23:52, Devlin wrote: > > > > > Rietveld won't let me comment on the pictures, so commenting here. > > > > > > > > > > Are you sure that these images are not copyrighted/licenses, etc? Are > > free > > > to > > > > > use and distribute with the open source chromium license? > > > > > > > > > > Is there a reason we can't just use one of the pngs we already have in > > > chrome? > > > > > > > > Good point. Changed to use the existing image files in chrome. New image > > files > > > > used: > > > > chrome/test/data/extensions/icon1.png: existing file > > > > chrome/test/data/extensions/test.jpg: copied from > > > > chrome/test/data/extensions/api_test/wallpaper/test.jpg > > > > chrome/test/data/extensions/redirect_target.gif: copied from > > > > chrome/test/data/extensions/api_test/webrequest/cors/redirect_target.gif > > > > > > I see we're still adding the other images, though? > > > > The other images from copied from existing image files from other directory so > > that they are all sitting in the same directory. If you prefer to refer > directly > > to existing file from other directly, I can make the change. > > Latest patch set: > https://codereview.chromium.org/2379573008/diff/280001/chrome/test/data/exten... > > That's a new image being added to the source tree. We shouldn't need to do that > anymore, right? I have removed the previously added image files.
https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/20 22:16:53, jennyz wrote: > On 2016/12/09 15:23:43, Devlin wrote: > > On 2016/12/07 01:21:48, jennyz wrote: > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > Why do we need this permission? > > > > > > This permission is need to showing the warning string for "accessing > clipboard > > > data" at the installation time of the app/extension who use > chrome.clipboard. > > > > But we have clipboardRead and clipboardWrite; can't we just require both of > > those instead of making a new permission? > > Yes, that was what I originally did. But UI designer does not like it since it > shows two separate strings instead one on the installation warning dialog, which > violates one of the guideline in "CRX API Security Checklist" > (https://docs.google.com/document/u/1/d/1RamP4-HJ7GAJY3yv2ju2cK50K9GhOsydJN6KI...) > as follows: > > Collapse multiple warnings into a single warning when possible.[2] > Example: Combine “Read files” and “Write files” into “Read and write files”. > > The string is provided by our UI designer. Therefore, I added this combined > permission instead of requiring clipboardRead and clipboardWrite. > > We should be able to solve that using permission rules. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:53: ChromeExtensionsAPIClient::ChromeExtensionsAPIClient() { Why this change? https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:178: clipboard_extension_helper_.reset(new ClipboardExtensionHelper()); prefer MakeUnique https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:90: clipboard_image_data_decoder_->Cancel(); Have you thought about the case of multiple extensions trying to use this at the same time? Doesn't this mean that only that last extension call gets the result? https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:12: #include "extensions/browser/api/extensions_api_client.h" needed?
https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/20 22:16:54, jennyz wrote: > On 2016/12/09 15:23:43, Devlin wrote: > > On 2016/12/07 01:21:48, jennyz wrote: > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > This will disable this entire api for any existing extensions. > > > > > > This extension is a new one we added for Citrix remote app, so far they are > > the > > > only client who uses this API, no one has use this extension yet, so it > should > > > be ok to make change now. > > > > Where is this whitelist enforced? > > No, I didn't enforce the whitelist. According to your comment later, I thought > it is OK for me to document the deprecation plan in the clipboard.idl, instead > of enforcing the whitelist. But this API isn't *only* used for the new function being added; it's also used for the onClipboardDataChanged event. Since the clipboard API is not restricted to a whitelist, arbitrary apps and extensions can be relying on it. If we add an additional dependency, then any apps or extensions that don't specify that dependency will be unable to use it, and will suddenly break in this chrome version. Additionally, there's no reason that an extension using the onClipboardDataChanged event requires clipboardWrite, and no reason that setting the image data to the clipboard requires clipboard read. Right?
https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:153: "clipboardReadWrite": { On 2016/12/21 23:58:38, Devlin wrote: > On 2016/12/20 22:16:53, jennyz wrote: > > On 2016/12/09 15:23:43, Devlin wrote: > > > On 2016/12/07 01:21:48, jennyz wrote: > > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > > Why do we need this permission? > > > > > > > > This permission is need to showing the warning string for "accessing > > clipboard > > > > data" at the installation time of the app/extension who use > > chrome.clipboard. > > > > > > But we have clipboardRead and clipboardWrite; can't we just require both of > > > those instead of making a new permission? > > > > Yes, that was what I originally did. But UI designer does not like it since it > > shows two separate strings instead one on the installation warning dialog, > which > > violates one of the guideline in "CRX API Security Checklist" > > > (https://docs.google.com/document/u/1/d/1RamP4-HJ7GAJY3yv2ju2cK50K9GhOsydJN6KI...) > > as follows: > > > > Collapse multiple warnings into a single warning when possible.[2] > > Example: Combine “Read files” and “Write files” into “Read and write files”. > > > > The string is provided by our UI designer. Therefore, I added this combined > > permission instead of requiring clipboardRead and clipboardWrite. > > > > > > We should be able to solve that using permission rules. Removed this permission and changed to add a permission rule to associate the string with both clipboardRead and clipboardWrite permission. https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/22 16:07:49, Devlin wrote: > On 2016/12/20 22:16:54, jennyz wrote: > > On 2016/12/09 15:23:43, Devlin wrote: > > > On 2016/12/07 01:21:48, jennyz wrote: > > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > > This will disable this entire api for any existing extensions. > > > > > > > > This extension is a new one we added for Citrix remote app, so far they > are > > > the > > > > only client who uses this API, no one has use this extension yet, so it > > should > > > > be ok to make change now. > > > > > > Where is this whitelist enforced? > > > > No, I didn't enforce the whitelist. According to your comment later, I thought > > it is OK for me to document the deprecation plan in the clipboard.idl, instead > > of enforcing the whitelist. > > But this API isn't *only* used for the new function being added; it's also used > for the onClipboardDataChanged event. Since the clipboard API is not restricted > to a whitelist, arbitrary apps and extensions can be relying on it. If we add > an additional dependency, then any apps or extensions that don't specify that > dependency will be unable to use it, and will suddenly break in this chrome > version. Additionally, there's no reason that an extension using the > onClipboardDataChanged event requires clipboardWrite, and no reason that setting > the image data to the clipboard requires clipboard read. Right? Good point. I have removed clipboardReadWrite permission in the new patch. So the app depending on onClipboardDataChanged will only need to require clipboardRead permission. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:53: ChromeExtensionsAPIClient::ChromeExtensionsAPIClient() { On 2016/12/21 23:58:38, Devlin wrote: > Why this change? I thought you suggested me to move it to ChromeExtensionsAPIClient, see your previous comment: "If you're convinced we can't implement this in //extensions, this should go on the ChromeExtensionsAPIClient." https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... Maybe I misunderstood? https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:178: clipboard_extension_helper_.reset(new ClipboardExtensionHelper()); On 2016/12/21 23:58:38, Devlin wrote: > prefer MakeUnique Done. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:90: clipboard_image_data_decoder_->Cancel(); On 2016/12/21 23:58:38, Devlin wrote: > Have you thought about the case of multiple extensions trying to use this at the > same time? Doesn't this mean that only that last extension call gets the > result? In the case of multiple extensions trying to this at the "same" time to save image onto the clipboard, since the clipboard can only store one image data, like "paste" case, we still only need the same request win, it sets image data on the clipboard. I have updated the comment to make this clear. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:12: #include "extensions/browser/api/extensions_api_client.h" On 2016/12/21 23:58:38, Devlin wrote: > needed? Removed.
https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/22 22:45:20, jennyz wrote: > On 2016/12/22 16:07:49, Devlin wrote: > > On 2016/12/20 22:16:54, jennyz wrote: > > > On 2016/12/09 15:23:43, Devlin wrote: > > > > On 2016/12/07 01:21:48, jennyz wrote: > > > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > > > This will disable this entire api for any existing extensions. > > > > > > > > > > This extension is a new one we added for Citrix remote app, so far they > > are > > > > the > > > > > only client who uses this API, no one has use this extension yet, so it > > > should > > > > > be ok to make change now. > > > > > > > > Where is this whitelist enforced? > > > > > > No, I didn't enforce the whitelist. According to your comment later, I > thought > > > it is OK for me to document the deprecation plan in the clipboard.idl, > instead > > > of enforcing the whitelist. > > > > But this API isn't *only* used for the new function being added; it's also > used > > for the onClipboardDataChanged event. Since the clipboard API is not > restricted > > to a whitelist, arbitrary apps and extensions can be relying on it. If we add > > an additional dependency, then any apps or extensions that don't specify that > > dependency will be unable to use it, and will suddenly break in this chrome > > version. Additionally, there's no reason that an extension using the > > onClipboardDataChanged event requires clipboardWrite, and no reason that > setting > > the image data to the clipboard requires clipboard read. Right? > Good point. I have removed clipboardReadWrite permission in the new patch. So > the app depending on onClipboardDataChanged will only need to require > clipboardRead permission. I read your comment again. After I removed the additional clipboardReadWrite permission, if we need to make sure onClipboardDataChanged only requires clipboardRead permission and SetImageData only requires clipboardWrite, then the permission requirement is refined to API level instead of chrome.clipboard namespace level, do I need to change _api_features_.json file to reconfigure the permission dependency?
https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/22 23:31:40, jennyz wrote: > On 2016/12/22 22:45:20, jennyz wrote: > > On 2016/12/22 16:07:49, Devlin wrote: > > > On 2016/12/20 22:16:54, jennyz wrote: > > > > On 2016/12/09 15:23:43, Devlin wrote: > > > > > On 2016/12/07 01:21:48, jennyz wrote: > > > > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > > > > This will disable this entire api for any existing extensions. > > > > > > > > > > > > This extension is a new one we added for Citrix remote app, so far > they > > > are > > > > > the > > > > > > only client who uses this API, no one has use this extension yet, so > it > > > > should > > > > > > be ok to make change now. > > > > > > > > > > Where is this whitelist enforced? > > > > > > > > No, I didn't enforce the whitelist. According to your comment later, I > > thought > > > > it is OK for me to document the deprecation plan in the clipboard.idl, > > instead > > > > of enforcing the whitelist. > > > > > > But this API isn't *only* used for the new function being added; it's also > > used > > > for the onClipboardDataChanged event. Since the clipboard API is not > > restricted > > > to a whitelist, arbitrary apps and extensions can be relying on it. If we > add > > > an additional dependency, then any apps or extensions that don't specify > that > > > dependency will be unable to use it, and will suddenly break in this chrome > > > version. Additionally, there's no reason that an extension using the > > > onClipboardDataChanged event requires clipboardWrite, and no reason that > > setting > > > the image data to the clipboard requires clipboard read. Right? > > Good point. I have removed clipboardReadWrite permission in the new patch. So > > the app depending on onClipboardDataChanged will only need to require > > clipboardRead permission. > > I read your comment again. After I removed the additional clipboardReadWrite > permission, if we need to make sure onClipboardDataChanged only requires > clipboardRead permission and SetImageData only requires clipboardWrite, then the > permission requirement is refined to API level instead of chrome.clipboard > namespace level, do I need to change _api_features_.json file to reconfigure the > permission dependency? Yes, if you want different permissions for different API methods, you need to adjust the _api_features.json file. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:53: ChromeExtensionsAPIClient::ChromeExtensionsAPIClient() { On 2016/12/22 22:45:21, jennyz wrote: > On 2016/12/21 23:58:38, Devlin wrote: > > Why this change? > > I thought you suggested me to move it to ChromeExtensionsAPIClient, see your > previous comment: > > "If you're convinced we can't implement this in //extensions, this should go on > the ChromeExtensionsAPIClient." > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > > Maybe I misunderstood? Yes. But why do we need to put a newline here? It seems against clang format, is inconsistent with line 56 below, and isn't relevant to this change. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:90: clipboard_image_data_decoder_->Cancel(); On 2016/12/22 22:45:21, jennyz wrote: > On 2016/12/21 23:58:38, Devlin wrote: > > Have you thought about the case of multiple extensions trying to use this at > the > > same time? Doesn't this mean that only that last extension call gets the > > result? > > In the case of multiple extensions trying to this at the "same" time to save > image onto the clipboard, since the clipboard can only store one image data, > like "paste" case, we still only need the same request win, it sets image data > on the clipboard. I have updated the comment to make this clear. Should we send a different error, then? Rather than just saying "Image data decoding failed"?
Patchset #19 (id:360001) has been deleted
https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/100001/extensions/common/api/... extensions/common/api/_api_features.json:117: "permission:clipboard", "permission:clipboardReadWrite" On 2016/12/27 17:08:00, Devlin wrote: > On 2016/12/22 23:31:40, jennyz wrote: > > On 2016/12/22 22:45:20, jennyz wrote: > > > On 2016/12/22 16:07:49, Devlin wrote: > > > > On 2016/12/20 22:16:54, jennyz wrote: > > > > > On 2016/12/09 15:23:43, Devlin wrote: > > > > > > On 2016/12/07 01:21:48, jennyz wrote: > > > > > > > On 2016/11/16 02:34:33, Devlin wrote: > > > > > > > > This will disable this entire api for any existing extensions. > > > > > > > > > > > > > > This extension is a new one we added for Citrix remote app, so far > > they > > > > are > > > > > > the > > > > > > > only client who uses this API, no one has use this extension yet, so > > it > > > > > should > > > > > > > be ok to make change now. > > > > > > > > > > > > Where is this whitelist enforced? > > > > > > > > > > No, I didn't enforce the whitelist. According to your comment later, I > > > thought > > > > > it is OK for me to document the deprecation plan in the clipboard.idl, > > > instead > > > > > of enforcing the whitelist. > > > > > > > > But this API isn't *only* used for the new function being added; it's also > > > used > > > > for the onClipboardDataChanged event. Since the clipboard API is not > > > restricted > > > > to a whitelist, arbitrary apps and extensions can be relying on it. If we > > add > > > > an additional dependency, then any apps or extensions that don't specify > > that > > > > dependency will be unable to use it, and will suddenly break in this > chrome > > > > version. Additionally, there's no reason that an extension using the > > > > onClipboardDataChanged event requires clipboardWrite, and no reason that > > > setting > > > > the image data to the clipboard requires clipboard read. Right? > > > Good point. I have removed clipboardReadWrite permission in the new patch. > So > > > the app depending on onClipboardDataChanged will only need to require > > > clipboardRead permission. > > > > I read your comment again. After I removed the additional clipboardReadWrite > > permission, if we need to make sure onClipboardDataChanged only requires > > clipboardRead permission and SetImageData only requires clipboardWrite, then > the > > permission requirement is refined to API level instead of chrome.clipboard > > namespace level, do I need to change _api_features_.json file to reconfigure > the > > permission dependency? > > Yes, if you want different permissions for different API methods, you need to > adjust the _api_features.json file. I made the change to adjust the permissions to API method level. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/chrome_extensions_api_client.cc:53: ChromeExtensionsAPIClient::ChromeExtensionsAPIClient() { On 2016/12/27 17:08:00, Devlin wrote: > On 2016/12/22 22:45:21, jennyz wrote: > > On 2016/12/21 23:58:38, Devlin wrote: > > > Why this change? > > > > I thought you suggested me to move it to ChromeExtensionsAPIClient, see your > > previous comment: > > > > "If you're convinced we can't implement this in //extensions, this should go > on > > the ChromeExtensionsAPIClient." > > > > > https://codereview.chromium.org/2379573008/diff/220001/chrome/browser/extensi... > > > > Maybe I misunderstood? > > Yes. But why do we need to put a newline here? It seems against clang format, > is inconsistent with line 56 below, and isn't relevant to this change. Done. https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/320001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:90: clipboard_image_data_decoder_->Cancel(); On 2016/12/27 17:08:00, Devlin wrote: > On 2016/12/22 22:45:21, jennyz wrote: > > On 2016/12/21 23:58:38, Devlin wrote: > > > Have you thought about the case of multiple extensions trying to use this at > > the > > > same time? Doesn't this mean that only that last extension call gets the > > > result? > > > > In the case of multiple extensions trying to this at the "same" time to save > > image onto the clipboard, since the clipboard can only store one image data, > > like "paste" case, we still only need the same request win, it sets image data > > on the clipboard. I have updated the comment to make this clear. > > Should we send a different error, then? Rather than just saying "Image data > decoding failed"? We are sending a different error: "Request canceled", see ClipboardExtensionHelper::OnImageDecodeCancel().
The CL as a whole looks reasonable to me, so as long as devlin is on board, I'm happy. Just one drive-by thought. https://codereview.chromium.org/2379573008/diff/380001/extensions/common/perm... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/2379573008/diff/380001/extensions/common/perm... extensions/common/permissions/api_permission.h:245: kClipboardReadWrite, Nit: remove this?
rdevlin.cronin@chromium.org changed reviewers: + meacer@chromium.org
Nice! This is looking much better. :) There are a few last nits, and I'd like meacer@'s input on the permission string, since it seems a bit odd. https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:3894: + <message name="IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE" desc="Permission string for acess to clipboard for read and write permission."> typo: access https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:3895: + Access your clipboard Was this the string given by ui? It seems very technical. Most users don't know what a clipboard is. https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/permission_message_combinations_unittest.cc (right): https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... chrome/browser/extensions/permission_message_combinations_unittest.cc:1166: CreateAndInstall( slightly cleaner: const char kManifest[] = "{" " 'app': {" " ..." " }," " 'permissions': [%s]" "}"; CreateAndInstall(base::StringPrintf(kManifest, "'clipboardRead'")); CreateAndInstall( base::StringPrintf(kManifest, "'clipboardRead', 'clipboardWrite'")); CreateAndInstall(base::StringPrintf(kManifest, "'clipboardWrite'")); https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... extensions/common/api/_api_features.json:116: "dependencies": [ We can put these on one line, e.g. "dependencies": ["permission:clipboard"]
On 2016/12/29 16:53:17, Devlin wrote: > Nice! This is looking much better. :) > > There are a few last nits, and I'd like meacer@'s input on the permission > string, since it seems a bit odd. > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > chrome/app/generated_resources.grd:3894: + <message > name="IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE" desc="Permission string > for acess to clipboard for read and write permission."> > typo: access > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > chrome/app/generated_resources.grd:3895: + Access your clipboard > Was this the string given by ui? It seems very technical. Most users don't > know what a clipboard is. > > https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... > File chrome/browser/extensions/permission_message_combinations_unittest.cc > (right): > > https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... > chrome/browser/extensions/permission_message_combinations_unittest.cc:1166: > CreateAndInstall( > slightly cleaner: > const char kManifest[] = > "{" > " 'app': {" > " ..." > " }," > " 'permissions': [%s]" > "}"; > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardRead'")); > > > CreateAndInstall( > base::StringPrintf(kManifest, "'clipboardRead', 'clipboardWrite'")); > > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardWrite'")); > > https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... > File extensions/common/api/_api_features.json (right): > > https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... > extensions/common/api/_api_features.json:116: "dependencies": [ > We can put these on one line, e.g. > "dependencies": ["permission:clipboard"] Wow, that's a popular bug. I didn't know there were this many stars in the bug tracker :) I read the thread but might have missed the plan for deprecation. Is the idea to remove this API altogether once the open web API is shipped? Also, does this API require a user gesture to modify the clipboard? If that were the case, it would be similar to the already shipped clipboard write feature in blink (which requires a user gesture), and we could remove the permission from this API altogether. But I guess that's not the case. As for the strings, how about avoiding the word clipboard? IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_WRITE: "Modify data you copy and paste" IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE: "Read and modify data you copy and paste" Users might be more familiar with copy and paste than the clipboard concept, but again I don't know if my strings are any better. If the API is going to be deprecated soon enough, perhaps it's not that big of a deal.
Patchset #20 (id:400001) has been deleted
On 2016/12/29 19:09:31, Mustafa Emre Acer wrote: > On 2016/12/29 16:53:17, Devlin wrote: > > Nice! This is looking much better. :) > > > > There are a few last nits, and I'd like meacer@'s input on the permission > > string, since it seems a bit odd. > > > > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > > File chrome/app/generated_resources.grd (right): > > > > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > > chrome/app/generated_resources.grd:3894: + <message > > name="IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE" desc="Permission > string > > for acess to clipboard for read and write permission."> > > typo: access > > > > > https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... > > chrome/app/generated_resources.grd:3895: + Access your clipboard > > Was this the string given by ui? It seems very technical. Most users don't > > know what a clipboard is. > > > > > https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... > > File chrome/browser/extensions/permission_message_combinations_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... > > chrome/browser/extensions/permission_message_combinations_unittest.cc:1166: > > CreateAndInstall( > > slightly cleaner: > > const char kManifest[] = > > "{" > > " 'app': {" > > " ..." > > " }," > > " 'permissions': [%s]" > > "}"; > > > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardRead'")); > > > > > > CreateAndInstall( > > base::StringPrintf(kManifest, "'clipboardRead', 'clipboardWrite'")); > > > > > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardWrite'")); > > > > > https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... > > File extensions/common/api/_api_features.json (right): > > > > > https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... > > extensions/common/api/_api_features.json:116: "dependencies": [ > > We can put these on one line, e.g. > > "dependencies": ["permission:clipboard"] > > Wow, that's a popular bug. I didn't know there were this many stars in the bug > tracker :) > > I read the thread but might have missed the plan for deprecation. Is the idea to > remove this API altogether once the open web API is shipped? > Also, does this API require a user gesture to modify the clipboard? If that were > the case, it would be similar to the already shipped clipboard write feature in > blink (which requires a user gesture), and we could remove the permission from > this API altogether. But I guess that's not the case. > > As for the strings, how about avoiding the word clipboard? > > IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_WRITE: "Modify data you copy and paste" > IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE: "Read and modify data you copy > and paste" > > Users might be more familiar with copy and paste than the clipboard concept, but > again I don't know if my strings are any better. If the API is going to be > deprecated soon enough, perhaps it's not that big of a deal. Thanks for your quick review. This is a highly demanded feature for years from one of our major enterprise client. Once the open-web alternative is available, we can deprecate this API, but our client can't wait that long, that's why we are making this effort here. This API does not require a user gesture to modify the clipboard. For the strings, I like the ones you suggested, it is more readable to users and consistent with the old clipboardRead string. So I changed the strings to yours.
https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:3894: + <message name="IDS_EXTENSION_PROMPT_WARNING_CLIPBOARD_READWRITE" desc="Permission string for acess to clipboard for read and write permission."> On 2016/12/29 16:53:17, Devlin wrote: > typo: access Done. https://codereview.chromium.org/2379573008/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:3895: + Access your clipboard On 2016/12/29 16:53:17, Devlin wrote: > Was this the string given by ui? It seems very technical. Most users don't > know what a clipboard is. This is the string given by PM and UI designer. Changed to the ones suggested by meacer@. https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/permission_message_combinations_unittest.cc (right): https://codereview.chromium.org/2379573008/diff/380001/chrome/browser/extensi... chrome/browser/extensions/permission_message_combinations_unittest.cc:1166: CreateAndInstall( On 2016/12/29 16:53:17, Devlin wrote: > slightly cleaner: > const char kManifest[] = > "{" > " 'app': {" > " ..." > " }," > " 'permissions': [%s]" > "}"; > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardRead'")); > > > CreateAndInstall( > base::StringPrintf(kManifest, "'clipboardRead', 'clipboardWrite'")); > > > CreateAndInstall(base::StringPrintf(kManifest, "'clipboardWrite'")); Done. https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... File extensions/common/api/_api_features.json (right): https://codereview.chromium.org/2379573008/diff/380001/extensions/common/api/... extensions/common/api/_api_features.json:116: "dependencies": [ On 2016/12/29 16:53:17, Devlin wrote: > We can put these on one line, e.g. > "dependencies": ["permission:clipboard"] Done. https://codereview.chromium.org/2379573008/diff/380001/extensions/common/perm... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/2379573008/diff/380001/extensions/common/perm... extensions/common/permissions/api_permission.h:245: kClipboardReadWrite, On 2016/12/29 09:22:21, dcheng wrote: > Nit: remove this? Done.
lgtm, but please wait for meacer@ to sign off.
On 2017/01/04 19:16:21, Devlin (sheriff - maybe slow) wrote: > lgtm, but please wait for meacer@ to sign off. lgtm too, but please wait for dcheng@ to sign off :)
LGTM with two nits https://codereview.chromium.org/2379573008/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/420001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:43: default: Nit: Remove this case, because the parameter validation should have failed. That way, the compiler will also warn if this needs additional case statements in the future. https://codereview.chromium.org/2379573008/diff/420001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/420001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:66: base::Bind(&ClipboardSetImageDataFunction::OnSaveImageDataError, this)); Nit: I would just inline this into Run()
https://codereview.chromium.org/2379573008/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2379573008/diff/420001/chrome/browser/extensi... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:43: default: On 2017/01/04 19:45:23, dcheng wrote: > Nit: Remove this case, because the parameter validation should have failed. That > way, the compiler will also warn if this needs additional case statements in the > future. Done. https://codereview.chromium.org/2379573008/diff/420001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2379573008/diff/420001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:66: base::Bind(&ClipboardSetImageDataFunction::OnSaveImageDataError, this)); On 2017/01/04 19:45:23, dcheng wrote: > Nit: I would just inline this into Run() Done.
Description was changed from ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman ==========
The CQ bit was checked by jennyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, dcheng@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2379573008/#ps440001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1483572104162910,
"parent_rev": "e8d945c882e2c06e4b62938c59e3d50db6e89dce", "commit_rev":
"45d1a4cb9cf2b280a2d2349f0bf74057fae52e37"}
Message was sent while issue was closed.
Description was changed from ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman Review-Url: https://codereview.chromium.org/2379573008 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman Review-Url: https://codereview.chromium.org/2379573008 ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman Committed: https://crrev.com/6298fc54177c7f2919fc158a3b43e7a3e08f36fe Cr-Commit-Position: refs/heads/master@{#441546} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/6298fc54177c7f2919fc158a3b43e7a3e08f36fe Cr-Commit-Position: refs/heads/master@{#441546}
Message was sent while issue was closed.
Description was changed from ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=150835 TBR=isherman Committed: https://crrev.com/6298fc54177c7f2919fc158a3b43e7a3e08f36fe Cr-Commit-Position: refs/heads/master@{#441546} ========== to ========== Add SetImageData api to chrome.clipboard extension to allow js script to pass the png or jpeg encoded image data in ArrayBuffer to be written on chromeos clipboard. BUG=678794 TBR=isherman Committed: https://crrev.com/6298fc54177c7f2919fc158a3b43e7a3e08f36fe Cr-Commit-Position: refs/heads/master@{#441546} ==========
Message was sent while issue was closed.
|
