Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801813003:
Omnibox - ClipboardRecentContent - Make Proper Singleton (Closed)
DescriptionOmnibox - ClipboardRecentContent - Make Proper Singleton
This is a follow-up from
https://codereview.chromium.org/2790993003
in particular
https://codereview.chromium.org/2790993003#msg61
with its concerns about the lifetime of this object.
TBR=jif
BUG=682446
Patch Set 1 #Patch Set 2 : restore field trial state #
Total comments: 10
Patch Set 3 : pkasting's comments #
Total comments: 2
Patch Set 4 : typo #Patch Set 5 : jif@'s suggestion #
Total comments: 6
Patch Set 6 : pkasting comments #Patch Set 7 : latest pkasting suggestion #Patch Set 8 : fix iOS #
Messages
Total messages: 44 (27 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Omnibox - ClipboardRecentContent - Make Proper Singleton BUG=682446 ========== to ========== Omnibox - ClipboardRecentContent - Make Proper Singleton This is a follow-up from https://codereview.chromium.org/2790993003 in particular https://codereview.chromium.org/2790993003#msg61 with its concerns about the lifetime of this object. BUG=682446 ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Since it's okay to leak on process exit, this code suffices. (It passes the tests and whatnot.) Or do you want something more complicated? --mark
https://codereview.chromium.org/2801813003/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:245: ClipboardRecentContent::SetInstance(new ClipboardRecentContentGeneric()); Nit: Might want a comment like "This pointer is intentionally leaked, since no action is necessary at shutdown." https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (left): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; Why are you removing this line? The only effect of doing so is that GetInstance() will return pointer-to-garbage in cases where it was returning pointer-to-null. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:35: void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) { Is there an advantage of having this setter at all over just having the constructor do this stuff? This seems like it would be simpler (for callers) and safer as well.
https://codereview.chromium.org/2801813003/diff/20001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:245: ClipboardRecentContent::SetInstance(new ClipboardRecentContentGeneric()); On 2017/04/05 23:23:51, Peter Kasting wrote: > Nit: Might want a comment like "This pointer is intentionally leaked, since no > action is necessary at shutdown." Done. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (left): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; On 2017/04/05 23:23:51, Peter Kasting wrote: > Why are you removing this line? The only effect of doing so is that > GetInstance() will return pointer-to-garbage in cases where it was returning > pointer-to-null. No, that's not the only effect. If someone creates a ClipboardRecentContent object locally, say, on a stack in tests, then when the object gets deleted, we clear the global singleton. That's not right. The global singleton should never be cleared or deleted, just auto-cleaned when the OS shuts down the process. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:35: void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) { On 2017/04/05 23:23:51, Peter Kasting wrote: > Is there an advantage of having this setter at all over just having the > constructor do this stuff? This seems like it would be simpler (for callers) > and safer as well. Oh, do you mean have the constructor set the global singleton pointer to point to the new object? That seems like it would work. But there are two reasons I don't want to do: 1. That's a weird and unexpected side effect. Constructing an object causes a modification to global state. 2. Various tests will have to revised because then they wouldn't be able to create their own recent content object. They'd all have to use the global. For example, this means they cannot run simultaneously. We'd also have to be more careful to make them hermetic. (I think I'm using that term right.)
I don't think the debates below are worth holding this up over. LGTM, do what you think is wise. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (left): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; On 2017/04/06 17:58:00, Mark P wrote: > On 2017/04/05 23:23:51, Peter Kasting wrote: > > Why are you removing this line? The only effect of doing so is that > > GetInstance() will return pointer-to-garbage in cases where it was returning > > pointer-to-null. > > No, that's not the only effect. > > If someone creates a ClipboardRecentContent object locally, say, on a stack in > tests, then when the object gets deleted, we clear the global singleton. That's > not right. The global singleton should never be cleared or deleted, just > auto-cleaned when the OS shuts down the process. OK. I think my assumption mentally is that creating one of these locally is wrong. It only makes sense to access them through the static getter. See other comment's discussion thread. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:35: void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) { On 2017/04/06 17:58:00, Mark P wrote: > On 2017/04/05 23:23:51, Peter Kasting wrote: > > Is there an advantage of having this setter at all over just having the > > constructor do this stuff? This seems like it would be simpler (for callers) > > and safer as well. > > Oh, do you mean have the constructor set the global singleton pointer to point > to the new object? That seems like it would work. But there are two reasons I > don't want to do: > 1. That's a weird and unexpected side effect. Constructing an object causes a > modification to global state. True, but sort of makes sense if no one is ever expected to construct one of these. We use this technique with various other globals in views/chrome that tests occasionally need to override. > 2. Various tests will have to revised because then they wouldn't be able to > create their own recent content object. They'd all have to use the global. Correct. > For example, this means they cannot run simultaneously. I don't think our test harness actually runs simultaneous tests within the same process, does it? I thought it sharded using multiple processes? There are lots of tests that muck with global state while they're running, so I can't imagine how things work today if we just use multiple threads. > We'd also have to be more > careful to make them hermetic. (I think I'm using that term right.) I don't know, I'm not sure what you're saying :) https://codereview.chromium.org/2801813003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:245: // This point is intentionally leaked, since no action is necessary at Nit: pointer
mpearson@chromium.org changed reviewers: + jif@chromium.org
jif@, Can you please review this slight refactoring? thanks, mark https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (left): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:27: g_clipboard_recent_content = nullptr; On 2017/04/06 19:35:41, Peter Kasting wrote: > On 2017/04/06 17:58:00, Mark P wrote: > > On 2017/04/05 23:23:51, Peter Kasting wrote: > > > Why are you removing this line? The only effect of doing so is that > > > GetInstance() will return pointer-to-garbage in cases where it was returning > > > pointer-to-null. > > > > No, that's not the only effect. > > > > If someone creates a ClipboardRecentContent object locally, say, on a stack in > > tests, then when the object gets deleted, we clear the global singleton. > That's > > not right. The global singleton should never be cleared or deleted, just > > auto-cleaned when the OS shuts down the process. > > OK. I think my assumption mentally is that creating one of these locally is > wrong. It only makes sense to access them through the static getter. See other > comment's discussion thread. Acknowledged. https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/20001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:35: void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) { Left as-is because the side effect of what you propose unnerves me. Let's see what the OWNER of this directory says. On 2017/04/06 19:35:41, Peter Kasting wrote: > On 2017/04/06 17:58:00, Mark P wrote: > > On 2017/04/05 23:23:51, Peter Kasting wrote: > > > Is there an advantage of having this setter at all over just having the > > > constructor do this stuff? This seems like it would be simpler (for > callers) > > > and safer as well. > > > > Oh, do you mean have the constructor set the global singleton pointer to point > > to the new object? That seems like it would work. But there are two reasons > I > > don't want to do: > > 1. That's a weird and unexpected side effect. Constructing an object causes a > > modification to global state. > > True, but sort of makes sense if no one is ever expected to construct one of > these. We use this technique with various other globals in views/chrome that > tests occasionally need to override. > > > 2. Various tests will have to revised because then they wouldn't be able to > > create their own recent content object. They'd all have to use the global. > > Correct. > > > For example, this means they cannot run simultaneously. > > I don't think our test harness actually runs simultaneous tests within the same > process, does it? I thought it sharded using multiple processes? > > There are lots of tests that muck with global state while they're running, so I > can't imagine how things work today if we just use multiple threads. > > > We'd also have to be more > > careful to make them hermetic. (I think I'm using that term right.) > > I don't know, I'm not sure what you're saying :) https://codereview.chromium.org/2801813003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:245: // This point is intentionally leaked, since no action is necessary at On 2017/04/06 19:35:41, Peter Kasting wrote: > Nit: pointer Done.
Should we consider having the instance be in a unique_ptr? This would make it impossible for the instance pointer to be a pointer-to-garbage, and allow us to call SetInstance multiple times in a row (and thus remove the DCHECK). I don't think we are allowed to have global static unique_ptr, so we'd have to do a bit of gymnastic, for example: https://paste.googleplex.com/5852006807764992
jif@, Please take another look. On 2017/04/07 09:28:05, jif wrote: > Should we consider having the instance be in a unique_ptr? > This would make it impossible for the instance pointer to be a > pointer-to-garbage, and allow us to call SetInstance multiple times in a row > (and thus remove the DCHECK). Seems reasonable. This handles most of Peter's concerns. > I don't think we are allowed to have global static unique_ptr, so we'd have to > do a bit of gymnastic, for example: > https://paste.googleplex.com/5852006807764992 Thanks for the example code. It made making this change easy. :-) thanks, mark
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/2801813003/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:247: std::unique_ptr<ClipboardRecentContentGeneric>()); Doesn't this fail to actually create the underlying object, i.e. the contents of this unique_ptr are null? I think you want MakeUnique instead. https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:52: ClipboardRecentContent::UniquePtrToInstance() { Why do this as a static member function instead of a file-scoped unique_ptr? The latter seems more direct.
https://codereview.chromium.org/2801813003/diff/80001/components/omnibox/brow... File components/omnibox/browser/autocomplete_controller.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/omnibox/brow... components/omnibox/browser/autocomplete_controller.cc:247: std::unique_ptr<ClipboardRecentContentGeneric>()); On 2017/04/10 21:59:22, Peter Kasting wrote: > Doesn't this fail to actually create the underlying object, i.e. the contents of > this unique_ptr are null? I think you want MakeUnique instead. Right you are. :-) Done. (I didn't bother testing this change, instead waiting for the try jobs.) https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:52: ClipboardRecentContent::UniquePtrToInstance() { On 2017/04/10 21:59:22, Peter Kasting wrote: > Why do this as a static member function instead of a file-scoped unique_ptr? > The latter seems more direct. jif@ wrote: "I don't think we are allowed to have global static unique_ptr," Do you disagree?
https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:52: ClipboardRecentContent::UniquePtrToInstance() { On 2017/04/10 22:05:22, Mark P wrote: > On 2017/04/10 21:59:22, Peter Kasting wrote: > > Why do this as a static member function instead of a file-scoped unique_ptr? > > The latter seems more direct. > > jif@ wrote: > "I don't think we are allowed to have global static unique_ptr," > > Do you disagree? On thinking about it more I think both methods are problematic. The style guide basically bans globals/statics that run destructors. So no matter where we declare it this is a problem. We want the object to not be destroyed on exit. We could do this with CR_DEFINE_STATIC_LOCAL here instead. But this all feels like overkill. Why not leave the interface above as taking a scoped_ptr (to indicate ownership transfer), but under the hood store this in a raw pointer like before? ClipboardRecentContent* g_clipboard_recent_content = nullptr; void ClipboardRecentContent::SetInstance( std::unique_ptr<ClipboardRecentContent> new_instance) { delete g_clipboard_recent_content; g_clipboard_recent_content = new_instance.release(); }
The CQ bit was checked by mpearson@chromium.org to run a CQ dry run
https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... File components/open_from_clipboard/clipboard_recent_content.cc (right): https://codereview.chromium.org/2801813003/diff/80001/components/open_from_cl... components/open_from_clipboard/clipboard_recent_content.cc:52: ClipboardRecentContent::UniquePtrToInstance() { On 2017/04/10 22:18:34, Peter Kasting wrote: > On 2017/04/10 22:05:22, Mark P wrote: > > On 2017/04/10 21:59:22, Peter Kasting wrote: > > > Why do this as a static member function instead of a file-scoped unique_ptr? > > > > The latter seems more direct. > > > > jif@ wrote: > > "I don't think we are allowed to have global static unique_ptr," > > > > Do you disagree? > > On thinking about it more I think both methods are problematic. The style guide > basically bans globals/statics that run destructors. So no matter where we > declare it this is a problem. We want the object to not be destroyed on exit. > > We could do this with CR_DEFINE_STATIC_LOCAL here instead. But this all feels > like overkill. Why not leave the interface above as taking a scoped_ptr (to > indicate ownership transfer), but under the hood store this in a raw pointer > like before? > > ClipboardRecentContent* g_clipboard_recent_content = nullptr; > > void ClipboardRecentContent::SetInstance( > std::unique_ptr<ClipboardRecentContent> new_instance) { > delete g_clipboard_recent_content; > g_clipboard_recent_content = new_instance.release(); > } Okay. This is also fine with me. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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...
If jif@ approves, I may need to add someone for the ios/chrome/browser/ios_chrome_main_parts.mm change. --mark
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Omnibox - ClipboardRecentContent - Make Proper Singleton This is a follow-up from https://codereview.chromium.org/2790993003 in particular https://codereview.chromium.org/2790993003#msg61 with its concerns about the lifetime of this object. BUG=682446 ========== to ========== Omnibox - ClipboardRecentContent - Make Proper Singleton This is a follow-up from https://codereview.chromium.org/2790993003 in particular https://codereview.chromium.org/2790993003#msg61 with its concerns about the lifetime of this object. TBR=jif (I feel comfortable doing this because this implementation is basically exactly what he asked for in his last comment before going away.) BUG=682446 ==========
Description was changed from ========== Omnibox - ClipboardRecentContent - Make Proper Singleton This is a follow-up from https://codereview.chromium.org/2790993003 in particular https://codereview.chromium.org/2790993003#msg61 with its concerns about the lifetime of this object. TBR=jif (I feel comfortable doing this because this implementation is basically exactly what he asked for in his last comment before going away.) BUG=682446 ========== to ========== Omnibox - ClipboardRecentContent - Make Proper Singleton This is a follow-up from https://codereview.chromium.org/2790993003 in particular https://codereview.chromium.org/2790993003#msg61 with its concerns about the lifetime of this object. TBR=jif 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 Link to the patchset: https://codereview.chromium.org/2801813003/#ps140001 (title: "fix iOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492025492472810,
"parent_rev": "038501c2703a4439409cd0d7c002b303ffc6377f", "commit_rev":
"05c918b17e88cc3916584f2a82d04b73ff8ebd72"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492025492472810,
"parent_rev": "c8dd8f2427404cbc64f103733124c6c4e0f51621", "commit_rev":
"b46b1ebd67e20e2d0f754c45c1a8505baeddbeaf"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492025492472810,
"parent_rev": "3aa3acab3973a1d26a8bfe4f335d64a37a31eaae", "commit_rev":
"03a5e89da19258391f9b0f06bb0d14fae46ce401"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by mpearson@chromium.org
The CQ bit was unchecked by mpearson@chromium.org
On 2017/04/12 21:26:59, commit-bot: I haz the power wrote: > Failed to commit the patch. Despite this message, the associated bug has a message posted to it about the change being committed. https://chromium.googlesource.com/chromium/src.git/+/03a5e89da19258391f9b0f06... --mark
Message was sent while issue was closed.
On 2017/04/12 21:36:04, Mark P (away Mon Apr 17) wrote: > On 2017/04/12 21:26:59, commit-bot: I haz the power wrote: > > Failed to commit the patch. > > Despite this message, the associated bug has a message posted to it about the > change being committed. > https://chromium.googlesource.com/chromium/src.git/+/03a5e89da19258391f9b0f06... > > --mark lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
