GetEffectiveDomain should handle ws scheme as same as http scheme.

net/cookies/cookie_monster_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cookies/cookie_store_unittest.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 2593 matching lines @@
   it = cookies.begin();
   base::Closure task = base::Bind(
       &net::MultiThreadedCookieMonsterTest::DeleteCanonicalCookieTask,
       base::Unretained(this),
       cm, *it, &callback);
   RunOnOtherThread(task);
   EXPECT_TRUE(callback.did_run());
   EXPECT_TRUE(callback.result());
 }
 
+TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) {

[Ryan Sleevi, 2015/01/24 03:46:59]

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.

[yhirano, 2015/01/26 04:45:25]

Done.

+  std::vector<CanonicalCookie*> cookies;
+  // This cookie will be freed by the CookieMonster.
+  cookies.push_back(CanonicalCookie::Create(url_google_, kValidCookieLine,
+                                            Time::Now(), CookieOptions()));
+  CanonicalCookie cookie = *cookies[0];
+  scoped_refptr<NewMockPersistentCookieStore> store(
+      new NewMockPersistentCookieStore);
+  scoped_refptr<CookieMonster> cm(new CookieMonster(store.get(), NULL));
+
+  CookieMonster::PersistentCookieStore::LoadedCallback loaded_callback;
+  ::testing::StrictMock<::testing::MockFunction<void(int)>> checkpoint;
+  const std::string key =
+      cookie_util::GetEffectiveDomain(url_google_.scheme(), url_google_.host());
+
+  {
+    ::testing::InSequence s;
+    EXPECT_CALL(checkpoint, Call(0));
+    EXPECT_CALL(*store, Load(::testing::_));
+    EXPECT_CALL(*store, LoadCookiesForKey(key, ::testing::_))
+        .WillOnce(::testing::SaveArg<1>(&loaded_callback));
+    EXPECT_CALL(checkpoint, Call(1));
+    // LoadCookiesForKey will never be called, because all URLS below share
+    // the same key.
+  }
+
+  {
+    GetCookieListCallback callback;
+    checkpoint.Call(0);
+    GetAllCookiesForURLTask(cm.get(), url_google_, &callback);
+    checkpoint.Call(1);

[Ryan Sleevi, 2015/01/24 03:46:59]

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?

[yhirano, 2015/01/26 04:45:25]

(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.

[Ryan Sleevi, 2015/01/26 19:49:54]

[Same in mine]
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.
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.

[yhirano, 2015/01/27 02:07:10]

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.

+    EXPECT_FALSE(callback.did_run());

[Ryan Sleevi, 2015/01/24 03:46:59]

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?

[yhirano, 2015/01/26 04:45:25]

Done.

+    // Pass the cookies to the CookieMonster.
+    loaded_callback.Run(cookies);
+    // Now GetAllCookiesForURLTask is done.
+    EXPECT_TRUE(callback.did_run());
+    // See that the callback was called with the cookies.
+    ASSERT_EQ(1u, callback.cookies().size());
+    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  }
+  {
+    // Call the function with the same URL and see if it is done synchronously

[Ryan Sleevi, 2015/01/24 03:46:59]

s/see if it/verify it/

The wording suggests it's "optional" to be synchronous, but your expectation is
that the implementation always will.

[yhirano, 2015/01/26 04:45:25]

Done.

+    // without calling LoadCookiesForKey.
+    GetCookieListCallback callback;
+    GetAllCookiesForURLTask(cm.get(), url_google_, &callback);
+    EXPECT_TRUE(callback.did_run());
+    ASSERT_EQ(1u, callback.cookies().size());
+    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  }
+  {
+    // Call the function with an https URL and see if it is done synchronously
+    // without calling LoadCookiesForKey.
+    GetCookieListCallback callback;
+    GetAllCookiesForURLTask(cm.get(), url_google_secure_, &callback);
+    EXPECT_TRUE(callback.did_run());
+    ASSERT_EQ(1u, callback.cookies().size());
+    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  }
+  {
+    // Call the function with a ws URL and see if it is done synchronously
+    // without calling LoadCookiesForKey.
+    GetCookieListCallback callback;
+    GetAllCookiesForURLTask(cm.get(), GURL(kUrlGoogleWebSocket), &callback);
+    EXPECT_TRUE(callback.did_run());
+    ASSERT_EQ(1u, callback.cookies().size());
+    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  }
+  {
+    // Call the function with a wss URL and see if it is done synchronously
+    // without calling LoadCookiesForKey.
+    GetCookieListCallback callback;
+    GetAllCookiesForURLTask(cm.get(), GURL(kUrlGoogleWebSocketSecure),
+                            &callback);
+    EXPECT_TRUE(callback.did_run());
+    ASSERT_EQ(1u, callback.cookies().size());
+    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  }

[Ryan Sleevi, 2015/01/24 03:46:59]

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.

[yhirano, 2015/01/26 04:45:25]

Done.

+}
+
 TEST_F(CookieMonsterTest, InvalidExpiryTime) {
   std::string cookie_line =
       std::string(kValidCookieLine) + "; expires=Blarg arg arg";
   scoped_ptr<CanonicalCookie> cookie(
       CanonicalCookie::Create(url_google_, cookie_line, Time::Now(),
                               CookieOptions()));
   ASSERT_FALSE(cookie->IsPersistent());
 }
 
 // Test that CookieMonster writes session cookies into the underlying
@@ skipping 255 matching lines @@
   scoped_ptr<CookieStore::CookieChangedSubscription> sub1(
       monster()->AddCallbackForCookie(test_url_, "abc",
           base::Bind(&RecordCookieChanges, &cookies1, nullptr)));
   SetCookie(monster(), test_url_, "abc=def");
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(1U, cookies0.size());
   EXPECT_EQ(1U, cookies0.size());
 }
 
 }  // namespace net