|
|
DescriptionModify SetImageData to add additional items along with image data to save on clipboard.
BUG=712874
Review-Url: https://codereview.chromium.org/2837983002
Cr-Commit-Position: refs/heads/master@{#474701}
Committed: https://chromium.googlesource.com/chromium/src/+/0d792f0d848e8cc25e568cc4d376b482115c149b
Patch Set 1 #
Total comments: 25
Patch Set 2 : Make additional data items parameter optional and address other comments. #
Total comments: 32
Patch Set 3 : Address more code review comments. #Patch Set 4 : update open web solution timeline. #
Total comments: 6
Patch Set 5 : Minor changes addressing code reviews. #
Total comments: 6
Patch Set 6 : Nits and rebase. #Messages
Total messages: 29 (11 generated)
Description was changed from ========== Modify SetImageData to add additional items along with image data to save on clipboard. BUG=712874 ========== to ========== Modify SetImageData to add additional items along with image data to save on clipboard. BUG=712874 ==========
jennyz@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:127: scw.WriteHTML(base::UTF8ToUTF16(item.data), std::string("")); So far this text (item.data) is totally unvalidated, beyond knowing it's a string. dcheng@: Is there any security risk here? https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:23: struct AdditionalDataItem { How is this different from the generated type? https://codereview.chromium.org/2837983002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2837983002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:10: expectSucceed) { We should expand this to take some kind of expected error. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. Any update on this? https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:19: // "text/plain" and "text/html". if these are the only allowed types, why not an enum? https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:24: DOMString data; From the implementation, I see that this has a limit, but it's not described here in the API? https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:49: AdditionalDataItem[] additional_items, This is not a backwards compatible change, and will break all existing clients.
(left a few drive-by thoughts as well) https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:127: scw.WriteHTML(base::UTF8ToUTF16(item.data), std::string("")); On 2017/04/26 18:20:15, Devlin wrote: > So far this text (item.data) is totally unvalidated, beyond knowing it's a > string. > > dcheng@: Is there any security risk here? It mirrors what we do for a renderer that calls DataTransfer.setData("text/html", ...), so I think this is fine. See https://cs.chromium.org/chromium/src/content/renderer/webclipboard_impl.cc?rc... (which is populated by https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard..., and converted in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard...) That being said, we should prefer std::string() over std::string("") https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... extensions/browser/api/clipboard/clipboard_api.cc:90: std::ceil(4 * 1024 * 4 * 1024 * 4 / 3) * 4 + 2048; This is currently a runtime calculation, but it should just be calculated out. FWIW, I think it'd be easier to just pick an arbitrary byte limit and just use it rather than trying to say we'll allow images up to a certain size. In fact, Chrome probably won't even load a data URL longer than 2 MB... https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... extensions/browser/api/clipboard/clipboard_api.cc:93: return false; I would just omit this, this will get caught by the for loop. No need to handle this case separately.
Patchset #2 (id:20001) has been deleted
Sorry for the late delay for addressing the review feedback, I was pulled off by other duties. I have uploaded the new patch to address the comments. PTAL! Thanks, https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:127: scw.WriteHTML(base::UTF8ToUTF16(item.data), std::string("")); On 2017/04/27 15:42:09, dcheng wrote: > On 2017/04/26 18:20:15, Devlin wrote: > > So far this text (item.data) is totally unvalidated, beyond knowing it's a > > string. > > > > dcheng@: Is there any security risk here? > > It mirrors what we do for a renderer that calls > DataTransfer.setData("text/html", ...), so I think this is fine. > > See > https://cs.chromium.org/chromium/src/content/renderer/webclipboard_impl.cc?rc... > (which is populated by > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard..., > and converted in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard...) > > That being said, we should prefer std::string() over std::string("") Done. https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:127: scw.WriteHTML(base::UTF8ToUTF16(item.data), std::string("")); On 2017/04/26 18:20:15, Devlin (catching up) wrote: > So far this text (item.data) is totally unvalidated, beyond knowing it's a > string. > > dcheng@: Is there any security risk here? See dcheng@'s comment. https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/clipboard_extension_helper_chromeos.h (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/clipboard_extension_helper_chromeos.h:23: struct AdditionalDataItem { On 2017/04/26 18:20:15, Devlin wrote: > How is this different from the generated type? Not really, so removed this struct, just use the generated type. https://codereview.chromium.org/2837983002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2837983002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:10: expectSucceed) { On 2017/04/26 18:20:15, Devlin (catching up) wrote: > We should expand this to take some kind of expected error. Done. https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... extensions/browser/api/clipboard/clipboard_api.cc:90: std::ceil(4 * 1024 * 4 * 1024 * 4 / 3) * 4 + 2048; On 2017/04/27 15:42:09, dcheng wrote: > This is currently a runtime calculation, but it should just be calculated out. > > FWIW, I think it'd be easier to just pick an arbitrary byte limit and just use > it rather than trying to say we'll allow images up to a certain size. In fact, > Chrome probably won't even load a data URL longer than 2 MB... Done. https://codereview.chromium.org/2837983002/diff/1/extensions/browser/api/clip... extensions/browser/api/clipboard/clipboard_api.cc:93: return false; On 2017/04/27 15:42:09, dcheng wrote: > I would just omit this, this will get caught by the for loop. No need to handle > this case separately. Done. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/04/26 18:20:15, Devlin wrote: > Any update on this? I will ask dcheng@ for update, he is OOO until May 2nd. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:19: // "text/plain" and "text/html". On 2017/04/26 18:20:15, Devlin wrote: > if these are the only allowed types, why not an enum? Done. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:24: DOMString data; On 2017/04/26 18:20:15, Devlin (catching up) wrote: > From the implementation, I see that this has a limit, but it's not described > here in the API? Done. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:49: AdditionalDataItem[] additional_items, On 2017/04/26 18:20:15, Devlin (catching up) wrote: > This is not a backwards compatible change, and will break all existing clients. Mark the new parameter as optional.
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/05/16 18:22:03, jennyz wrote: > On 2017/04/26 18:20:15, Devlin wrote: > > Any update on this? > > I will ask dcheng@ for update, he is OOO until May 2nd. Past May 2 :) https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:105: additonal_items_.push_back(std::move(data_item)); If we're just going to move all of these anyway, why not just pass the vector by value and move it in? Unless we want to allow null, in which case we should just use swap(). https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:15: chrome.test.assertEq(expectedError, chrome.runtime.lastError); instead of this, which requires callers to pass in {message: <value>}, let's do: if (expectedError) chrome.test.assertLastError(expectedError); https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:19: function testSetImageDataClipboardWithAdditionalData( Seems like we can combine this with testSetImageDataClipboard pretty easily, since we can just not pass the additional parameters if we don't need them (they default to undefined). https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:20: imageUrl, imageType, additional_items, expectedError) { additional_items: useJsStyle :) https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:30: if (additional_items == null) { why not just if (additionalItems)? https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:51: baseUrl + '/icon1.png', 'png', undefined); no need to pass undefined https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:61: 'message': 'Image data decoding failed.' no need for quotes around keys, but moot with other suggestion. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:83: function testSavePngImageWithAdditionalDataToClipboardDuplicateTypeItems(baseUrl) { lines <= 80 char https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:59: // Fill in the omitted additiona data items with empty data. typo: additional https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:60: if (!params->additional_items.get()) no need for .get() https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:64: return RespondNow(Error("Unsupported additional_items parameter data.")); additionalItems https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:91: if (item.type == clipboard::DATA_ITEM_TYPE_TEXT_PLAIN) { use a switch statement https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:15: enum DataItemType {text_plain, text_html}; prefer jsCasing https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:20: // MIME type of the additional data item, supported types are: "supported types" portion of this comment is out-dated. https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:26: // data can not exceed 2MB characters. "2MB characters" doesn't make sense to me. We should just say "2MB". https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:52: optional AdditionalDataItem[] additional_items, jsStyle
https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:105: additonal_items_.push_back(std::move(data_item)); On 2017/05/17 16:08:43, Devlin (catching up) wrote: > If we're just going to move all of these anyway, why not just pass the vector by > value and move it in? Unless we want to allow null, in which case we should > just use swap(). Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js (right): https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:15: chrome.test.assertEq(expectedError, chrome.runtime.lastError); On 2017/05/17 16:08:43, Devlin (catching up) wrote: > instead of this, which requires callers to pass in {message: <value>}, let's do: > if (expectedError) > chrome.test.assertLastError(expectedError); Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:19: function testSetImageDataClipboardWithAdditionalData( On 2017/05/17 16:08:43, Devlin (catching up) wrote: > Seems like we can combine this with testSetImageDataClipboard pretty easily, > since we can just not pass the additional parameters if we don't need them (they > default to undefined). Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:20: imageUrl, imageType, additional_items, expectedError) { On 2017/05/17 16:08:44, Devlin (catching up) wrote: > additional_items: useJsStyle :) Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:30: if (additional_items == null) { On 2017/05/17 16:08:43, Devlin (catching up) wrote: > why not just if (additionalItems)? Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:51: baseUrl + '/icon1.png', 'png', undefined); On 2017/05/17 16:08:43, Devlin (catching up) wrote: > no need to pass undefined Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:61: 'message': 'Image data decoding failed.' On 2017/05/17 16:08:44, Devlin (catching up) wrote: > no need for quotes around keys, but moot with other suggestion. Done. https://codereview.chromium.org/2837983002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js:83: function testSavePngImageWithAdditionalDataToClipboardDuplicateTypeItems(baseUrl) { On 2017/05/17 16:08:44, Devlin (catching up) wrote: > lines <= 80 char Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:59: // Fill in the omitted additiona data items with empty data. On 2017/05/17 16:08:44, Devlin (catching up) wrote: > typo: additional Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:60: if (!params->additional_items.get()) On 2017/05/17 16:08:44, Devlin (catching up) wrote: > no need for .get() Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:64: return RespondNow(Error("Unsupported additional_items parameter data.")); On 2017/05/17 16:08:44, Devlin (catching up) wrote: > additionalItems Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:91: if (item.type == clipboard::DATA_ITEM_TYPE_TEXT_PLAIN) { On 2017/05/17 16:08:44, Devlin (catching up) wrote: > use a switch statement Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:15: enum DataItemType {text_plain, text_html}; On 2017/05/17 16:08:44, Devlin (catching up) wrote: > prefer jsCasing Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:20: // MIME type of the additional data item, supported types are: On 2017/05/17 16:08:44, Devlin (catching up) wrote: > "supported types" portion of this comment is out-dated. Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:26: // data can not exceed 2MB characters. On 2017/05/17 16:08:44, Devlin (catching up) wrote: > "2MB characters" doesn't make sense to me. We should just say "2MB". Done. https://codereview.chromium.org/2837983002/diff/40001/extensions/common/api/c... extensions/common/api/clipboard.idl:52: optional AdditionalDataItem[] additional_items, On 2017/05/17 16:08:44, Devlin (catching up) wrote: > jsStyle Done.
(Let me know if there's something specific you'd like me to review as well; I didn't look at the CL itself) https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/05/17 16:08:43, Devlin (catching up) wrote: > On 2017/05/16 18:22:03, jennyz wrote: > > On 2017/04/26 18:20:15, Devlin wrote: > > > Any update on this? > > > > I will ask dcheng@ for update, he is OOO until May 2nd. > > Past May 2 :) garykac@ is working on landing the IDL changes for it now and it's in the implement phase. It will still have to go through the shipping intent. garykac@ will have the full details =)
On 2017/05/19 13:05:09, dcheng (in AEST) wrote: > (Let me know if there's something specific you'd like me to review as well; I > didn't look at the CL itself) > > https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... > File extensions/common/api/clipboard.idl (right): > > https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... > extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is > available, which could be in 2017 Q2. > On 2017/05/17 16:08:43, Devlin (catching up) wrote: > > On 2017/05/16 18:22:03, jennyz wrote: > > > On 2017/04/26 18:20:15, Devlin wrote: > > > > Any update on this? > > > > > > I will ask dcheng@ for update, he is OOO until May 2nd. > > > > Past May 2 :) > > garykac@ is working on landing the IDL changes for it now and it's in the > implement phase. It will still have to go through the shipping intent. garykac@ > will have the full details =) Other than that, how about the rest part of the code?
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/05/19 13:05:09, dcheng (in AEST) wrote: > On 2017/05/17 16:08:43, Devlin (catching up) wrote: > > On 2017/05/16 18:22:03, jennyz wrote: > > > On 2017/04/26 18:20:15, Devlin wrote: > > > > Any update on this? > > > > > > I will ask dcheng@ for update, he is OOO until May 2nd. > > > > Past May 2 :) > > garykac@ is working on landing the IDL changes for it now and it's in the > implement phase. It will still have to go through the shipping intent. garykac@ > will have the full details =) I will sync with garykac@, thanks!
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/05/19 13:05:09, dcheng wrote: > On 2017/05/17 16:08:43, Devlin (catching up) wrote: > > On 2017/05/16 18:22:03, jennyz wrote: > > > On 2017/04/26 18:20:15, Devlin wrote: > > > > Any update on this? > > > > > > I will ask dcheng@ for update, he is OOO until May 2nd. > > > > Past May 2 :) > > garykac@ is working on landing the IDL changes for it now and it's in the > implement phase. It will still have to go through the shipping intent. garykac@ > will have the full details =) Done. https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipb... extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could be in 2017 Q2. On 2017/04/26 18:20:15, Devlin (OOO Monday 22.May) wrote: > Any update on this? Done.
https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:68: params->image_data, params->type, std::move(*(params->additional_items)), Won't this crash if additional_items is null? https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.h (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.h:15: using AdditionalDataItemList = std::vector<api::clipboard::AdditionalDataItem>; iwyu <vector> https://codereview.chromium.org/2837983002/diff/80001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/common/api/c... extensions/common/api/clipboard.idl:21: // "textPlain" and "textHtml". We don't need to specify the supported types, since they will be documented through DataItemType.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.cc:68: params->image_data, params->type, std::move(*(params->additional_items)), On 2017/05/23 17:04:47, Devlin (catching up) wrote: > Won't this crash if additional_items is null? additional_items is filled in at line 61 if it is null. https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... File extensions/browser/api/clipboard/clipboard_api.h (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/... extensions/browser/api/clipboard/clipboard_api.h:15: using AdditionalDataItemList = std::vector<api::clipboard::AdditionalDataItem>; On 2017/05/23 17:04:47, Devlin (catching up) wrote: > iwyu <vector> Done. https://codereview.chromium.org/2837983002/diff/80001/extensions/common/api/c... File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/common/api/c... extensions/common/api/clipboard.idl:21: // "textPlain" and "textHtml". On 2017/05/23 17:04:47, Devlin (catching up) wrote: > We don't need to specify the supported types, since they will be documented > through DataItemType. Done.
lgtm https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:61: params->additional_items.reset(new AdditionalDataItemList()); prefer base::MakeUnique<> https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:63: if (!IsAdditionalItemsParamValid(*(params->additional_items))) { no need for parens around `params->additional_items` https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:68: params->image_data, params->type, std::move(*(params->additional_items)), ditto re parens
https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:61: params->additional_items.reset(new AdditionalDataItemList()); On 2017/05/24 21:25:11, Devlin (catching up) wrote: > prefer base::MakeUnique<> Done. https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:63: if (!IsAdditionalItemsParamValid(*(params->additional_items))) { On 2017/05/24 21:25:12, Devlin (catching up) wrote: > no need for parens around `params->additional_items` Done. https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api... extensions/browser/api/clipboard/clipboard_api.cc:68: params->image_data, params->type, std::move(*(params->additional_items)), On 2017/05/24 21:25:11, Devlin (catching up) wrote: > ditto re parens Done.
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 Link to the patchset: https://codereview.chromium.org/2837983002/#ps140001 (title: "Nits and rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jennyz@chromium.org
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": 140001, "attempt_start_ts": 1495732083681710, "parent_rev": "3ef0c4a62c3bbc2cea73b2821fa6637570c94888", "commit_rev": "0d792f0d848e8cc25e568cc4d376b482115c149b"}
Message was sent while issue was closed.
Description was changed from ========== Modify SetImageData to add additional items along with image data to save on clipboard. BUG=712874 ========== to ========== Modify SetImageData to add additional items along with image data to save on clipboard. BUG=712874 Review-Url: https://codereview.chromium.org/2837983002 Cr-Commit-Position: refs/heads/master@{#474701} Committed: https://chromium.googlesource.com/chromium/src/+/0d792f0d848e8cc25e568cc4d376... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0d792f0d848e8cc25e568cc4d376... |