Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
859663003:
GetEffectiveDomain should handle ws scheme as same as http scheme. (Closed)
|
Created:
5 years, 11 months ago by yhirano Modified:
5 years, 10 months ago CC:
erikwright (departed), cbentzel+watch_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGetEffectiveDomain should handle ws scheme as same as http scheme.
CookieMonster::AnyEquivalentCookie asserts that there are no multiple
cookies that are identical to each other. That is achieved by inserting / deleting
cookies carefully, but it goes wrong with WebSocket schemes.
CookieMonster::DoCookieTaskForURL for given URL loads cookies when
cookies for key = cookie_util::GeteEffectiveDomain(scheme, host) is not
yet loaded. When the task ends, it stores loaded cookies and marks the key as
loaded. cookie_util::GetEffectiveDomain consults
egistry_controlled_domains::GetDomainAndRegistry
when http or https schemes are given, whereas it doesn't when ws or wss
schemes are given.
Imagine we are about to load stored cookies for ws://www.example.com/
and http://www.example.com/. As written above, they have different keys:
www.example.com and example.com. So each of them is loaded and it breaks
the assertion.
BUG=370021
R=ricea@chromium.org
Committed: https://crrev.com/5ca0f699ed11b59167acb32f3fa7c7480630bdcc
Cr-Commit-Position: refs/heads/master@{#313706}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 12
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 12
Patch Set 10 : #
Total comments: 8
Patch Set 11 : #
Messages
Total messages: 27 (3 generated)
https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (right): https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:34: void SetFindDomainTestGraph() { I don't think you can put this here. I think there is no alternative but to create a test_util.cc file for this function. I think you will have to copy the namespace test1 { ... } code from above as well. https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... File net/cookies/cookie_util_unittest.cc (right): https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... net/cookies/cookie_util_unittest.cc:206: net::test::registry_controlled_domains::SetFindDomainTestGraph(); This should go in CookieUtilDomainTest::SetUp() for symmetry with TearDown(). https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... net/cookies/cookie_util_unittest.cc:215: EXPECT_EQ("ac.jp", net::cookie_util::GetEffectiveDomain("foo", "ac.jp")); Suggestion: use "ftp" instead of "foo" here. I think it will make it clearer that we are testing the difference in handling of http/ws schemes and other schemes.
https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (right): https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:34: void SetFindDomainTestGraph() { On 2015/01/19 05:18:56, Adam Rice wrote: > I don't think you can put this here. > > I think there is no alternative but to create a test_util.cc file for this > function. I think you will have to copy the namespace test1 { ... } code from > above as well. Done. Unfortunately, test_util.cc is more strictly checked by presubmit than _unittests.cc. It reports that the include order is wrong, so I disable the check. https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... File net/cookies/cookie_util_unittest.cc (right): https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... net/cookies/cookie_util_unittest.cc:206: net::test::registry_controlled_domains::SetFindDomainTestGraph(); On 2015/01/19 05:18:56, Adam Rice wrote: > This should go in CookieUtilDomainTest::SetUp() for symmetry with TearDown(). Done. https://codereview.chromium.org/859663003/diff/1/net/cookies/cookie_util_unit... net/cookies/cookie_util_unittest.cc:215: EXPECT_EQ("ac.jp", net::cookie_util::GetEffectiveDomain("foo", "ac.jp")); On 2015/01/19 05:18:56, Adam Rice wrote: > Suggestion: use "ftp" instead of "foo" here. I think it will make it clearer > that we are testing the difference in handling of http/ws schemes and other > schemes. Done.
lgtm https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (right): https://codereview.chromium.org/859663003/diff/1/net/base/registry_controlled... net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:34: void SetFindDomainTestGraph() { On 2015/01/19 05:56:21, yhirano wrote: > On 2015/01/19 05:18:56, Adam Rice wrote: > > I don't think you can put this here. > > > > I think there is no alternative but to create a test_util.cc file for this > > function. I think you will have to copy the namespace test1 { ... } code from > > above as well. > > Done. > Unfortunately, test_util.cc is more strictly checked by presubmit than > _unittests.cc. It reports that the include order is wrong, so I disable the > check. I think that's okay. The check is not perfect, and this is a very strange case. https://codereview.chromium.org/859663003/diff/20001/net/base/registry_contro... File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (right): https://codereview.chromium.org/859663003/diff/20001/net/base/registry_contro... net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:6: #include "net/base/registry_controlled_domains/test_util.h" Please remove this too. https://codereview.chromium.org/859663003/diff/20001/net/cookies/cookie_util_... File net/cookies/cookie_util_unittest.cc (right): https://codereview.chromium.org/859663003/diff/20001/net/cookies/cookie_util_... net/cookies/cookie_util_unittest.cc:211: // unittests. Nit: It would be clearer to add the word "own", ie. "tested in its own unittests."
https://codereview.chromium.org/859663003/diff/20001/net/base/registry_contro... File net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (right): https://codereview.chromium.org/859663003/diff/20001/net/base/registry_contro... net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc:6: #include "net/base/registry_controlled_domains/test_util.h" On 2015/01/19 06:05:57, Adam Rice wrote: > Please remove this too. Done. https://codereview.chromium.org/859663003/diff/20001/net/cookies/cookie_util_... File net/cookies/cookie_util_unittest.cc (right): https://codereview.chromium.org/859663003/diff/20001/net/cookies/cookie_util_... net/cookies/cookie_util_unittest.cc:211: // unittests. On 2015/01/19 06:05:57, Adam Rice wrote: > Nit: It would be clearer to add the word "own", ie. "tested in its own > unittests." Done.
yhirano@chromium.org changed reviewers: + rdsmith@chromium.org
+rdsmith for OWNER review.
rdsmith@chromium.org changed reviewers: + rsleevi@chromium.org - rdsmith@chromium.org
I don't think I'm a good owner for this; I know nothing about WebSockets, and very little more about registry_controlled_domains/. Lateralling to rsleevi, who I think knows something about both.
1) I'm not keen on speculative fixes. Please explain why this fix is correct. 2) I'm not keen on exposing the test data, especially with conflicting namespaces and file hierarchy 3) I'm not keen on the overall pattern here; from the associated bug, and this speculative fix, it suggests that there may be some more endemic issues relating to handling cookies, and the description from the bug and this CL make me fear that they may not have been fully investigated. From reading RFC 6455, for example, I can't see any statements that connections share the same cookie stores, but then again, I'm no stranger to bad/underspecified behaviours.
Two small notes. On 2015/01/21 01:21:10, Ryan Sleevi wrote: > 1) I'm not keen on speculative fixes. Please explain why this fix is correct. This is at least an actual fix for crbug.com/423667. > From reading RFC 6455, for example, I can't see any statements that connections > share the same cookie stores, but then again, I'm no stranger to > bad/underspecified behaviours. Cookie behaviour with regards to WebSockets is not specced anywhere, due to disagreements between spec editors. The de-facto standard behaviour is to treat it like HTTP.
On 2015/01/21 01:35:17, Adam Rice wrote: > This is at least an actual fix for crbug.com/423667. Awesome. Can you note that in the description? In particular, if you think that this is related to 370021, can you please actually add a test that fails without this fix and passes with? That's one of the things I was aluding to with "not fully investigating" the issue - the unit test doesn't seem at all related to the bug noted. > Cookie behaviour with regards to WebSockets is not specced anywhere, due to > disagreements between spec editors. > > The de-facto standard behaviour is to treat it like HTTP. This is... unfortunate. Exceedingly. However, if this is the conclusion, it seems we are certainly lacking in comprehensive unit tests that ensure this behaviour is both correct and doesn't regress. At the risk of being a pain in the review, but in the Googly/Chromie spirit of "leave things better than when you found them", having more tests that explicitly test the WebSocket+cookie behaviour are expected (and don't regress) seem critical for me feeling comfortable with this review. For example, I have no idea if, immediately above this code, something else does something "unexpected" that causes some new subtle error. Having unit tests helps me feel confident that we aren't introducing new bugs.
Wrote how the current impl causes https://crbug.com/370021.
On 2015/01/21 04:03:57, yhirano wrote: > Wrote how the current impl causes https://crbug.com/370021. Unit tests that fail under the old code and pass in the new? ;) And it seems like an integration test at a higher layer surely should have caught this, right? That is, I suspect there need to be tests at the CookieMonster level and perhaps at the URLRequest level all asserting basic sanity checks that ws:// is indeed handled the same as http:// with respect to cookies, and that wss:// implications are also properly considered (e.g. SecureOnly cookies). In case it's not clear what I'm asking: I think you've indeed spotted a bug between assumption and implementation. However, the fix - and the tests added - suggest that many of the other assumptions are either untested or not tested sufficiently, which is how this bug could exist (for so long?). While you're here in the code, I'm asking you to use this bug as a concrete opportunity to improve the surrounding tests, since it provides a very reasonable path to demonstrate an issue from the lowest to the highest layers.
Added a CookieMonster level unit test. WebSocket started to use the http stack half a year ago, and we still have some inconsistency, such as https://crbug.com/id=446459, and we are fixing them. In terms of end-to-end test, Adam has already started working at https://crbug.com/441709, so we are going to be able to write such tests. I want to post this CL and see if it fixes the crash, as I couldn't reproduce the crash in my local environment.
the //net/base/registry_controlled_domain changes do NOT look good. You should not be coupling your test at these layers. If you want to test with a custom DAFSA, //net already exposes the function for tests to do so - you should use a custom .gperf / .cc file for your test. There's still structural issues with the tests, it seems; documentation is lacking as to what you're expecting to happen and why you expect it to happen. You also still do not seem to be testing the cookie monster behaviour. Before I can LG a change, I would like to see a regression test for this bug added. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2616: // This cookie is freed by the CookieMonster. is? or will be? https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2619: CanonicalCookie cookie = *cookies[0]; Did you mean to copy? If so, why? https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2625: ::testing::StrictMock<::testing::MockFunction<void(int)>> checkpoint; This seems unnecessary/overkill. The relation of checkpoint to the rest of this test is not at all obvious. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2626: const std::string key = "google.izzle"; What is this magic string and why are you using it? https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2644: loaded_callback.Run(cookies); This is a bit surprising and non-obvious. Needs better documentation, but I wonder if this can't be eliminated entirely. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2677: } The documentation for each of these assertions is very poor, and it's not easy to read what the test is testing. Please document, in a single sentence, the assertions being made here.
> the //net/base/registry_controlled_domain changes do NOT look good. I found using custom PSL data is not needed. Fixed. > There's still structural issues with the tests, it seems; documentation is lacking as to what you're expecting to happen and why you expect it to happen. > You also still do not seem to be testing the cookie monster behaviour. I don't understand. The CookieMonsterTest does test CookieMonster's behavior (and it fails without the change, passes with the change). I added WebSocketStreamCreateTests in case, though they don't fail even without this change, because we need to control the loading timing to reproduce the issue (and that's I did in the CookieMonsterTest). > Before I can LG a change, I would like to see a regression test for this bug added. Again, I don't understand. What do you mean by "regression test"? Both two tests I added before fail without the change and pass with the change. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2616: // This cookie is freed by the CookieMonster. On 2015/01/22 01:43:41, Ryan Sleevi wrote: > is? or will be? Done. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2619: CanonicalCookie cookie = *cookies[0]; On 2015/01/22 01:43:41, Ryan Sleevi wrote: > Did you mean to copy? If so, why? I want to have a cookie to verify the result below. I don't want to have a raw pointer which can be destructed by the CookiMonster at any time. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2625: ::testing::StrictMock<::testing::MockFunction<void(int)>> checkpoint; On 2015/01/22 01:43:41, Ryan Sleevi wrote: > This seems unnecessary/overkill. The relation of checkpoint to the rest of this > test is not at all obvious. I want to check that all monitored calls happens between the checkpoint calls. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2626: const std::string key = "google.izzle"; On 2015/01/22 01:43:41, Ryan Sleevi wrote: > What is this magic string and why are you using it? Done. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2644: loaded_callback.Run(cookies); On 2015/01/22 01:43:41, Ryan Sleevi wrote: > This is a bit surprising and non-obvious. Needs better documentation, but I > wonder if this can't be eliminated entirely. It is necessary. By calling loaded_callback, we notify the CookieMonster that LoadCookiesForKey task is done. https://codereview.chromium.org/859663003/diff/80001/net/cookies/cookie_monst... net/cookies/cookie_monster_unittest.cc:2677: } On 2015/01/22 01:43:41, Ryan Sleevi wrote: > The documentation for each of these assertions is very poor, and it's not easy > to read what the test is testing. Please document, in a single sentence, the > assertions being made here. Done.
As I plan to merge this fix to branches, I want it be as small as possible. Given that WebSocket unit tests pass without the change, I would like to decouple it if you are fine with it https://codereview.chromium.org/869073002/.
Thanks to Adam, I added failing WebSocket tests on https://codereview.chromium.org/869073002/. The tests pass with this fix.
Thanks for continuing to bear with me. It was not clear from your comments that the buggy behaviour reproduced with the test. I have mostly comment/documentation nits, but I will LGTM this now with the expectation you'll address them. I'm still deeply concerned about the E2E tests, but I see you're working on those tests, so I'm happy to let this through and continue the discussion on the follow-up review. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2614: TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) { Can you add a description to this test explaining what you're testing and why? "Ensure that cookies for http, https, ws, and wss all share the same storage and policies" or something (probably better worded) that specifically explains why you're testing these. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2644: checkpoint.Call(1); I'm confused; Why is this in a separate section than 2629-2638? Why are these not combined with a set of expectations? That is, from a 'naive' reading, I expect that 2630-2638 independently executes and will fail, but from what I can tell that this test is doing, the actual expectations are satisfied here (in 2640-2652). Is this correct? Why the different blocks? https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2645: EXPECT_FALSE(callback.did_run()); Historically, tests with EXPECTS on callback running may hang when these conditions are violated, because the state machine gets all confused. Should these (this, line 2649, 2659, 2668, 2677, 2687) all be ASSERT_TRUE, to ensure that the state machine is in a correct state before we run off the deep end? https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2655: // Call the function with the same URL and see if it is done synchronously s/see if it/verify it/ The wording suggests it's "optional" to be synchronous, but your expectation is that the implementation always will. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2690: } Should this code be refactored to a loop over the 4 GURLs and ensure that they're each the same? Seems to avoid the copy/paste.
https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2614: TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) { On 2015/01/24 03:46:59, Ryan Sleevi wrote: > Can you add a description to this test explaining what you're testing and why? > > "Ensure that cookies for http, https, ws, and wss all share the same storage and > policies" or something (probably better worded) that specifically explains why > you're testing these. Done. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2644: checkpoint.Call(1); On 2015/01/24 03:46:59, Ryan Sleevi wrote: > I'm confused; Why is this in a separate section than 2629-2638? Why are these > not combined with a set of expectations? > > That is, from a 'naive' reading, I expect that 2630-2638 independently executes > and will fail, but from what I can tell that this test is doing, the actual > expectations are satisfied here (in 2640-2652). Is this correct? Why the > different blocks? (All line numbers represents line numbers in PS9 in this comment.) I like separating EXPECT_CALLs from other code, because we can see the expectations more clearly. In this case, we can see easily that call expectations ends at L2638. Actually, L2629-L2638 does not correspond to L2640-L2653. The call expectations include the expectation that LoadCookiesForKey will never be called again (L2636) which corresponds to L2654-L2690. So I don't think it's reasonable to conflate L2629-L2638 block with L2640-L2653 block, though I'm open to de-block L2629-L2638 if you prefer it strongly. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2645: EXPECT_FALSE(callback.did_run()); On 2015/01/24 03:46:59, Ryan Sleevi wrote: > Historically, tests with EXPECTS on callback running may hang when these > conditions are violated, because the state machine gets all confused. > > Should these (this, line 2649, 2659, 2668, 2677, 2687) all be ASSERT_TRUE, to > ensure that the state machine is in a correct state before we run off the deep > end? Done. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2655: // Call the function with the same URL and see if it is done synchronously On 2015/01/24 03:46:59, Ryan Sleevi wrote: > s/see if it/verify it/ > > The wording suggests it's "optional" to be synchronous, but your expectation is > that the implementation always will. Done. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2690: } On 2015/01/24 03:46:59, Ryan Sleevi wrote: > Should this code be refactored to a loop over the 4 GURLs and ensure that > they're each the same? Seems to avoid the copy/paste. Done.
LGTM functionally, although still nits on structure. https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2644: checkpoint.Call(1); On 2015/01/26 04:45:25, yhirano wrote: > (All line numbers represents line numbers in PS9 in this comment.) [Same in mine] > I like separating EXPECT_CALLs from other code, because we can see the > expectations more clearly. In this case, we can see easily that call > expectations ends at L2638. But that's what doesn't make sense. At no point does |checkpoint| get bound to anything (compare line 2625 with the rest of the code in 2329-2836). That is, I read that block of code, and I see nothing that tells me why I should expect |checkpoint| to be called, because it's not used for anything. However, when I look at 2641-2644, I do see |checkpoint| being called, which does satisfy all of the expectations setup in 2630-2638, and thus it's non-intuitive why that sequence block is in it's own block. It further gets confusing when considering lines like 2658, which also call GetAllCookiesForURLTask, but don't call checkpoint. It creates an asymmetry in the test between the two blocks that is non-intuitive. That's what I meant about this test being confusing. > Actually, L2629-L2638 does not correspond to L2640-L2653. The call expectations > include the expectation that LoadCookiesForKey will never be called again > (L2636) which corresponds to L2654-L2690. > So I don't think it's reasonable to conflate L2629-L2638 block with L2640-L2653 > block, though I'm open to de-block L2629-L2638 if you prefer it strongly. > Attempting to put it differently, it's not clear to me, reading this CL, how 2629-2638 relate to the rest of the test, as indicated by the structure of the code. The best I can take from reading this code (short of patching into my local repo and stepping through with a debugger) is to guess that it relates to assertions for 2640-2653, but you've said no, it doesn't, and it's not reasonable to conflate the two. So what does this block do? And if it is related, then it's confusing the asymmetry between what's being tested in 2640-2652 and the remaining blocks that are doing testing. Nor is it obvious why you've even set up each of these tests as individual blocks of code, when either they're part of the same test (and thus should all be a single block) or they would be better off as unique, individual tests testing individual assertions with no sharing of state. As best I can tell, the only reason you did this is so that you can share variable names? I think the new structure of the code helps greatly (lines 2658-2674 of the new patchset), but I think it's still confusing how lines 2633-2657 of the new patchset are related to eachother and the rest of the test. In the least, comments to help guide an understanding will help here, but I also question if you can't go further by ensuring both comment and structure help guide the reader. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2615: // and policies, when GetAllCookiesForURLAsync is used. This test is located grammar nit: no comma between policies and when. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2617: // asynchronously, but doesn't use a separate thread in spite of the name. grammar nit: I found this second sentence a bit confusing. Perhaps reworded as: This test is part of MultiThreadedCookieMonsterTest in order to test and use GetAllCookiesForURLAsync, but it does not use any additional threads. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2659: // All urls in |urls| share the same cookie domain. s/share/should share/ https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2660: const GURL urls[] = { nit NAMING: s/urls/kUrls/
https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/160001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2644: checkpoint.Call(1); > As best I can tell, the only reason you did this is so that you can share variable names? Yes. I created blocks L2640-L2653, L2654-L2662, L2663-L2671, L2672-L2680, L2681-L2690 because I wanted to use different |callback| instances with the same name (As I said before, I created L2629-L2638 block for a different reason). Now latter four blocks are combined and we can de-block the first one. I did so in PS11. I also moved call expectations out of the block in PS11 and put EXPECT_CALL(...).Times(0) as you don't see the expectation's relevance to latter code blocks although EXPECT_CALL(...).Times(0) does not have a practical meaning. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2615: // and policies, when GetAllCookiesForURLAsync is used. This test is located On 2015/01/26 19:49:54, Ryan Sleevi wrote: > grammar nit: no comma between policies and when. Done. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2617: // asynchronously, but doesn't use a separate thread in spite of the name. On 2015/01/26 19:49:54, Ryan Sleevi wrote: > grammar nit: I found this second sentence a bit confusing. Perhaps reworded as: > > This test is part of MultiThreadedCookieMonsterTest in order to test and use > GetAllCookiesForURLAsync, but it does not use any additional threads. Done. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2659: // All urls in |urls| share the same cookie domain. On 2015/01/26 19:49:54, Ryan Sleevi wrote: > s/share/should share/ Done. https://codereview.chromium.org/859663003/diff/180001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:2660: const GURL urls[] = { On 2015/01/26 19:49:54, Ryan Sleevi wrote: > nit NAMING: s/urls/kUrls/ Done.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/859663003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5ca0f699ed11b59167acb32f3fa7c7480630bdcc Cr-Commit-Position: refs/heads/master@{#313706} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
