Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1114213004:
Remove NOTIMPLEMENTED() logspam on Android clipboard actions (Closed)
|
Created:
5 years, 7 months ago by Tobias Sargeant Modified:
5 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove NOTIMPLEMENTED() logspam. Provide an implementation that
mirrors what org/chromium/ui/base/Clipboard.java can currently
provide.
Also add testing for ReadAvaliableTypes() and correct bounds error
on Android with UnicodeHTMLTest.
BUG=447167
Committed: https://crrev.com/868100a8a0ffe3506b4adcf7b21e5e6c06fa84ce
Cr-Commit-Position: refs/heads/master@{#339879}
Patch Set 1 #Patch Set 2 : Implement ReadAvailableTypes for the test clipboard too. #Patch Set 3 : rebase #Patch Set 4 : #Patch Set 5 : clean up tests #
Total comments: 15
Patch Set 6 : address comments #Patch Set 7 : #
Total comments: 1
Patch Set 8 : nits #Patch Set 9 : Make substring operation safer in case of overflow #Patch Set 10 : #
Messages
Total messages: 28 (9 generated)
tobiasjs@chromium.org changed reviewers: + dcheng@chromium.org
Could you please take a look? Thanks.
https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_test_template.h (right): https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:67: bool AvailableTypesIsEmpty(ClipboardType type = CLIPBOARD_TYPE_COPY_PASTE) { Default arguments are not permitted in Chromium. https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:74: return types.size() == 0; types.empty() https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:78: const std::set<base::string16> &expected) { & by the type. For style issues like this, git cl format is quite helpful. https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:86: for (auto it = expected.begin(); it != expected.end(); ++it) { This is copying each string16. Use const auto& https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:95: bool AvailableTypesHas(const char* _1, It seems to me that the code would be clearer if you just used gmock matchers? EXPECT_THAT(types, Contains(Eq(Clipboard::kMimeTypeHTML))) etc https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:310: std::min(fragment_end, static_cast<uint32>(markup.size())), Can you explain to me why we need to use fragment_end here sometimes? This doesn't seem right. https://codereview.chromium.org/1114213004/diff/80001/ui/base/test/test_clipb... File ui/base/test/test_clipboard.cc (right): https://codereview.chromium.org/1114213004/diff/80001/ui/base/test/test_clipb... ui/base/test/test_clipboard.cc:46: return; Why add this check? Both |types| and |contains_filenames| should never be null.
Also, note that the changes you made probably aren't getting tested. The clipboard unit tests haven't run on Android in a very long time. https://codereview.chromium.org/1027993002 was my attempt to fix it, but it got bogged down in the proper way to implement it: jam@ didn't want SERIALIZED_ to show up in test output, etc, but it turns out that getting that to all work is non-trivial. If this is something you are willing to adopt, it'd be very valuable: Android support of the clipboard has gotten pretty busted due to the lack of test coverage.
On 2015/07/15 01:08:15, dcheng wrote: > Also, note that the changes you made probably aren't getting tested. The > clipboard unit tests haven't run on Android in a very long time. I'm aware of that. I'm reasonably confident to say that the non-test-based changes in this CL are correct based upon comparison with implementations for other platforms, but it would be nice to fix the android clipboard test situation. > https://codereview.chromium.org/1027993002 was my attempt to fix it, but it got > bogged down in the proper way to implement it: jam@ didn't want SERIALIZED_ to > show up in test output, etc, but it turns out that getting that to all work is > non-trivial. If this is something you are willing to adopt, it'd be very > valuable: Android support of the clipboard has gotten pretty busted due to the > lack of test coverage. I can take a look. I can't promise I'll have time. Maybe assign the bug to me, and we'll see how it goes? https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_test_template.h (right): https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:67: bool AvailableTypesIsEmpty(ClipboardType type = CLIPBOARD_TYPE_COPY_PASTE) { On 2015/07/15 01:04:45, dcheng wrote: > Default arguments are not permitted in Chromium. Done. https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:74: return types.size() == 0; On 2015/07/15 01:04:45, dcheng wrote: > types.empty() removed https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:78: const std::set<base::string16> &expected) { On 2015/07/15 01:04:45, dcheng wrote: > & by the type. For style issues like this, git cl format is quite helpful. Thanks for pointing this out. I did this soon after joining, before I'd adjusted to the chromium style / learnt about clang-format and friends. https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:86: for (auto it = expected.begin(); it != expected.end(); ++it) { On 2015/07/15 01:04:45, dcheng wrote: > This is copying each string16. Use const auto& removed (I admit it was lazy, but it didn't seem like it should matter in a test). https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:95: bool AvailableTypesHas(const char* _1, On 2015/07/15 01:04:45, dcheng wrote: > It seems to me that the code would be clearer if you just used gmock matchers? > > EXPECT_THAT(types, Contains(Eq(Clipboard::kMimeTypeHTML))) > > etc I've switched everything to using this scheme. Thanks for pointing it out. https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:310: std::min(fragment_end, static_cast<uint32>(markup.size())), On 2015/07/15 01:04:45, dcheng wrote: > Can you explain to me why we need to use fragment_end here sometimes? This > doesn't seem right. Again, It has been a while, and I can't remember the details. I think that what I saw was that in a failing case, fragment_start and fragment_end were 0 (possibly when I forced this to run on android) and that lead fragment_end - markup.size() to be a large value, which caused the test to crash. I think what I've replaced it with now is clearer than both the preceding version and what's here. What do you think? https://codereview.chromium.org/1114213004/diff/80001/ui/base/test/test_clipb... File ui/base/test/test_clipboard.cc (right): https://codereview.chromium.org/1114213004/diff/80001/ui/base/test/test_clipb... ui/base/test/test_clipboard.cc:46: return; On 2015/07/15 01:04:46, dcheng wrote: > Why add this check? Both |types| and |contains_filenames| should never be null. It was a copy from (for example) clipboard_android.cc. I've removed it.
lgtm with a nit https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_test_template.h (right): https://codereview.chromium.org/1114213004/diff/80001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_test_template.h:310: std::min(fragment_end, static_cast<uint32>(markup.size())), On 2015/07/15 at 14:04:00, Tobias Sargeant wrote: > On 2015/07/15 01:04:45, dcheng wrote: > > Can you explain to me why we need to use fragment_end here sometimes? This > > doesn't seem right. > > Again, It has been a while, and I can't remember the details. I think that what I saw was that in a failing case, fragment_start and fragment_end were 0 (possibly when I forced this to run on android) and that lead fragment_end - markup.size() to be a large value, which caused the test to crash. > > I think what I've replaced it with now is clearer than both the preceding version and what's here. What do you think? As long as it passes the trybots. https://codereview.chromium.org/1114213004/diff/120001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_test_template.h (right): https://codereview.chromium.org/1114213004/diff/120001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_test_template.h:119: Contains(base::string16(ASCIIToUTF16(Clipboard::kMimeTypeText)))); Here and elsewhere, the explicit invocation of base::string16's constructor should be unnecessary.
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1114213004/#ps140001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114213004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> As long as it passes the trybots. Interestingly, on linux it clearly doesn't. It looks like windows is the only platform that uses fragment_start and fragment_end, however it's not clear to me whether the <meta http-equiv="content-type" content="text/html; charset=utf-8"> that the linux html clipboard prepends should be considered part of the HTML, or part of the context required to parse the HTML (as mentioned to in the ReadHTML comment - https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...). I don't know whether the test is now wrong, or the linux clipboard implementation should set a non-zero fragment_start to trim off the meta tag. If the test is wrong, then should it be testing whether markup is a substring of the fragment-trimmed result? And what about case changes?
On 2015/07/16 at 10:57:02, tobiasjs wrote: > > As long as it passes the trybots. > > Interestingly, on linux it clearly doesn't. > > It looks like windows is the only platform that uses fragment_start and fragment_end, however it's not clear to me whether the <meta http-equiv="content-type" content="text/html; charset=utf-8"> that the linux html clipboard prepends should be considered part of the HTML, or part of the context required to parse the HTML (as mentioned to in the ReadHTML comment - https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/clipboard/...). > > I don't know whether the test is now wrong, or the linux clipboard implementation should set a non-zero fragment_start to trim off the meta tag. If the test is wrong, then should it be testing whether markup is a substring of the fragment-trimmed result? And what about case changes? Ah, these failures reminded me why I wrote it this way. On Windows, we happen to have fragment start / end indices, because it's part of the platform-specific CF_HTML format. No other platform has it, so by convention, the fragment indices get set to [0, length of clipboard markup). However, Mac/Linux don't have this, but have random things prefixed. That's the reason that we calculate the indices for the test more cleverly: fragment_end should always be in bounds, and fragment_end - size of the markup we passed in should point to the start of the HTML content. This should work, even on Android.
I've reverted now, but changed the preceding EXPECT_LE to ASSERT_LE so that if fragment_end isn't <= markup.size() it won't continue (and crash).
On 2015/07/21 at 15:06:35, tobiasjs wrote: > I've reverted now, but changed the preceding EXPECT_LE to ASSERT_LE so that if fragment_end isn't <= markup.size() it won't continue (and crash). Which platform is this failing on again? ClipboardAndroid::ReadHTML has this: *fragment_start = 0; *fragment_end = static_cast<uint32>(markup->length()); Which I would expect to be sufficient to not crash.
On 2015/07/21 19:48:55, dcheng wrote: > On 2015/07/21 at 15:06:35, tobiasjs wrote: > > I've reverted now, but changed the preceding EXPECT_LE to ASSERT_LE so that if > fragment_end isn't <= markup.size() it won't continue (and crash). > > Which platform is this failing on again? ClipboardAndroid::ReadHTML has this: > *fragment_start = 0; > *fragment_end = static_cast<uint32>(markup->length()); > > Which I would expect to be sufficient to not crash. At this point, I can't remember where it was that I saw it. I've put it back the way it was, and I'll commit that. If it's actually an issue, I'm sure I'll run into it again.
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1114213004/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114213004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tobiasjs@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
sky@ or sadrul@, sorry for the noise, but could one of you please take a look at ui/base/test/test_clipboard.cc changes? I'm not sure who a more appropriate owner for that file could be. Thanks.
LGTM
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114213004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/868100a8a0ffe3506b4adcf7b21e5e6c06fa84ce Cr-Commit-Position: refs/heads/master@{#339879} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
