Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2971323002:
Switch cookie async mechanism over to using callbacks. (Closed)
|
Created:
3 years, 5 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 5 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch cookie async mechanism over to using callbacks.
Switch mechanism for delaying cookie reuqests until the appropriate
parts of the cookie database are loaded over to using callbacks instead
of home-grown classes.
BUG=None
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2971323002
Cr-Commit-Position: refs/heads/master@{#486105}
Committed: https://chromium.googlesource.com/chromium/src/+/e5c701df767b131a3074131a886314e53a7dd0dd
Patch Set 1 #
Total comments: 13
Patch Set 2 : Added back in null callback tests. #
Total comments: 1
Patch Set 3 : Converted all interfaces over to callback. #Patch Set 4 : Put callback null tests back in :-}. #Patch Set 5 : Avoid BindOnce on null callbacks. #Patch Set 6 : Cleanup usage of GetAllCookies() in cookie_monster_perftest.cc #Patch Set 7 : Sync'd to p485987 #
Total comments: 37
Patch Set 8 : Incorporated comments. #
Dependent Patchsets: Messages
Total messages: 47 (31 generated)
The CQ bit was checked by rdsmith@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_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:109: std::move(response).Run(std::move(function).Run()); The old code null checked all of these callbacks. Should this as well? https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( Not suggesting it in this CL, but wonder if we could get rid of this double-bind in the future by, for instance, having SetCookieWithDetails take + invoke the callback itself. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1501: return true; This doesn't seem to belong in this CL. I'm not at all sure we have sufficient testing, in particular, so think this needs review independent of everything else. I'm also wondering if it's used enough for us to care enough to do this.
*chuckle* I hadn't actually intended to submit this for review yet, specifically because I wanted to think about two of the issues you raise and decide what I wanted to do about them before asking for your opinion. I'm not addressing those two points in these comments (I'll come back to them after I figure out what I think), just the one point you raised that surprised me. (And yes, I'm indulging myself in a pure cleanup CL on my vacation. I'm not completely certain how I feel about that :-}.) https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (left): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:797: void CookieMonster::SetAllCookiesTask::Run() { Location of the code I refer to in the other comment. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1501: return true; On 2017/07/07 16:54:58, mmenke wrote: > This doesn't seem to belong in this CL. I'm not at all sure we have sufficient > testing, in particular, so think this needs review independent of everything > else. I'm also wondering if it's used enough for us to care enough to do this. To be clear, this isn't new code--it's just a refactor. The equivalent code used to be in SetAllCookiesTask::Run. Given that, do you still want to make this statement? I'm not sure what to do in this CL if I *don't* copy this code over.
On 2017/07/07 20:09:55, Randy Smith (Not in Mondays) wrote: > *chuckle* I hadn't actually intended to submit this for review yet, > specifically because I wanted to think about two of the issues you raise and > decide what I wanted to do about them before asking for your opinion. I'm not > addressing those two points in these comments (I'll come back to them after I > figure out what I think), just the one point you raised that surprised me. > > (And yes, I'm indulging myself in a pure cleanup CL on my vacation. I'm not > completely certain how I feel about that :-}.) You just find cleaning up old code as exciting as I do. Who even needs vacation when you can spend time being paid to clean up code instead? https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1501: return true; On 2017/07/07 20:09:54, Randy Smith (Not in Mondays) wrote: > On 2017/07/07 16:54:58, mmenke wrote: > > This doesn't seem to belong in this CL. I'm not at all sure we have > sufficient > > testing, in particular, so think this needs review independent of everything > > else. I'm also wondering if it's used enough for us to care enough to do this. > > To be clear, this isn't new code--it's just a refactor. The equivalent code > used to be in SetAllCookiesTask::Run. Given that, do you still want to make > this statement? I'm not sure what to do in this CL if I *don't* copy this code > over. Sorry, completely missed that.
The CQ bit was checked by rdsmith@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.
Comments responded to, and callback testing put back in; PTAL? One specific thing I'd like your perspective on: Is there something I should be doing, other than net_perftests::CookieMonsterTest.*, to evaluate whether this change has a negative performance impact? Running release compiles of net_perftests::CookieMonsterTest.* with and without this change showed only noise, but as noted below, my faith in that test is ... lacking. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:109: std::move(response).Run(std::move(function).Run()); On 2017/07/07 16:54:58, mmenke wrote: > The old code null checked all of these callbacks. Should this as well? Yeah, it should; in fact, try jobs fail if it doesn't. Done. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( On 2017/07/07 16:54:58, mmenke wrote: > Not suggesting it in this CL, but wonder if we could get rid of this double-bind > in the future by, for instance, having SetCookieWithDetails take + invoke the > callback itself. I need a word for "Yeah, but if I go in that direction the slippery slope will catch me and I'll end up at the bottom of the hill" :-}. So I thought about this and ended up with: * I think this would work fine. * It isn't needed for performance (at least according to CookieMonsterPerftest, which is a completely reliable source of information on cookie performance :-z.) * After some consideration I concluded that it would indeed be better in terms of code size and comprehensibility. * I started looking at the interfaces and realized that if I did this, *CookieWith* and *CookieWith*Async would have the same signatures, which make me think that then I should really combine those routines and conditionalize their action based on the state of the cookie loading (which I think would be a win for performance). * I then realized that that mean I'd need to deal with reentrancy in the callbacks, and PostTask the response if I wasn't actually queueing the callback pending loading. * I then threw up my hands and decided that going further than what I've done wasn't actually what I wanted to do in this CL, and by proxy, in the immediate or planned future. Let me know if you want to try and twist my arm, or if you want a TODO() sketching out the above somewhere. But for the moment I'm disinclined to go in this, perfectly reasonable, direction. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1501: return true; On 2017/07/07 20:15:12, mmenke wrote: > On 2017/07/07 20:09:54, Randy Smith (Not in Mondays) wrote: > > On 2017/07/07 16:54:58, mmenke wrote: > > > This doesn't seem to belong in this CL. I'm not at all sure we have > > sufficient > > > testing, in particular, so think this needs review independent of everything > > > else. I'm also wondering if it's used enough for us to care enough to do > this. > > > > To be clear, this isn't new code--it's just a refactor. The equivalent code > > used to be in SetAllCookiesTask::Run. Given that, do you still want to make > > this statement? I'm not sure what to do in this CL if I *don't* copy this > code > > over. > > Sorry, completely missed that. Yeah, so did I on my first pass, then the tests bopped me on the nose :-}.
Sorry, one other note on a piece of code you might or might not want to comment on. https://codereview.chromium.org/2971323002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1514: for (const auto& cookie : list) { I'll note that using |list| rather than |positive_diff| here makes me twitch, though I believe it's a faithful refactor. If you'd like me to try and tweak this code to make more sense, I'm willing, though as noted in a previous CL, getting rid of it would be a better (and much more time consuming) choice.
May do another pass today, may put it off for a day or two. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( On 2017/07/08 15:02:08, Randy Smith (Not in Mondays) wrote: > On 2017/07/07 16:54:58, mmenke wrote: > > Not suggesting it in this CL, but wonder if we could get rid of this > double-bind > > in the future by, for instance, having SetCookieWithDetails take + invoke the > > callback itself. > > I need a word for "Yeah, but if I go in that direction the slippery slope will > catch me and I'll end up at the bottom of the hill" :-}. > > So I thought about this and ended up with: > * I think this would work fine. > * It isn't needed for performance (at least according to CookieMonsterPerftest, > which is a completely reliable source of information on cookie performance :-z.) > * After some consideration I concluded that it would indeed be better in terms > of code size and comprehensibility. > * I started looking at the interfaces and realized that if I did this, > *CookieWith* and *CookieWith*Async would have the same signatures, which make me > think that then I should really combine those routines and conditionalize their > action based on the state of the cookie loading (which I think would be a win > for performance). > * I then realized that that mean I'd need to deal with reentrancy in the > callbacks, and PostTask the response if I wasn't actually queueing the callback > pending loading. > * I then threw up my hands and decided that going further than what I've done > wasn't actually what I wanted to do in this CL, and by proxy, in the immediate > or planned future. > > Let me know if you want to try and twist my arm, or if you want a TODO() > sketching out the above somewhere. But for the moment I'm disinclined to go in > this, perfectly reasonable, direction. I'm not going to twist your arm on this. Note that the current code (Both before and after this CL) already has to deal with re-entrancy. Callbacks for calls to non-delete methods can be called re-entrantly, and that's expected behavior. My concern isn't performance, but readability - I find nested binds very hard to follow, when they're using in anything other than a very basic post-task-and-reply-ish pattern (You can argue this pattern is somewhat similar, but I find it sufficiently different that it's confusing). I had not thought about merging the async / internal methods.
Not trying to convince you to do more cleanup - you're right that there's always a balance between cleanup and actually getting things done, just trying to intellectually figure out why I find the code hard to read. https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( On 2017/07/10 16:27:28, mmenke wrote: > On 2017/07/08 15:02:08, Randy Smith (Not in Mondays) wrote: > > On 2017/07/07 16:54:58, mmenke wrote: > > > Not suggesting it in this CL, but wonder if we could get rid of this > > double-bind > > > in the future by, for instance, having SetCookieWithDetails take + invoke > the > > > callback itself. > > > > I need a word for "Yeah, but if I go in that direction the slippery slope will > > catch me and I'll end up at the bottom of the hill" :-}. > > > > So I thought about this and ended up with: > > * I think this would work fine. > > * It isn't needed for performance (at least according to > CookieMonsterPerftest, > > which is a completely reliable source of information on cookie performance > :-z.) > > * After some consideration I concluded that it would indeed be better in terms > > of code size and comprehensibility. > > * I started looking at the interfaces and realized that if I did this, > > *CookieWith* and *CookieWith*Async would have the same signatures, which make > me > > think that then I should really combine those routines and conditionalize > their > > action based on the state of the cookie loading (which I think would be a win > > for performance). > > * I then realized that that mean I'd need to deal with reentrancy in the > > callbacks, and PostTask the response if I wasn't actually queueing the > callback > > pending loading. > > * I then threw up my hands and decided that going further than what I've done > > wasn't actually what I wanted to do in this CL, and by proxy, in the immediate > > or planned future. > > > > Let me know if you want to try and twist my arm, or if you want a TODO() > > sketching out the above somewhere. But for the moment I'm disinclined to go > in > > this, perfectly reasonable, direction. > > I'm not going to twist your arm on this. > > Note that the current code (Both before and after this CL) already has to deal > with re-entrancy. Callbacks for calls to non-delete methods can be called > re-entrantly, and that's expected behavior. > > My concern isn't performance, but readability - I find nested binds very hard to > follow, when they're using in anything other than a very basic > post-task-and-reply-ish pattern (You can argue this pattern is somewhat similar, > but I find it sufficiently different that it's confusing). > > I had not thought about merging the async / internal methods. Also note that in one of the delete case, we have 5 callbacks. We take in a callback, we bind two here, and we bind two more in WrapDeleteCallback, though the innermost bind isn't too complicated.
https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( On 2017/07/10 16:27:28, mmenke wrote: > On 2017/07/08 15:02:08, Randy Smith (Not in Mondays) wrote: > > On 2017/07/07 16:54:58, mmenke wrote: > > > Not suggesting it in this CL, but wonder if we could get rid of this > > double-bind > > > in the future by, for instance, having SetCookieWithDetails take + invoke > the > > > callback itself. > > > > I need a word for "Yeah, but if I go in that direction the slippery slope will > > catch me and I'll end up at the bottom of the hill" :-}. > > > > So I thought about this and ended up with: > > * I think this would work fine. > > * It isn't needed for performance (at least according to > CookieMonsterPerftest, > > which is a completely reliable source of information on cookie performance > :-z.) > > * After some consideration I concluded that it would indeed be better in terms > > of code size and comprehensibility. > > * I started looking at the interfaces and realized that if I did this, > > *CookieWith* and *CookieWith*Async would have the same signatures, which make > me > > think that then I should really combine those routines and conditionalize > their > > action based on the state of the cookie loading (which I think would be a win > > for performance). > > * I then realized that that mean I'd need to deal with reentrancy in the > > callbacks, and PostTask the response if I wasn't actually queueing the > callback > > pending loading. > > * I then threw up my hands and decided that going further than what I've done > > wasn't actually what I wanted to do in this CL, and by proxy, in the immediate > > or planned future. > > > > Let me know if you want to try and twist my arm, or if you want a TODO() > > sketching out the above somewhere. But for the moment I'm disinclined to go > in > > this, perfectly reasonable, direction. > > I'm not going to twist your arm on this. > > Note that the current code (Both before and after this CL) already has to deal > with re-entrancy. Callbacks for calls to non-delete methods can be called > re-entrantly, and that's expected behavior. Huh. Good point. Hold off on doing another review for a couple of days--I'll look at how much of a hassle merging the methods without worrying about reentrancy would be, and if I decide not to do that I'll take a couple of steps back and think about how I can make the current callback use less confusing. I completely agree I'm going a bit hog-wild with callbacks :-}. > > My concern isn't performance, but readability - I find nested binds very hard to > follow, when they're using in anything other than a very basic > post-task-and-reply-ish pattern (You can argue this pattern is somewhat similar, > but I find it sufficiently different that it's confusing). > > I had not thought about merging the async / internal methods.
https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:451: base::BindOnce( On 2017/07/11 15:47:07, Randy Smith (Not in Mondays) wrote: > On 2017/07/10 16:27:28, mmenke wrote: > > On 2017/07/08 15:02:08, Randy Smith (Not in Mondays) wrote: > > > On 2017/07/07 16:54:58, mmenke wrote: > > > > Not suggesting it in this CL, but wonder if we could get rid of this > > > double-bind > > > > in the future by, for instance, having SetCookieWithDetails take + invoke > > the > > > > callback itself. > > > > > > I need a word for "Yeah, but if I go in that direction the slippery slope > will > > > catch me and I'll end up at the bottom of the hill" :-}. > > > > > > So I thought about this and ended up with: > > > * I think this would work fine. > > > * It isn't needed for performance (at least according to > > CookieMonsterPerftest, > > > which is a completely reliable source of information on cookie performance > > :-z.) > > > * After some consideration I concluded that it would indeed be better in > terms > > > of code size and comprehensibility. > > > * I started looking at the interfaces and realized that if I did this, > > > *CookieWith* and *CookieWith*Async would have the same signatures, which > make > > me > > > think that then I should really combine those routines and conditionalize > > their > > > action based on the state of the cookie loading (which I think would be a > win > > > for performance). > > > * I then realized that that mean I'd need to deal with reentrancy in the > > > callbacks, and PostTask the response if I wasn't actually queueing the > > callback > > > pending loading. > > > * I then threw up my hands and decided that going further than what I've > done > > > wasn't actually what I wanted to do in this CL, and by proxy, in the > immediate > > > or planned future. > > > > > > Let me know if you want to try and twist my arm, or if you want a TODO() > > > sketching out the above somewhere. But for the moment I'm disinclined to go > > in > > > this, perfectly reasonable, direction. > > > > I'm not going to twist your arm on this. > > > > Note that the current code (Both before and after this CL) already has to deal > > with re-entrancy. Callbacks for calls to non-delete methods can be called > > re-entrantly, and that's expected behavior. > > Huh. Good point. > > Hold off on doing another review for a couple of days--I'll look at how much of > a hassle merging the methods without worrying about reentrancy would be, and if > I decide not to do that I'll take a couple of steps back and think about how I > can make the current callback use less confusing. I completely agree I'm going > a bit hog-wild with callbacks :-}. > > > > > My concern isn't performance, but readability - I find nested binds very hard > to > > follow, when they're using in anything other than a very basic > > post-task-and-reply-ish pattern (You can argue this pattern is somewhat > similar, > > but I find it sufficiently different that it's confusing). > > > > I had not thought about merging the async / internal methods. > Another option is to land this, and then do another cleanup pass. Or just land this and not do another cleanup pass. :) Anyhow, I'll defer to you. Just tell me when I should look at this again.
The CQ bit was checked by rdsmith@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_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@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 rdsmith@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_compile_dbg_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 rdsmith@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...
I'm going to call this good enough on try jobs to request another review. Two points: * I decided that your high level feedback of "The callback wrapping is confusing" was worth listening to. I tried to do the "combine and conditionalize" approach I sketched about above, but that failed because we need to keep queued and new tasks in-order, so we need to make a distinction between "Execute this after everything's loaded" and "Execute this now even though the queue bit is still on--I'm running the queue". So I kept the separate *Cookies() and *CookiesAsync() methods and just changed the signature of the non-Async methods to take a callback. I expected this to be simple, and I was, of course, wrong. Worse, there's still some callback wrapping magic (around conditionalizing the callbacks for is_null() and for CookieMonster going out of scope across store_->Flush()). But I do think it's clearer than it was. I welcome your thoughts, though I'm going to increase my bias towards minimal changes at this point :-}. * Part of what this refactor resulted in was yet more "Wow, SetAllCookies() odor is strong and all shall flee before it" aura. I kept myself to a pure refactor in this CL (I believe), but given that the spec for SetAllCookies *doesn't matter*, I indulged myself in some slash-and-burn in https://codereview.chromium.org/2974363002. (I'll call out that this wasn't intended to be a major change to semantics; specifically, the change in semantics implied by the comment edit was because the comment was actually incorrect before the change.) . Feel free to review those two CLs in whatever order and consider them as a unit or separately as you like.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:105: void ConditionalDeleteCallback(base::WeakPtr<net::CookieMonster> cookie_monster, Suggest renaming these so it's clearer they call callback, and are not callbacks themselves. ConditionallyCallFooCallback? InvokeFooCallback? MaybeRunFooCallback? https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:107: if (cookie_monster && !callback.is_null()) nit: For all of these, can just use callback instead of !callback.is_null(), I believe. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:118: T result) { If these two are called with strings, or cookies, will they do an extra copy? i.e., should these by const T& result (Or invoked with std::move) https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:750: return; return not needed https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:770: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:796: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:841: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:862: ConditionalCookieCallback(std::move(callback), cookie_line); Optional: Could combine the paths like you did with GetCookieListWithOptions. I don't have strong feelings about it, but may be nice to be consistent. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:863: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:872: // TODO(rdsmith): Would be good to provide a failure indication here. Could just make all delete callbacks report number of deleted callbacks. Then we'd have one fewer callback class, too. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:904: std::move(callback))); The next method has a null callback check, but this one does not. Is there a reason for that? ConditionalDeleteCallback also has its own null check. In general, I feel we should either consistently have null checks here, and not have one in ConditionalDeleteCallback, or vice versa. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:968: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1516: if (positive_diff.size() == 0) { This is old code, and I'm not asking you to fix it, but this seems weird: If positive_diff is 0, we do nothing, but if it's not, we re-add all cookies, including those already in the list. It seems like we could just add only the cookies that were not in the store before. Or is the issue garbage collecting non-expired cookies? In that case, it seems we should be unconditionally re-adding them to update their last access date. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1547: return; -return https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2022: ConditionalCookieCallback(std::move(callback)); I don't think these can be nullptr? https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2061: ConditionalCookieCallback(std::move(callback)); ditto https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster_perftest.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster_perftest.cc:101: class GetAllCookiesCallback : public BaseCallback { While you're here...would you mind renaming BaseCallback? Given that base::Callback is a thing, I was confused for a second there. MaybeWaitForResponseCallback? Or just CookieTestCallback? https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:170: std::string cookie_line, nit: const std::string& cookie_line
The CQ bit was checked by rdsmith@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...
Ok, there's enough green for me to ask for another round of review. PTAL? https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:105: void ConditionalDeleteCallback(base::WeakPtr<net::CookieMonster> cookie_monster, On 2017/07/12 18:29:04, mmenke wrote: > Suggest renaming these so it's clearer they call callback, and are not callbacks > themselves. ConditionallyCallFooCallback? InvokeFooCallback? > MaybeRunFooCallback? Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:107: if (cookie_monster && !callback.is_null()) On 2017/07/12 18:29:04, mmenke wrote: > nit: For all of these, can just use callback instead of !callback.is_null(), I > believe. Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:118: T result) { On 2017/07/12 18:29:04, mmenke wrote: > If these two are called with strings, or cookies, will they do an extra copy? > i.e., should these by const T& result (Or invoked with std::move) Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:750: return; On 2017/07/12 18:29:04, mmenke wrote: > return not needed Oy vais. Thank you. Fixed through file. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:770: return; On 2017/07/12 18:29:03, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:796: return; On 2017/07/12 18:29:04, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:841: return; On 2017/07/12 18:29:03, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:862: ConditionalCookieCallback(std::move(callback), cookie_line); On 2017/07/12 18:29:04, mmenke wrote: > Optional: Could combine the paths like you did with GetCookieListWithOptions. > I don't have strong feelings about it, but may be nice to be consistent. Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:863: return; On 2017/07/12 18:29:04, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:872: // TODO(rdsmith): Would be good to provide a failure indication here. On 2017/07/12 18:29:04, mmenke wrote: > Could just make all delete callbacks report number of deleted callbacks. Then > we'd have one fewer callback class, too. Perfectly reasonable suggestion, but not in this CL (I want to keep this a pure refactor) and hence probably not soon (I want to get back to moving forward on servicification). https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:904: std::move(callback))); On 2017/07/12 18:29:03, mmenke wrote: > The next method has a null callback check, but this one does not. Is there a > reason for that? ConditionalDeleteCallback also has its own null check. In > general, I feel we should either consistently have null checks here, and not > have one in ConditionalDeleteCallback, or vice versa. As per our offline conversation, put in a comment as to why this was different than the surrounding code. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:968: return; On 2017/07/12 18:29:04, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1516: if (positive_diff.size() == 0) { On 2017/07/12 18:29:03, mmenke wrote: > This is old code, and I'm not asking you to fix it, but this seems weird: > > If positive_diff is 0, we do nothing, but if it's not, we re-add all cookies, > including those already in the list. It seems like we could just add only the > cookies that were not in the store before. Or is the issue garbage collecting > non-expired cookies? In that case, it seems we should be unconditionally > re-adding them to update their last access date. As noted in the review request message, this function is a pile of crap, but as you note, it is a correctly refactored pile of crap. I intended to fix its problems (with a small tactical nuke) in https://codereview.chromium.org/2974363002, but I pushed the functional changes out to that CL. Consider the two CLs in combination and let me know if you want me to shift code from one to the other, or do something differently at the high level? https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1547: return; On 2017/07/12 18:29:04, mmenke wrote: > -return Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2022: ConditionalCookieCallback(std::move(callback)); On 2017/07/12 18:29:03, mmenke wrote: > I don't think these can be nullptr? Ah, good point. Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2061: ConditionalCookieCallback(std::move(callback)); On 2017/07/12 18:29:04, mmenke wrote: > ditto Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster_perftest.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster_perftest.cc:101: class GetAllCookiesCallback : public BaseCallback { On 2017/07/12 18:29:04, mmenke wrote: > While you're here...would you mind renaming BaseCallback? Given that > base::Callback is a thing, I was confused for a second there. > MaybeWaitForResponseCallback? Or just CookieTestCallback? Done. https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:170: std::string cookie_line, On 2017/07/12 18:29:05, mmenke wrote: > nit: const std::string& cookie_line Done.
LGTM! https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:872: // TODO(rdsmith): Would be good to provide a failure indication here. On 2017/07/12 19:47:34, Randy Smith (Not in Mondays) wrote: > On 2017/07/12 18:29:04, mmenke wrote: > > Could just make all delete callbacks report number of deleted callbacks. Then > > we'd have one fewer callback class, too. > > Perfectly reasonable suggestion, but not in this CL (I want to keep this a pure > refactor) and hence probably not soon (I want to get back to moving forward on > servicification). Sure, was not suggesting it for this CL. :)
On 2017/07/12 20:00:11, mmenke wrote: > LGTM! > > https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... > File net/cookies/cookie_monster.cc (right): > > https://codereview.chromium.org/2971323002/diff/120001/net/cookies/cookie_mon... > net/cookies/cookie_monster.cc:872: // TODO(rdsmith): Would be good to provide a > failure indication here. > On 2017/07/12 19:47:34, Randy Smith (Not in Mondays) wrote: > > On 2017/07/12 18:29:04, mmenke wrote: > > > Could just make all delete callbacks report number of deleted callbacks. > Then > > > we'd have one fewer callback class, too. > > > > Perfectly reasonable suggestion, but not in this CL (I want to keep this a > pure > > refactor) and hence probably not soon (I want to get back to moving forward on > > servicification). > > Sure, was not suggesting it for this CL. :) Awesome; thanks!!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@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": 140001, "attempt_start_ts": 1499895843809260,
"parent_rev": "35c4068a8eee7be339359983901b7dc3a023ca0f", "commit_rev":
"e5c701df767b131a3074131a886314e53a7dd0dd"}
Message was sent while issue was closed.
Description was changed from ========== Switch cookie async mechanism over to using callbacks. Switch mechanism for delaying cookie reuqests until the appropriate parts of the cookie database are loaded over to using callbacks instead of home-grown classes. BUG=None R=mmenke@chromium.org ========== to ========== Switch cookie async mechanism over to using callbacks. Switch mechanism for delaying cookie reuqests until the appropriate parts of the cookie database are loaded over to using callbacks instead of home-grown classes. BUG=None R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2971323002 Cr-Commit-Position: refs/heads/master@{#486105} Committed: https://chromium.googlesource.com/chromium/src/+/e5c701df767b131a3074131a8863... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e5c701df767b131a3074131a8863... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
