Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(274)

Issue 2837983002: Modify SetImageData to add additional items along with image data to save on clipboard. (Closed)

Created:
3 years, 8 months ago by jennyz
Modified:
3 years, 7 months ago
Reviewers:
Devlin, dcheng
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, dcheng, extensions-reviews_chromium.org, garykac
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -17 lines) Patch
M chrome/browser/extensions/api/chrome_extensions_api_client.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/chrome_extensions_api_client.cc View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/clipboard_extension_helper_chromeos.h View 1 2 3 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/clipboard_extension_helper_chromeos.cc View 1 2 5 chunks +15 lines, -2 lines 0 comments Download
M chrome/test/data/extensions/api_test/clipboard/set_image_data/test.js View 1 2 4 chunks +63 lines, -10 lines 0 comments Download
M extensions/browser/api/clipboard/clipboard_api.h View 1 2 3 4 3 chunks +7 lines, -0 lines 0 comments Download
M extensions/browser/api/clipboard/clipboard_api.cc View 1 2 3 4 5 3 chunks +40 lines, -1 line 0 comments Download
M extensions/browser/api/extensions_api_client.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M extensions/browser/api/extensions_api_client.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M extensions/common/api/clipboard.idl View 1 2 3 4 2 chunks +19 lines, -3 lines 0 comments Download

Messages

Total messages: 29 (11 generated)
jennyz
3 years, 8 months ago (2017-04-24 22:14:05 UTC) #3
Devlin
https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc#newcode127 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 ...
3 years, 8 months ago (2017-04-26 18:20:15 UTC) #5
dcheng
(left a few drive-by thoughts as well) https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/1/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc#newcode127 chrome/browser/extensions/clipboard_extension_helper_chromeos.cc:127: scw.WriteHTML(base::UTF8ToUTF16(item.data), std::string("")); ...
3 years, 7 months ago (2017-04-27 15:42:09 UTC) #6
jennyz
Sorry for the late delay for addressing the review feedback, I was pulled off by ...
3 years, 7 months ago (2017-05-16 18:22:03 UTC) #8
Devlin
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl#newcode8 extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could ...
3 years, 7 months ago (2017-05-17 16:08:44 UTC) #9
jennyz
https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc File chrome/browser/extensions/clipboard_extension_helper_chromeos.cc (right): https://codereview.chromium.org/2837983002/diff/40001/chrome/browser/extensions/clipboard_extension_helper_chromeos.cc#newcode105 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: > ...
3 years, 7 months ago (2017-05-18 23:33:49 UTC) #10
dcheng
(Let me know if there's something specific you'd like me to review as well; I ...
3 years, 7 months ago (2017-05-19 13:05:09 UTC) #11
jennyz
On 2017/05/19 13:05:09, dcheng (in AEST) wrote: > (Let me know if there's something specific ...
3 years, 7 months ago (2017-05-19 17:10:30 UTC) #12
jennyz
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl#newcode8 extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could ...
3 years, 7 months ago (2017-05-19 22:53:13 UTC) #13
jennyz
https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl File extensions/common/api/clipboard.idl (right): https://codereview.chromium.org/2837983002/diff/1/extensions/common/api/clipboard.idl#newcode8 extensions/common/api/clipboard.idl:8: // deprecated once open-web solution is available, which could ...
3 years, 7 months ago (2017-05-22 23:59:10 UTC) #14
Devlin
https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/clipboard/clipboard_api.cc File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/clipboard/clipboard_api.cc#newcode68 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 ...
3 years, 7 months ago (2017-05-23 17:04:47 UTC) #15
jennyz
https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/clipboard/clipboard_api.cc File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/80001/extensions/browser/api/clipboard/clipboard_api.cc#newcode68 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) ...
3 years, 7 months ago (2017-05-23 21:36:39 UTC) #17
Devlin
lgtm https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api/clipboard/clipboard_api.cc File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api/clipboard/clipboard_api.cc#newcode61 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/clipboard/clipboard_api.cc#newcode63 extensions/browser/api/clipboard/clipboard_api.cc:63: if (!IsAdditionalItemsParamValid(*(params->additional_items))) ...
3 years, 7 months ago (2017-05-24 21:25:12 UTC) #18
jennyz
https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api/clipboard/clipboard_api.cc File extensions/browser/api/clipboard/clipboard_api.cc (right): https://codereview.chromium.org/2837983002/diff/120001/extensions/browser/api/clipboard/clipboard_api.cc#newcode61 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: ...
3 years, 7 months ago (2017-05-24 23:21:27 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2837983002/140001
3 years, 7 months ago (2017-05-24 23:22:39 UTC) #22
commit-bot: I haz the power
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_android_rel_ng/builds/302674)
3 years, 7 months ago (2017-05-25 03:17:54 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2837983002/140001
3 years, 7 months ago (2017-05-25 17:08:21 UTC) #26
commit-bot: I haz the power
3 years, 7 months ago (2017-05-25 17:59:43 UTC) #29
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0d792f0d848e8cc25e568cc4d376...

Powered by Google App Engine
This is Rietveld 408576698