Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790993003:
Add Generic Implementation of ClipboardRecentContent (Closed)
DescriptionAdd Generic Implementation of ClipboardRecentContent
i.e., an implementation that works on all platforms for which
ui/base/Clipboard
has an implementation. This is all of them except iOS.
The only platform for which the implementation is sufficient at
this time to provide the necessary functionality is Android.
(The other platforms always return clipboard modification times of
the start of epoch, meaning any URLs in the clipboard will not be
suggested.)
I have tested this interactively and it works.
TBR=sky
(for the modifications to ui/base/test/test_clipboard.{h,cc}
and the inclusion of ui/base/test in components/open_from_clipboard/DEPS )
BUG=682446
Review-Url: https://codereview.chromium.org/2790993003
Cr-Commit-Position: refs/heads/master@{#461631}
Committed: https://chromium.googlesource.com/chromium/src/+/054a0eadff47f8d158a2314e3858fb050ada6a4f
Patch Set 1 #Patch Set 2 : slight polish #Patch Set 3 : cleanup #Patch Set 4 : blank line #
Total comments: 6
Patch Set 5 : add detailed unit test, minor implementation modifications, pkasting's comments #
Total comments: 12
Patch Set 6 : use ui/base/test/test_clipboard #
Total comments: 4
Patch Set 7 : suppress suggestion regardless of history_service existing #
Total comments: 4
Patch Set 8 : revise BUILD files #Patch Set 9 : guard deletion call #Patch Set 10 : revise build file #Patch Set 11 : exclude generic files on iOS build #Patch Set 12 : guard generic instantiation on iOS #Patch Set 13 : constexpr #Patch Set 14 : make maxage non-static, plus guard recent content for tests #
Total comments: 2
Patch Set 15 : remember to clear the global instance when it's deleted. #Patch Set 16 : tested interactively; works. removed field trial force-enable code #
Total comments: 2
Messages
Total messages: 76 (51 generated)
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on * remove all the LOG(INFO) lines * resolve presubmit issues with illegal includes BUG=682446 ==========
mpearson@chromium.org changed reviewers: + jif@chromium.org
jif@, Can you take a look at this first draft of this changelist? It's mostly complete, just needs some polish that I'll get to soon. I'm hoping to submit it early next week so it can go live in next week's dev push. thanks, mark
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on * remove all the LOG(INFO) lines * resolve presubmit issues with illegal includes BUG=682446 ==========
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on * remove all the LOG(INFO) lines * resolve presubmit issues with illegal includes BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ==========
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ==========
mpearson@chromium.org changed reviewers: + msramek@chromium.org, pkasting@chromium.org, tedchoc@chromium.org
jif@ is the primary reviewer of this changelist, but I need certain secondary
reviewers for particular files:
msramek@ - please review chrome_browsing_data_remover_delegate.cc
tedchoc@ - please review autocomplete_controller_android.cc
pkasting@ - please review autocomplete_controller.{h,cc}
thanks all,
mark
https://codereview.chromium.org/2790993003/diff/60001/chrome/browser/android/...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):
https://codereview.chromium.org/2790993003/diff/60001/chrome/browser/android/...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:231: if
(autocomplete_controller_->result().match_at(selected_index).type ==
For context, this call should be analogous to this iOS call.
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_po...
https://codereview.chromium.org/2790993003/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):
https://codereview.chromium.org/2790993003/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:426:
ClipboardRecentContent::GetInstance()->SuppressClipboardContent();
For context, I'm trying to make a call analogous to this iOS call:
https://cs.chromium.org/chromium/src/ios/chrome/browser/browsing_data/browsin...
mpearson@chromium.org changed reviewers: + dcheng@chromium.org
Oh, and +dcheng@ for adding ui/base/clipboard to components/open_from_clipboard/DEPS. Please review that. thanks, mark
LGTM https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:240: clipboard_recent_content_.reset(new ClipboardRecentContentGeneric()); Nit: Prefer =MakeUnique to reset(new https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:244: if (clipboard_recent_content) { It seems like this can't fail now and can be removed. Nit: In which case we could eliminate the temp because it's more trouble than it's worth: if (!ClipboardRecentContent::GetInstance()) { ... } ...use ClipboardRecentContent::GetInstance() directly
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * finish writing unit test * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ==========
The DEP on //ui/base/clipboard LGTM Had a few random comments though. https://codereview.chromium.org/2790993003/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:52: //#endif Nit: remove the commented-out code https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:24: const base::TimeDelta ClipboardRecentContent::kMaximumAgeOfClipboard = Does this generate a static initializer? (The constructor is constexpr, but I'm not sure what happens since the actual static variable isn't marked constexpr) https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:44: for (size_t i = 0; i < arraysize(kAuthorizedSchemes); ++i) { Alternatively: for (const auto* authorized_scheme : kAuthorizedSchemes) { if (url.SchemeIs(authorized_scheme)) return true; } https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_generic.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_generic.cc:14: base::Time last_modified_time = clipboard->GetClipboardLastModifiedTime(); Btw, I'm wondering if we should have made this return a TimeTicks. I wonder if this might interact strangely across DST changes. I guess there's also a question of how to handle a sleeping device (where the behavior of TimeTicks is implementation defined, and may or may not actually advance) Edit: I guess we try to handle this on line 54, but I'm curious if we should be using TimeTicks here anyway. https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_generic_unittest.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_generic_unittest.cc:20: class MockClipboard : public ui::Clipboard { Does ui::TestClipboard help with this?
mpearson@chromium.org changed reviewers: + sky@chromium.org
+sky for the modifications to
ui/base/test/test_clipboard.{h,cc}
and the inclusion of ui/base/test in components/open_from_clipboard/DEPS
thanks all,
mark
https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_controller.cc (right):
https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_controller.cc:240:
clipboard_recent_content_.reset(new ClipboardRecentContentGeneric());
On 2017/04/01 05:54:28, Peter Kasting wrote:
> Nit: Prefer =MakeUnique to reset(new
Done.
https://codereview.chromium.org/2790993003/diff/60001/components/omnibox/brow...
components/omnibox/browser/autocomplete_controller.cc:244: if
(clipboard_recent_content) {
On 2017/04/01 05:54:28, Peter Kasting wrote:
> It seems like this can't fail now and can be removed.
>
> Nit: In which case we could eliminate the temp because it's more trouble than
> it's worth:
>
> if (!ClipboardRecentContent::GetInstance()) {
> ...
> }
> ...use ClipboardRecentContent::GetInstance() directly
Yeah, I agree that it's more trouble than it's worth. Done. Thanks for
pointing out the obvious above (that the creation can't fail), which thus makes
it easy to use ClipboardRecentContent::GetInstance() where needed. (Otherwise
the repeated ClipboardRecentContent::GetInstance() looked like a pain.)
https://codereview.chromium.org/2790993003/diff/80001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.cc (right):
https://codereview.chromium.org/2790993003/diff/80001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:52: //#endif
On 2017/04/02 05:04:03, dcheng wrote:
> Nit: remove the commented-out code
Acknowledged. It's a TODO in the changelist description. Thanks for the
reminder.
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content.cc (right):
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content.cc:24: const
base::TimeDelta ClipboardRecentContent::kMaximumAgeOfClipboard =
On 2017/04/02 05:04:03, dcheng wrote:
> Does this generate a static initializer? (The constructor is constexpr, but
I'm
> not sure what happens since the actual static variable isn't marked constexpr)
Changed to constexpr.
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content.cc:44: for (size_t i =
0; i < arraysize(kAuthorizedSchemes); ++i) {
On 2017/04/02 05:04:03, dcheng wrote:
> Alternatively:
>
> for (const auto* authorized_scheme : kAuthorizedSchemes) {
> if (url.SchemeIs(authorized_scheme)) return true;
> }
Nice. Done.
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_generic.cc (right):
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_generic.cc:14:
base::Time last_modified_time = clipboard->GetClipboardLastModifiedTime();
On 2017/04/02 05:04:03, dcheng wrote:
> Btw, I'm wondering if we should have made this return a TimeTicks. I wonder if
> this might interact strangely across DST changes. I guess there's also a
> question of how to handle a sleeping device (where the behavior of TimeTicks
is
> implementation defined, and may or may not actually advance)
>
> Edit: I guess we try to handle this on line 54, but I'm curious if we should
be
> using TimeTicks here anyway.
I consciously didn't use TimeTicks. It makes a lot of different when the user
put something in his clipboard, put the phone away for two weeks, then reopened
it and saw a suggestion from the clipboard because the timeticks was suspended
for that time. That's not okay. We need to use user-visible clock time to
reflect whether this URL was recently on the user's mind recently, i.e., real
time.
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_generic_unittest.cc
(right):
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_generic_unittest.cc:20:
class MockClipboard : public ui::Clipboard {
On 2017/04/02 05:04:03, dcheng wrote:
> Does ui::TestClipboard help with this?
Yes, a ton. Removed basically all the code and instead added two functions to
that class.
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/android...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/android...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:233:
UMA_HISTOGRAM_LONG_TIMES_100(
For context, this call should be analogous to this iOS call.
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_po...
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:426:
ClipboardRecentContent::GetInstance()->SuppressClipboardContent();
For context, I'm trying to make a call analogous to this iOS call:
https://cs.chromium.org/chromium/src/ios/chrome/browser/browsing_data/browsin...
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_generic.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_generic.cc:14: base::Time last_modified_time = clipboard->GetClipboardLastModifiedTime(); On 2017/04/02 05:33:56, Mark P wrote: > On 2017/04/02 05:04:03, dcheng wrote: > > Btw, I'm wondering if we should have made this return a TimeTicks. I wonder if > > this might interact strangely across DST changes. I guess there's also a > > question of how to handle a sleeping device (where the behavior of TimeTicks > is > > implementation defined, and may or may not actually advance) > > > > Edit: I guess we try to handle this on line 54, but I'm curious if we should > be > > using TimeTicks here anyway. > > I consciously didn't use TimeTicks. It makes a lot of different when the user > put something in his clipboard, put the phone away for two weeks, then reopened > it and saw a suggestion from the clipboard because the timeticks was suspended > for that time. That's not okay. We need to use user-visible clock time to > reflect whether this URL was recently on the user's mind recently, i.e., real > time. Are you sure TimeTicks won't advance when the phone has the screen off? I thought TimeTicks basically followed wall-clock time advancement, but increased monotonically and started counting on Chrome startup. In which case it should still advance here.
https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content_generic.cc (right): https://codereview.chromium.org/2790993003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content_generic.cc:14: base::Time last_modified_time = clipboard->GetClipboardLastModifiedTime(); On 2017/04/03 01:42:13, Peter Kasting wrote: > On 2017/04/02 05:33:56, Mark P wrote: > > On 2017/04/02 05:04:03, dcheng wrote: > > > Btw, I'm wondering if we should have made this return a TimeTicks. I wonder > if > > > this might interact strangely across DST changes. I guess there's also a > > > question of how to handle a sleeping device (where the behavior of TimeTicks > > is > > > implementation defined, and may or may not actually advance) > > > > > > Edit: I guess we try to handle this on line 54, but I'm curious if we should > > be > > > using TimeTicks here anyway. > > > > I consciously didn't use TimeTicks. It makes a lot of different when the user > > put something in his clipboard, put the phone away for two weeks, then > reopened > > it and saw a suggestion from the clipboard because the timeticks was suspended > > for that time. That's not okay. We need to use user-visible clock time to > > reflect whether this URL was recently on the user's mind recently, i.e., real > > time. > > Are you sure TimeTicks won't advance when the phone has the screen off? > > I thought TimeTicks basically followed wall-clock time advancement, but > increased monotonically and started counting on Chrome startup. In which case > it should still advance here. We also save the time (and last clipboard value) in prefs so we can estimate when Chrome loads how old the clipboard is. (If the clipboard info stored in prefs is the same as the current clipboard, the time is still valid.) For this to work, we need a wall time, not a time that's only valid on a particular run of Chrome.
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc (right): https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:426: ClipboardRecentContent::GetInstance()->SuppressClipboardContent(); On 2017/04/02 05:33:56, Mark P wrote: > For context, I'm trying to make a call analogous to this iOS call: > https://cs.chromium.org/chromium/src/ios/chrome/browser/browsing_data/browsin... > I'd prefer not putting this into "if (history_service)" unless the next line is actually "history_service->...". History service might not exist for various reasons. One non-hypothetical one is that history saving is disabled by the administator. If the clipboard data still *is* saved, then it still makes sense to delete it, regardless of what happens to the actual browsing history. Regarding Incognito, it's typically fine if BrowsingDataRemover doesn't delete Incognito profile data that would be deleted by closing Incognito. Is that the case here?
LGTM
On 2017/04/03 16:06:34, jif wrote: > LGTM I absolutely reviewed the CL :-) For instance I noticed some comments that were not cleaned, but I thought that it was a draft.
Current status of reviewers:
* msramek@ review in progress for chrome_browsing_data_remover_delegate.cc
* awaiting sky@ for the modifications to ui/base/test/test_clipboard.{h,cc} and
the inclusion of ui/base/test in components/open_from_clipboard/DEPS
* awaiting tedchoc@ for autocomplete_controller_android.cc
Completed:
* jif@ primary reviewer for whole changelist
* dcheng@ for adding ui/base/clipboard to
components/open_from_clipboard/DEPS
* pkasting@ for autocomplete_controller.{h,cc}
cheers,
mark
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):
https://codereview.chromium.org/2790993003/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:426:
ClipboardRecentContent::GetInstance()->SuppressClipboardContent();
On 2017/04/03 12:37:55, msramek wrote:
> On 2017/04/02 05:33:56, Mark P wrote:
> > For context, I'm trying to make a call analogous to this iOS call:
> >
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/browsing_data/browsin...
> >
>
> I'd prefer not putting this into "if (history_service)" unless the next line
is
> actually "history_service->...".
>
> History service might not exist for various reasons. One non-hypothetical one
is
> that history saving is disabled by the administator. If the clipboard data
still
> *is* saved, then it still makes sense to delete it, regardless of what happens
> to the actual browsing history.
>
> Regarding Incognito, it's typically fine if BrowsingDataRemover doesn't delete
> Incognito profile data that would be deleted by closing Incognito. Is that the
> case here?
Yes, I believe so. I think the source of my question comes from not
understanding the iOS code. I don't understand how/why BrowsingDataRemover
could be called on an incognito profile. But it cannot hurt to suppress the
suggestion regardless.
Did as you suggested. (Thanks for your explanation by the way. I never knew
admins could disable saving history.)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... components/omnibox/browser/omnibox_field_trial.cc:48: // #if defined(OS_IOS) Is this here to stay?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I have one more question :) https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc (right): https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:424: ClipboardRecentContent::GetInstance()->SuppressClipboardContent(); Are we sure that this exists? I can't find any callsite that would guaranteedly create this soon enough. Should we DCHECK (probably not), wrap it in "if (GetInstance())", or make BrowsingDataRemoverFactory depend on something?
https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc (right): https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:424: ClipboardRecentContent::GetInstance()->SuppressClipboardContent(); On 2017/04/03 17:44:56, msramek wrote: > Are we sure that this exists? I can't find any callsite that would guaranteedly > create this soon enough. Yes, it should exist. It gets created when the omnibox is created, which should happen as part of startup. That said ... > Should we DCHECK (probably not), wrap it in "if (GetInstance())", or make > BrowsingDataRemoverFactory depend on something? ... there's no reason not to guard it. If for some reason we decided not to create it, there's no harm in skipping the Suppress call. After all, if it's not created, there's nothing to suppress. https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... components/omnibox/browser/omnibox_field_trial.cc:48: // #if defined(OS_IOS) On 2017/04/03 16:32:02, jif wrote: > Is this here to stay? No, this will be removed when I'm done with interactive testing.
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/04/03 18:04:45, Mark P wrote: > https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... > File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc > (right): > > https://codereview.chromium.org/2790993003/diff/120001/chrome/browser/browsin... > chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:424: > ClipboardRecentContent::GetInstance()->SuppressClipboardContent(); > On 2017/04/03 17:44:56, msramek wrote: > > Are we sure that this exists? I can't find any callsite that would > guaranteedly > > create this soon enough. > > Yes, it should exist. It gets created when the omnibox is created, which should > happen as part of startup. That said ... > > > Should we DCHECK (probably not), wrap it in "if (GetInstance())", or make > > BrowsingDataRemoverFactory depend on something? > > ... there's no reason not to guard it. If for some reason we decided not to > create it, there's no harm in skipping the Suppress call. After all, if it's > not created, there's nothing to suppress. > > https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... > File components/omnibox/browser/omnibox_field_trial.cc (right): > > https://codereview.chromium.org/2790993003/diff/120001/components/omnibox/bro... > components/omnibox/browser/omnibox_field_trial.cc:48: // #if defined(OS_IOS) > On 2017/04/03 16:32:02, jif wrote: > > Is this here to stay? > > No, this will be removed when I'm done with interactive testing. autocomplete_controller_android.cc - lgtm
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this at a basic level interactively and it works. TODO: * test interactively more thoroughly * revert the change to omnibox_field_trial that forces the clipboard provider on BUG=682446 ========== to ========== Add Generic Implementation of ClipboardRecentContent i.e., an implementation that works on all platforms for which ui/base/Clipboard has an implementation. This is all of them except iOS. The only platform for which the implementation is sufficient at this time to provide the necessary functionality is Android. (The other platforms always return clipboard modification times of the start of epoch, meaning any URLs in the clipboard will not be suggested.) I have tested this interactively and it works. BUG=682446 ==========
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Just about ready to submit! Thank all you reviewers (5+) for a prompt review!
:-)
I tested this thoroughly interactively and it works, as does the logging. I
then removed the code in omnibox_field_trial.cc that turns this on by default.
I made some minor modifications to deal with dependency rules and test-only
failures.
Next steps:
* pkasting@, can please take a look at the changes in autocomplete_controller
and let me know if you have any additional comments?
* jif@, can you please take a look at the minor changes since patch set 7 (which
is when you reviewed it I believe) and let me know if you have additional
comments?
* sky@, please review the modifications to ui/base/test/test_clipboard.{h,cc}
and
the inclusion of ui/base/test in components/open_from_clipboard/DEPS
Given how minor these changes are, if I don't hear from someone by the time the
dry run finishes, I will commit this change and deal any additional comments in
a follow-up CL.
thanks all,
mark
https://codereview.chromium.org/2790993003/diff/260001/components/omnibox/bro... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2790993003/diff/260001/components/omnibox/bro... components/omnibox/browser/autocomplete_controller.cc:35: #if !defined(OS_IOS) Nit: Put #if'd #includes in a separate group below https://codereview.chromium.org/2790993003/diff/300001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2790993003/diff/300001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; I'm still worried about lifetime management here. Let's say we create one AutocompleteController, which creates a clipboard instance and sets it as the default. Then we create a second controller, which does nothing because there's already an instance. Then we destroy the first controller. This destroys the global instance. Now the second controller can remain alive, with no instance, for arbitrary time. It seems like what you really want is either to have the ClipboardRecentContent object be vended by a service factory, or have the one global instance be refcounted, and have AutocompleteController take a scoped_refptr. Also seems like maybe ClipboardRecentContent() should SetInstance() automatically in the constructor if there is no current instance, rather than exposing that publicly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Add Generic Implementation of ClipboardRecentContent
i.e., an implementation that works on all platforms for which
ui/base/Clipboard
has an implementation. This is all of them except iOS.
The only platform for which the implementation is sufficient at
this time to provide the necessary functionality is Android.
(The other platforms always return clipboard modification times of
the start of epoch, meaning any URLs in the clipboard will not be
suggested.)
I have tested this interactively and it works.
BUG=682446
==========
to
==========
Add Generic Implementation of ClipboardRecentContent
i.e., an implementation that works on all platforms for which
ui/base/Clipboard
has an implementation. This is all of them except iOS.
The only platform for which the implementation is sufficient at
this time to provide the necessary functionality is Android.
(The other platforms always return clipboard modification times of
the start of epoch, meaning any URLs in the clipboard will not be
suggested.)
I have tested this interactively and it works.
TBR=sky
(for the modifications to ui/base/test/test_clipboard.{h,cc}
and the inclusion of ui/base/test in components/open_from_clipboard/DEPS )
BUG=682446
==========
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, dcheng@chromium.org, jif@chromium.org, tedchoc@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2790993003/#ps300001 (title: "tested interactively; works. removed field trial force-enable code")
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 pkasting@chromium.org
The CQ bit was checked by mpearson@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": 300001, "attempt_start_ts": 1491280678422540,
"parent_rev": "61c77351598040e6cfbb59e7c0612a312e16bf98", "commit_rev":
"054a0eadff47f8d158a2314e3858fb050ada6a4f"}
Message was sent while issue was closed.
Description was changed from
==========
Add Generic Implementation of ClipboardRecentContent
i.e., an implementation that works on all platforms for which
ui/base/Clipboard
has an implementation. This is all of them except iOS.
The only platform for which the implementation is sufficient at
this time to provide the necessary functionality is Android.
(The other platforms always return clipboard modification times of
the start of epoch, meaning any URLs in the clipboard will not be
suggested.)
I have tested this interactively and it works.
TBR=sky
(for the modifications to ui/base/test/test_clipboard.{h,cc}
and the inclusion of ui/base/test in components/open_from_clipboard/DEPS )
BUG=682446
==========
to
==========
Add Generic Implementation of ClipboardRecentContent
i.e., an implementation that works on all platforms for which
ui/base/Clipboard
has an implementation. This is all of them except iOS.
The only platform for which the implementation is sufficient at
this time to provide the necessary functionality is Android.
(The other platforms always return clipboard modification times of
the start of epoch, meaning any URLs in the clipboard will not be
suggested.)
I have tested this interactively and it works.
TBR=sky
(for the modifications to ui/base/test/test_clipboard.{h,cc}
and the inclusion of ui/base/test in components/open_from_clipboard/DEPS )
BUG=682446
Review-Url: https://codereview.chromium.org/2790993003
Cr-Commit-Position: refs/heads/master@{#461631}
Committed:
https://chromium.googlesource.com/chromium/src/+/054a0eadff47f8d158a2314e3858...
==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/054a0eadff47f8d158a2314e3858...
Message was sent while issue was closed.
I'm 85% sure that this is safe on Android because of the way the autocomplete data structures are created (all via browser service factories) https://cs.chromium.org/chromium/src/chrome/browser/android/omnibox/autocompl... and because Android is single-profile. I'm submitting this now because I need the UI designers to look at how the omnibox animates. If the change doesn't cause problems, then submitting is fine. And I can fix the lifetime issue in another changelist. If it causes problems and misses the dev channel push this week, then that's not an issue because it'll be fixed before the next push. If it causes problems and hits it, then, ugh. That said, I did test this somewhat on Android so it shouldn't cause entire browser-breaking problems. --mark https://codereview.chromium.org/2790993003/diff/260001/components/omnibox/bro... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2790993003/diff/260001/components/omnibox/bro... components/omnibox/browser/autocomplete_controller.cc:35: #if !defined(OS_IOS) On 2017/04/03 23:35:18, Peter Kasting wrote: > Nit: Put #if'd #includes in a separate group below Done. https://codereview.chromium.org/2790993003/diff/300001/components/open_from_c... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2790993003/diff/300001/components/open_from_c... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; On 2017/04/03 23:35:18, Peter Kasting wrote: > I'm still worried about lifetime management here. Rightly so, you make a good point below. I see two reasonable possibilities: (i) set up a BrowserContextKeyedServiceFactory for ClipboardRecentContentGeneric (ii) simply have a global single instance created on startup, a la iOS https://cs.chromium.org/chromium/src/ios/chrome/browser/ios_chrome_main_parts... (i) seems heavyweight but is probably the right thing to do for this generic implementation. Also, if choosing this path, we probably should convert the iOS code to do the same thing. (ii) seems easy and consistent with the iOS code. Also, if choosing this path, we should probably convert hte iOS code to delete the variable on shutdown. (Technically the variable is leaking.) A third option, ref-counted pointers, seems notably worse than either of these. > Let's say we create one AutocompleteController, which creates a clipboard > instance and sets it as the default. Then we create a second controller, which > does nothing because there's already an instance. Then we destroy the first > controller. This destroys the global instance. Now the second controller can > remain alive, with no instance, for arbitrary time. > > It seems like what you really want is either to have the ClipboardRecentContent > object be vended by a service factory, or have the one global instance be > refcounted, and have AutocompleteController take a scoped_refptr. > > Also seems like maybe ClipboardRecentContent() should SetInstance() > automatically in the constructor if there is no current instance, rather than > exposing that publicly.
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
still lgtm |
