Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832263002:
Clipboard Android - Store and Read Last Modified Time from Prefs (Closed)
DescriptionClipboard Android - Store and Read Last Modified Time from Prefs
Tested by enabling clipboard provider and the clipboard interactively on a device.
Clipboard suggestions are persisted to and restored from prefs, which works
even if I kill Chrome.
I also added LOG(INFO) lines during this and saw the writes and reads happening
at the correct time and with the right values.
BUG=711574
R=bauerb@chromium.org, dcheng@chromium.org
Review-Url: https://codereview.chromium.org/2832263002 .
Cr-Commit-Position: refs/heads/master@{#467796}
Committed: https://chromium.googlesource.com/chromium/src/+/ee17df91c787b9da94a89bf7d19704eeb6d130c6
Patch Set 1 #Patch Set 2 : comment cleanup #
Total comments: 7
Patch Set 3 : dcheng's comments, with a fragment of a test #Patch Set 4 : restore guard #
Total comments: 8
Patch Set 5 : ted's comments #Patch Set 6 : revert omnibox_field_trial_code, rebase #
Total comments: 18
Patch Set 7 : everyone's comments #
Total comments: 6
Patch Set 8 : rewriting everything to use callback and remove prefs from ui/ #Patch Set 9 : iadd missing files #
Total comments: 7
Patch Set 10 : final comments #Messages
Total messages: 58 (33 generated)
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...
mpearson@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, Can you glance at this changelist, in particular at the four files in chrome/browser/... to see whether I'm doing something sane? thanks, mark
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clipboard Android - Store and Read Last Modified Time from Prefs BUG=711574 ========== to ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. BUG=711574 ==========
mpearson@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng (who is I know OOO through May 2nd) to review ui/base/..., as I suspect he might be willing to appears review this small, careful changelist there. Or, if not, he can nominate a different OWNER to review. --mark
Description was changed from ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. BUG=711574 ========== to ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. By the way, I intended to add a unit test to ui/base/clipboard/clipboard_android_unittest.cc for this behavior. BUG=711574 ==========
As a general question, how does this work in the presence of multiple profiles? https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:205: // |local_state_| may be null in tests. Would it be cumbersome to fix the tests? I feel like "null for testing" is a bit too common in Chromium code =/ https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:27: static void RegisterPrefs(PrefRegistrySimple* registry); Could just export this one helper too.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> As a general question, how does this work in the > presence of multiple profiles? It works fine. :-) The clipboard (ClipboardAndroid) is shared among all profiles. When it's modified, the modified timestamp is stored immediately to local state, which is also shared among all profiles. In other words, the pref value always corresponds with the value in ClipboardMap's memory. If the SetLocalState() call happens multiple times (right now I put it in the after-profile-init call; Ted may have better suggestions), that's fine. It just means that it reads the current timestamp from prefs unnecessarily on the extra calls. The value would still be correct (correspond the value already in ClipboardMap's memory). When browsing data is cleared, the null timestamp is written to prefs, and no suggestions will be given on this profile or any other until the clipboard s modified again. This behavior is fine and was the behavior approved by privacy. Please take another look. (No need to look at the unittest by the way; I doubt I'm going to check that in in this changelist. The code there seems abandoned; it's not compiled. I may resurrect the file and add this test there, but that's probably best done in a different changelist.) cheers, mark https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:205: // |local_state_| may be null in tests. On 2017/04/22 01:43:01, dcheng (OOO through May 2) wrote: > Would it be cumbersome to fix the tests? I feel like "null for testing" is a bit > too common in Chromium code =/ Yes, I think it would be too cumbersome (despite there not being many test of clipboard functionality). It's straightforward to solve this for C++ unit tests (i.e., skip browser startup), just make the tests that use the clipboard such as BookmarkUtilsTest set up a fake prefs store before running the test. But solving this for Java-side unit tests (ClipboardTest, ImeTest) is more trouble. There's not way to Java code at the moment to set up of a fake pref store for ClipboardAndroid to use. To make those tests works, I'll need to add a new native bridge function, something like SetupPrefsForClipboardAndroid that the Java unit tests can call. Creating a new JNI bridge simply for testing, when the Java-side cannot actually access / test any of the information read from / stored in the pref store, seems too burdensome, not worthwhile. https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:27: static void RegisterPrefs(PrefRegistrySimple* registry); On 2017/04/22 01:43:01, dcheng (OOO through May 2) wrote: > Could just export this one helper too. Ah, I didn't know I could export individual functions, not whole classes. Exported this function and the one below it and ceased exporting the whole class.
tedchoc@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb who is much more familiar with prefs and might have a better opinion about when to initialize them. To me, what you have "seems" ok. I was looking for something in the shared chrome_browser_main but nothing jumped out as the obvious this is where you should do this. https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:188: UpdateLastModifiedTimePref(); should we make UpdateLastModifiedTimePref take the time and set last_modified_time_ internally? https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:28: static UI_BASE_EXPORT void RegisterPrefs(PrefRegistrySimple* registry); I would have expected static to come after the UI_BASE_EXPORT, but it seems pretty split in ui/ (more heavily biased to coming first in content/ though). https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:32: UI_BASE_EXPORT void SetLocalState(PrefService* local_state); local state is not something that is super easy to grok in my opinion (although it might have more awareness with the rest of chromium in which case ignore this). I think SetPrefService is clearer to me. Does it really matter that it came from g_browser_process->local_state()?
It looks like this change is generally good. I'm hoping it's time for some approvals to start rolling in. :-) --mark https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:188: UpdateLastModifiedTimePref(); On 2017/04/24 20:45:48, Ted C wrote: > should we make UpdateLastModifiedTimePref take the time and set > last_modified_time_ internally? Makes sense to me. Done. https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:28: static UI_BASE_EXPORT void RegisterPrefs(PrefRegistrySimple* registry); On 2017/04/24 20:45:48, Ted C wrote: > I would have expected static to come after the UI_BASE_EXPORT, but it seems > pretty split in ui/ (more heavily biased to coming first in content/ though). Acknowledged. Swapped the order simply because the new order makes it align better with the function below. https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:32: UI_BASE_EXPORT void SetLocalState(PrefService* local_state); On 2017/04/24 20:45:48, Ted C wrote: > local state is not something that is super easy to grok in my opinion (although > it might have more awareness with the rest of chromium in which case ignore > this). I think SetPrefService is clearer to me. Does it really matter that it > came from g_browser_process->local_state()? I understand SetPrefService seems clearer and probably is clearer for most folks, yet I think most folks would take it wrong. When talking about this changelist with other people (in person and via e-mail), I learned that virtually everything assumes "pref" implies "per-profile". I wanted to make the function name make clear that this was something different. I don't feel strongly about this choice if people object. That's just my reasoning.
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: This issue passed the CQ dry run.
I'm OK with landing as-is, so I'm going to LGTM the ui/base/clipboard bits. As mentioned though, I have reservations about the fact that fixing the tests to not have the "null for testing case" is cumbersome. To me, this indicates that maybe we should be tracking the last modified time for zero suggest purposes in the browser, independently of ui::Clipboard. https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:205: // |local_state_| may be null in tests. On 2017/04/24 17:33:26, Mark P wrote: > On 2017/04/22 01:43:01, dcheng (OOO through May 2) wrote: > > Would it be cumbersome to fix the tests? I feel like "null for testing" is a > bit > > too common in Chromium code =/ > > Yes, I think it would be too cumbersome (despite there not being many test of > clipboard functionality). It's straightforward to solve this for C++ unit tests > (i.e., skip browser startup), just make the tests that use the clipboard such as > BookmarkUtilsTest set up a fake prefs store before running the test. > > But solving this for Java-side unit tests (ClipboardTest, ImeTest) is more > trouble. There's not way to Java code at the moment to set up of a fake pref > store for ClipboardAndroid to use. To make those tests works, I'll need to add > a new native bridge function, something like SetupPrefsForClipboardAndroid that > the Java unit tests can call. Creating a new JNI bridge simply for testing, > when the Java-side cannot actually access / test any of the information read > from / stored in the pref store, seems too burdensome, not worthwhile. I was feeling a bit uncertain of whether or not ui::Clipboard should depend on PrefService before, and this honestly leads me to believe that maybe it shouldn't... the layering doesn't seem quite right if writing tests is this troublesome. Prefs is kind of a browser concept, while ui::Clipboard is more generic than that. Unfortunately, I don't have all the context paged in for this, so while I don't want to block this CL based purely on that, I'll make a note to re-read the design doc for this when I'm back. https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:32: UI_BASE_EXPORT void SetLocalState(PrefService* local_state); On 2017/04/24 22:19:56, Mark P wrote: > On 2017/04/24 20:45:48, Ted C wrote: > > local state is not something that is super easy to grok in my opinion > (although > > it might have more awareness with the rest of chromium in which case ignore > > this). I think SetPrefService is clearer to me. Does it really matter that > it > > came from g_browser_process->local_state()? > > I understand SetPrefService seems clearer and probably is clearer for most > folks, yet I think most folks would take it wrong. When talking about this > changelist with other people (in person and via e-mail), I learned that > virtually everything assumes "pref" implies "per-profile". I wanted to make the > function name make clear that this was something different. > > I don't feel strongly about this choice if people object. That's just my > reasoning. PrefService does seem profile-specific to me, while local state is just a completely unknown beast. Maybe we could improve our docs in this area somewhat (or maybe they already exist and I don't know where they are). All this is to say I slightly prefer local state (once I understood what it meant) https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:200: void ClipboardMap::UpdateLastModifiedTime(const base::Time& time) { base::Time is designed to be efficiently passed by value, so we should prefer that here.
https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:205: // |local_state_| may be null in tests. On 2017/04/25 13:17:12, dcheng (OOO through May 2) wrote: > On 2017/04/24 17:33:26, Mark P wrote: > > On 2017/04/22 01:43:01, dcheng (OOO through May 2) wrote: > > > Would it be cumbersome to fix the tests? I feel like "null for testing" is a > > bit > > > too common in Chromium code =/ > > > > Yes, I think it would be too cumbersome (despite there not being many test of > > clipboard functionality). It's straightforward to solve this for C++ unit > tests > > (i.e., skip browser startup), just make the tests that use the clipboard such > as > > BookmarkUtilsTest set up a fake prefs store before running the test. > > > > But solving this for Java-side unit tests (ClipboardTest, ImeTest) is more > > trouble. There's not way to Java code at the moment to set up of a fake pref > > store for ClipboardAndroid to use. To make those tests works, I'll need to > add > > a new native bridge function, something like SetupPrefsForClipboardAndroid > that > > the Java unit tests can call. Creating a new JNI bridge simply for testing, > > when the Java-side cannot actually access / test any of the information read > > from / stored in the pref store, seems too burdensome, not worthwhile. > > I was feeling a bit uncertain of whether or not ui::Clipboard should depend on > PrefService before, and this honestly leads me to believe that maybe it > shouldn't... the layering doesn't seem quite right if writing tests is this > troublesome. Prefs is kind of a browser concept, while ui::Clipboard is more > generic than that. > > Unfortunately, I don't have all the context paged in for this, so while I don't > want to block this CL based purely on that, I'll make a note to re-read the > design doc for this when I'm back. Hm, the network stack for example is also fairly general-use, but also uses prefs for configuration. "Local state" is more Chrome-specific, but so is the concept of having per-profile PrefServices (because profiles themselves are a Chrome concept!). Coming back to this particular thing though, what tests are we talking about here? If ClipboardAndroid has a method now to inject a PrefService, we could just inject a TestingPrefService in any test that exercises it. https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/android/... File chrome/browser/android/preferences/browser_prefs_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/android/... chrome/browser/android/preferences/browser_prefs_android.cc:14: class PrefService; Nit: empty line after this one. https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_android.cc:110: DCHECK(local_state != nullptr); Nit: Just DCHECK(local_state). https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:381: ::android::RegisterPrefs(registry); I don't think the :: is necessary. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:45: // Stores the last modified time in Time::Time's internal format (int64) as base::Time? https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:46: // a pref in local store. (I.e., this is not a per-profile pref.) Again, also kind of a layering violation, but I guess if it serves to clarify mistaken assumptions a reader might have, I'm okay with it. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:26: // chrome/browser/android/preferences/browser_prefs_android.cc Nit: this comment seems like a bit of a layering violation. If it's just about the UI_BASE_EXPORT, the comment could probably say that it's exported so other components can use it, but even that is not so uncommon that I think it would warrant a comment. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android_unittest.cc (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android_unittest.cc:92: // Set up the pref system. *** Super-nit: The asterisks look a bit out of place... https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android_unittest.cc:116: // we read it. Is that still coming? :)
mpearson@chromium.org changed reviewers: - tedchoc@chromium.org
Looks like we're almost set. I think the only remaining question is whether we can do something better with the "null for testing" issue. If we decide to remove that guard and configure all the tests correctly, I'd like to do that in a separate changelist, as that'll touch a number of files. Hopefully this changelist can be LGTMed as-is. Awaiting bauerb@'s review, approval, and comments about testing. Between him and dcheng@ (who already approved), that covers virtually all the files in this changelist. Moving tedchoc@ to CC because his review/approval is no longer necessary. --mark https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:205: // |local_state_| may be null in tests. Leaving the usage of prefs in here, as Bernhard says there's precedent (in the network state). > Coming back to this particular thing though, what tests are we talking about > here? If ClipboardAndroid has a method now to inject a PrefService, we could > just inject a TestingPrefService in any test that exercises it. See earlier on this comment thread. The hardest issues I think to get right are Java-side unit tests ClipboardTest, ImeTest). I'm not sure if TestingPrefService helps much at the core problem I mention above. https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/50001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:32: UI_BASE_EXPORT void SetLocalState(PrefService* local_state); On 2017/04/25 13:17:12, dcheng (OOO through May 2) wrote: > On 2017/04/24 22:19:56, Mark P wrote: > > On 2017/04/24 20:45:48, Ted C wrote: > > > local state is not something that is super easy to grok in my opinion > > (although > > > it might have more awareness with the rest of chromium in which case ignore > > > this). I think SetPrefService is clearer to me. Does it really matter that > > it > > > came from g_browser_process->local_state()? > > > > I understand SetPrefService seems clearer and probably is clearer for most > > folks, yet I think most folks would take it wrong. When talking about this > > changelist with other people (in person and via e-mail), I learned that > > virtually everything assumes "pref" implies "per-profile". I wanted to make > the > > function name make clear that this was something different. > > > > I don't feel strongly about this choice if people object. That's just my > > reasoning. > > PrefService does seem profile-specific to me, while local state is just a > completely unknown beast. Maybe we could improve our docs in this area somewhat > (or maybe they already exist and I don't know where they are). > > All this is to say I slightly prefer local state (once I understood what it > meant) Left function name as-is as previously discussed. Expanded comment to clarify what "local state" is. https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/android/... File chrome/browser/android/preferences/browser_prefs_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/android/... chrome/browser/android/preferences/browser_prefs_android.cc:14: class PrefService; On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Nit: empty line after this one. Done. https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_android.cc:110: DCHECK(local_state != nullptr); On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Nit: Just DCHECK(local_state). Done. Also removed TODO while I was here, as it sounds like this is a reasonable place for this code (implicitly from your review, explicitly from tedchoc@'s review). https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2832263002/diff/90001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:381: ::android::RegisterPrefs(registry); On 2017/04/25 13:30:06, Bernhard Bauer wrote: > I don't think the :: is necessary. It is, otherwise I get a compile error that chrome::android::RegisterPrefs isn't found. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:45: // Stores the last modified time in Time::Time's internal format (int64) as On 2017/04/25 13:30:06, Bernhard Bauer wrote: > base::Time? Correct. Done. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:46: // a pref in local store. (I.e., this is not a per-profile pref.) On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Again, also kind of a layering violation, but I guess if it serves to clarify > mistaken assumptions a reader might have, I'm okay with it. Acknowledged. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.cc:200: void ClipboardMap::UpdateLastModifiedTime(const base::Time& time) { On 2017/04/25 13:17:12, dcheng (OOO through May 2) wrote: > base::Time is designed to be efficiently passed by value, so we should prefer > that here. Done. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android.h:26: // chrome/browser/android/preferences/browser_prefs_android.cc On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Nit: this comment seems like a bit of a layering violation. If it's just about > the UI_BASE_EXPORT, the comment could probably say that it's exported so other > components can use it, but even that is not so uncommon that I think it would > warrant a comment. In retrospect, I agree. This isn't worth a comment. Removed. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_android_unittest.cc (right): https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android_unittest.cc:92: // Set up the pref system. *** On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Super-nit: The asterisks look a bit out of place... Moot. I've removed this test from the changelist per dcheng@'s advice. This test isn't getting run and fixing it is a long-standing problem (crbug.com/434620) and out-of-scope for this changelist. https://codereview.chromium.org/2832263002/diff/90001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_android_unittest.cc:116: // we read it. On 2017/04/25 13:30:06, Bernhard Bauer wrote: > Is that still coming? :) No. :-P See above.
Description was changed from ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. By the way, I intended to add a unit test to ui/base/clipboard/clipboard_android_unittest.cc for this behavior. BUG=711574 ========== to ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. BUG=711574 ==========
mpearson@chromium.org changed reviewers: + sky@chromium.org
+sky@ for the additional include in ui/base/BUILD.gn (which for the record bauerb@ says is reasonable :-) ) --mark
We don't want ui depending upon prefs. Either use a delegate or callback to notify consumers when the clipboard changes.
On Tue, Apr 25, 2017 at 1:59 PM, <sky@chromium.org> wrote: > We don't want ui depending upon prefs. Either use a delegate or callback to > notify consumers when the clipboard changes. > I'm curious: why is net/ okay and ui/ not okay? Okay, so if the code for this needs to live elsewhere, where do you suggest? ui/browser/... ? chrome/browser/ui/...? components/prefs/ui/... ? Finally, do you (or bauerb@) have an example I can refer to? I know how to make a delegate or callback to notify someone of a clipboard change so they can write the prefs to the clipboard. It's less clear on a best practice for how to read the pref on startup and inject it into ui/base/clipboard. Where would that startup reading / injecting code live and where it be called from? thanks, mark -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Apr 25, 2017 at 3:01 PM, Mark Pearson <mpearson@chromium.org> wrote: > > On Tue, Apr 25, 2017 at 1:59 PM, <sky@chromium.org> wrote: > >> We don't want ui depending upon prefs. Either use a delegate or callback >> to >> notify consumers when the clipboard changes. >> > > I'm curious: why is net/ okay and ui/ not okay? > The /net dependency is iffy as well. We should really strive to minimize dependencies on shared framework code like this. > > Okay, so if the code for this needs to live elsewhere, where do you > suggest? ui/browser/... ? chrome/browser/ui/...? > components/prefs/ui/... ? > > chrome/browser/chrome_browser_main_android.cc could set the callback. > Finally, do you (or bauerb@) have an example I can refer to? I know how > to make a delegate or callback to notify someone of a clipboard change so > they can write the prefs to the clipboard. It's less clear on a best > practice for how to read the pref on startup and inject it into > ui/base/clipboard. Where would that startup reading / injecting code live > and where it be called from? > Could c/b/chrome_browser_main_android.cc set the callback and the last-modified-time at once? -Scott > > thanks, > mark > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Apr 25, 2017 at 3:01 PM, Mark Pearson <mpearson@chromium.org> wrote: > > On Tue, Apr 25, 2017 at 1:59 PM, <sky@chromium.org> wrote: > >> We don't want ui depending upon prefs. Either use a delegate or callback >> to >> notify consumers when the clipboard changes. >> > > I'm curious: why is net/ okay and ui/ not okay? > The /net dependency is iffy as well. We should really strive to minimize dependencies on shared framework code like this. > > Okay, so if the code for this needs to live elsewhere, where do you > suggest? ui/browser/... ? chrome/browser/ui/...? > components/prefs/ui/... ? > > chrome/browser/chrome_browser_main_android.cc could set the callback. > Finally, do you (or bauerb@) have an example I can refer to? I know how > to make a delegate or callback to notify someone of a clipboard change so > they can write the prefs to the clipboard. It's less clear on a best > practice for how to read the pref on startup and inject it into > ui/base/clipboard. Where would that startup reading / injecting code live > and where it be called from? > Could c/b/chrome_browser_main_android.cc set the callback and the last-modified-time at once? -Scott > > thanks, > mark > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/25 22:19:48, sky wrote:
> On Tue, Apr 25, 2017 at 3:01 PM, Mark Pearson <mailto:mpearson@chromium.org>
wrote:
>
> >
> > On Tue, Apr 25, 2017 at 1:59 PM, <mailto:sky@chromium.org> wrote:
> >
> >> We don't want ui depending upon prefs. Either use a delegate or callback
> >> to
> >> notify consumers when the clipboard changes.
> >>
> >
> > I'm curious: why is net/ okay and ui/ not okay?
> >
>
> The /net dependency is iffy as well. We should really strive to minimize
> dependencies on shared framework code like this.
FWIW, I think that using prefs in //net helped a lot with getting rid of
boilerplate code that had to watch preferences outside of it and update the
values in //net classes.
> >
> > Okay, so if the code for this needs to live elsewhere, where do you
> > suggest? ui/browser/... ? chrome/browser/ui/...?
> > components/prefs/ui/... ?
> >
> >
> chrome/browser/chrome_browser_main_android.cc could set the callback.
>
>
> > Finally, do you (or bauerb@) have an example I can refer to? I know how
> > to make a delegate or callback to notify someone of a clipboard change so
> > they can write the prefs to the clipboard. It's less clear on a best
> > practice for how to read the pref on startup and inject it into
> > ui/base/clipboard. Where would that startup reading / injecting code live
> > and where it be called from?
> >
>
> Could c/b/chrome_browser_main_android.cc set the callback and the
> last-modified-time at once?
It would probably be nice not to dump too much stuff straight into
c/b/chrome_browser_main_android.cc.
What I would suggest is adding a new pair of files to c/b/android/preferences/,
e.g. clipboard.{h,cc}, that contains the pref registration, a method that reads
the last-modified time, and a method that sets the last-modified time. As Local
State is owned by the browser process global, that class could just get it from
there when it needs to, so it wouldn't need any state and could just be static
methods. Then c/b/chrome_browser_main_android.cc would read the last-modified
time and set it on the ClipboardAndroid, and bind a callback to the update
method and set that on the ClipboardAndroid. ClipboardAndroid would then call
that update method when it's modified.
https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_android.cc (right):
https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:47: const char
kClipboardLastModifiedTimePref[] = "ui.clipboard.last_modified_time";
Oh, usually pref names are defined in chrome/common/pref_names.{h,cc}. Sorry, I
totally missed that earlier (and this class couldn't have accessed
chrome/common/ anyway, but now it should work).
https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_android.cc:80: void UpdateLastModifiedTime(const
base::Time time);
Small nit: The const now only makes the local variable const, which we usually
don't bother with.
mpearson@chromium.org changed reviewers: - sky@chromium.org
bauerb@, Can you please take a look at this changelist? It's been rewritten to remove the prefs include from ui/. The sooner, the better, as this change is still intended to be merged into M-59. I'm dropping sky@ as a reviewer because I no longer need his approval, as I'm no longer adding a new dependency to ui/. thanks, mark https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:47: const char kClipboardLastModifiedTimePref[] = "ui.clipboard.last_modified_time"; On 2017/04/26 10:03:42, Bernhard Bauer wrote: > Oh, usually pref names are defined in chrome/common/pref_names.{h,cc}. Sorry, I > totally missed that earlier (and this class couldn't have accessed > chrome/common/ anyway, but now it should work). Defined them there now. For what it's worth, I originally decided to put them in this file for a reasonable reason. I looked around for pref names and found many files where prefs are defined locally in a file, always in cases when the pref is not accessed elsewhere. I inferred the putting things in common was only necessary/required when a pref name needed a common place to live (i.e., be accessed from multiple files across the codebase). https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:80: void UpdateLastModifiedTime(const base::Time time); On 2017/04/26 10:03:42, Bernhard Bauer wrote: > Small nit: The const now only makes the local variable const, which we usually > don't bother with. Done.
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: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:47: const char kClipboardLastModifiedTimePref[] = "ui.clipboard.last_modified_time"; On 2017/04/26 22:18:32, Mark P wrote: > On 2017/04/26 10:03:42, Bernhard Bauer wrote: > > Oh, usually pref names are defined in chrome/common/pref_names.{h,cc}. Sorry, > I > > totally missed that earlier (and this class couldn't have accessed > > chrome/common/ anyway, but now it should work). > > Defined them there now. > > For what it's worth, I originally decided to put them in this file for a > reasonable reason. I looked around for pref names and found many files where > prefs are defined locally in a file, always in cases when the pref is not > accessed elsewhere. I inferred the putting things in common was only > necessary/required when a pref name needed a common place to live (i.e., be > accessed from multiple files across the codebase). That probably was more of an oversight on part of the reviewer ;-) I think the idea is to have a central place for all prefs (well, at least in a given layer). But anyway, thanks for the fix. https://codereview.chromium.org/2832263002/diff/150001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2832263002/diff/150001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_android.cc:105: // once per profile load), that's okay; the additional calls don't change Just for the sake of completeness: There aren't actually multiple profiles on Android (incognito notwithstanding). https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.h:25: typedef base::Callback<void(base::Time)> Modified; Nit: ModifiedCallback? Or just inline the type; it's not that complicated.
lgtm with some minor nits FWIW, I think I see a way to pull the last modified time logic out of the clipboard internals and maintain it nearer the omnibox suggestion code. I'll poke at that when I'm back. https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:103: modified_cb_ = std::move(cb); Nit: #include <utility> https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.h:25: typedef base::Callback<void(base::Time)> Modified; On 2017/04/27 09:27:17, Bernhard Bauer wrote: > Nit: ModifiedCallback? Or just inline the type; it's not that complicated. Nit 2: prefer using A = B; over typedef B A;
https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/110001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:47: const char kClipboardLastModifiedTimePref[] = "ui.clipboard.last_modified_time"; On 2017/04/27 09:27:17, Bernhard Bauer wrote: > On 2017/04/26 22:18:32, Mark P wrote: > > On 2017/04/26 10:03:42, Bernhard Bauer wrote: > > > Oh, usually pref names are defined in chrome/common/pref_names.{h,cc}. > Sorry, > > I > > > totally missed that earlier (and this class couldn't have accessed > > > chrome/common/ anyway, but now it should work). > > > > Defined them there now. > > > > For what it's worth, I originally decided to put them in this file for a > > reasonable reason. I looked around for pref names and found many files where > > prefs are defined locally in a file, always in cases when the pref is not > > accessed elsewhere. I inferred the putting things in common was only > > necessary/required when a pref name needed a common place to live (i.e., be > > accessed from multiple files across the codebase). > > That probably was more of an oversight on part of the reviewer ;-) I think the > idea is to have a central place for all prefs (well, at least in a given layer). > But anyway, thanks for the fix. Acknowledged. https://codereview.chromium.org/2832263002/diff/150001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2832263002/diff/150001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_android.cc:105: // once per profile load), that's okay; the additional calls don't change On 2017/04/27 09:27:17, Bernhard Bauer wrote: > Just for the sake of completeness: There aren't actually multiple profiles on > Android (incognito notwithstanding). Thanks for the remark. Did you want me to add that comment? I think it makes things more confusing. (There aren't actually multiple profiles, yet there is incognito, and doesn't that mean this gets called again here?) Left as-is. If you reply after I submit this and want a comment change, I'm happy to oblige. Or I can remove the comment entirely, or maybe instead put a comment like this by the definition of InitClipboardAndroidFromLocalState(). // Idempotent. If called multiple times wit the same |local_state| parameter, that's okay; the additional calls don't change anything. https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.cc (right): https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.cc:103: modified_cb_ = std::move(cb); On 2017/04/27 15:25:39, dcheng (OOO through May 2) wrote: > Nit: #include <utility> Done. https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... File ui/base/clipboard/clipboard_android.h (right): https://codereview.chromium.org/2832263002/diff/150001/ui/base/clipboard/clip... ui/base/clipboard/clipboard_android.h:25: typedef base::Callback<void(base::Time)> Modified; On 2017/04/27 15:25:39, dcheng (OOO through May 2) wrote: > On 2017/04/27 09:27:17, Bernhard Bauer wrote: > > Nit: ModifiedCallback? Sounds good to me. Done. > Or just inline the type; it's not that complicated. True, but it's used enough that inlining is cumbersome. (I could at least four places in the .cc file.) > Nit 2: prefer using A = B; over typedef B A; Done.
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2832263002/#ps170001 (title: "final comments")
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: 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-...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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
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 mpearson@chromium.org
Description was changed from ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. BUG=711574 ========== to ========== Clipboard Android - Store and Read Last Modified Time from Prefs Tested by enabling clipboard provider and the clipboard interactively on a device. Clipboard suggestions are persisted to and restored from prefs, which works even if I kill Chrome. I also added LOG(INFO) lines during this and saw the writes and reads happening at the correct time and with the right values. BUG=711574 R=bauerb@chromium.org, dcheng@chromium.org Review-Url: https://codereview.chromium.org/2832263002 . Cr-Commit-Position: refs/heads/master@{#467796} Committed: https://chromium.googlesource.com/chromium/src/+/ee17df91c787b9da94a89bf7d197... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) manually as ee17df91c787b9da94a89bf7d19704eeb6d130c6 (presubmit successful). |
