chrome/browser/net/sdch_browsertest.cc - Issue 380003002: Improve testing for SDCH.

Side by Side Diff: chrome/browser/net/sdch_browsertest.cc

Issue 380003002: Improve testing for SDCH. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Fixed braino: Added SDHC enable to the tests that need it. Created 6 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

OLD	NEW
(Empty)
	1 // Copyright 2014 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 // End-to-end SDCH tests. Uses the embedded test server to return SDCH

	6 // results

	7

	8 #include "base/base64.h"

	9 #include "base/bind.h"

	10 #include "base/callback.h"

	11 #include "base/command_line.h"

	12 #include "base/files/scoped_temp_dir.h"

	13 #include "base/memory/weak_ptr.h"

	14 #include "base/path_service.h"

	15 #include "base/run_loop.h"

	16 #include "base/strings/string_util.h"

	17 #include "base/strings/stringprintf.h"

	18 #include "chrome/browser/browser_process.h"

	19 #include "chrome/browser/browsing_data/browsing_data_helper.h"

	20 #include "chrome/browser/browsing_data/browsing_data_remover.h"

	21 #include "chrome/browser/browsing_data/browsing_data_remover_test_util.h"

	22 #include "chrome/browser/profiles/profile.h"

	23 #include "chrome/browser/profiles/profile_manager.h"

	24 #include "chrome/browser/ui/browser.h"

	25 #include "chrome/browser/ui/browser_tabstrip.h"

	26 #include "chrome/browser/ui/browser_window.h"

	27 #include "chrome/browser/ui/tabs/tab_strip_model.h"

	28 #include "chrome/common/chrome_paths.h"

	29 #include "chrome/test/base/in_process_browser_test.h"

	30 #include "content/public/browser/browser_thread.h"

	31 #include "content/public/common/content_switches.h"

	32 #include "content/public/test/browser_test_utils.h"

	33 #include "content/public/test/test_utils.h"

	34 #include "crypto/sha2.h"

	35 #include "net/base/sdch_manager.h"

	36 #include "net/http/http_response_headers.h"

	37 #include "net/test/embedded_test_server/embedded_test_server.h"

	38 #include "net/test/embedded_test_server/http_request.h"

	39 #include "net/test/embedded_test_server/http_response.h"

	40 #include "net/url_request/url_fetcher.h"

	41 #include "net/url_request/url_fetcher_delegate.h"

	42 #include "net/url_request/url_request_context.h"

	43 #include "net/url_request/url_request_context_getter.h"

	44 #include "sdch/open-vcdiff/src/google/vcencoder.h"

	45 #include "testing/gtest/include/gtest/gtest.h"

	46

	47 namespace {

	48

	49 typedef std::vector<net::test_server::HttpRequest> RequestVector;

	50 typedef std::map<std::string, std::string> HttpRequestHeaderMap;

	51

	52 // Credit Alfred, Lord Tennyson

	53 static const char kSampleData[] = "<html><body><pre>"

	54 "There lies the port; the vessel puffs her sail:\n"

	55 "There gloom the dark, broad seas. My mariners,\n"

	56 "Souls that have toil'd, and wrought, and thought with me—\n"

	57 "That ever with a frolic welcome took\n"

	58 "The thunder and the sunshine, and opposed\n"

	59 "Free hearts, free foreheads—you and I are old;\n"

	60 "Old age hath yet his honour and his toil;\n"

	61 "Death closes all: but something ere the end,\n"

	62 "Some work of noble note, may yet be done,\n"

	63 "Not unbecoming men that strove with Gods.\n"

	64 "The lights begin to twinkle from the rocks:\n"

	65 "The long day wanes: the slow moon climbs: the deep\n"

	66 "Moans round with many voices. Come, my friends,\n"

	67 "'T is not too late to seek a newer world.\n"

	68 "Push off, and sitting well in order smite\n"

	69 "The sounding furrows; for my purpose holds\n"

	70 "To sail beyond the sunset, and the baths\n"

	71 "Of all the western stars, until I die.\n"

	72 "It may be that the gulfs will wash us down:\n"

	73 "It may be we shall touch the Happy Isles,\n"

	74 "And see the great Achilles, whom we knew.\n"

	75 "Tho' much is taken, much abides; and tho'\n"

	76 "We are not now that strength which in old days\n"

	77 "Moved earth and heaven, that which we are, we are;\n"

	78 "One equal temper of heroic hearts,\n"

	79 "Made weak by time and fate, but strong in will\n"

	80 "To strive, to seek, to find, and not to yield.\n"

	81 "</pre></body></html>";

	82

	83 // Random selection of lines from above, to allow some encoding, but

	84 // not a trivial encoding.

	85 static const char kDictionaryContents[] =

	86 "The thunder and the sunshine, and opposed\n"

	87 "To sail beyond the sunset, and the baths\n"

	88 "Of all the western stars, until I die.\n"

	89 "Made weak by time and fate, but strong in will\n"

	90 "Moans round with many voices. Come, my friends,\n"

	91 "The lights begin to twinkle from the rocks:";

	92

	93 // Scanns in a case-insensitive way for \|header\| in \|map\|,
	mef 2014/07/23 18:34:28 nit: Scanns nit: Scanns Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done. Show quoted text On 2014/07/23 18:34:28, mef wrote: > nit: Scanns Done.
	94 // returning true if found and setting \|*value\| to the value

	95 // of that header. Does not handle multiple instances of the same

	96 // header.

	97 bool GetRequestHeader(const HttpRequestHeaderMap& map,

	98 const char* header,
	mef 2014/07/23 18:34:29 nit: alignment nit: alignment Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done. Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: alignment Done.
	99 std::string* value) {

	100 for (HttpRequestHeaderMap::const_iterator it = map.begin();

	101 it != map.end(); ++it) {

	102
	jar (doing other things) 2014/07/31 03:24:34 nit: minimize vertical whitespace... and I doubt nit: minimize vertical whitespace... and I doubt it helps here. Randy Smith (Not in Mondays) 2014/08/11 20:33:24 Yeah, typo. Done. Show quoted text On 2014/07/31 03:24:34, jar wrote: > nit: minimize vertical whitespace... and I doubt it helps here. Yeah, typo. Done.
	103 if (!base::strcasecmp(it->first.c_str(), header)) {

	104 *value = it->second;

	105 return true;

	106 }

	107 }

	108 return false;

	109 }

	110

	111 // Do a URL-safe base64 encoding. See the SDCH spec "Dictionary Identifier"

	112 // section, and RFC 3548 section 4.

	113 void SafeBase64Encode(const std::string& input_value, std::string* output) {

	114 DCHECK(output);

	115 base::Base64Encode(input_value, output);

	116 for (size_t i = 0; i < output->size(); ++i) {
	mef 2014/07/23 18:34:28 It seems that common practice is to use std::repla It seems that common practice is to use std::replace, like in: std::replace(output->begin(), output->end(), '+', '-'); std::replace(output->begin(), output->end(), '/', '_'); Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done, both here and in source copy location. Neat Show quoted text On 2014/07/23 18:34:28, mef wrote: > It seems that common practice is to use std::replace, like in: > > std::replace(output->begin(), output->end(), '+', '-'); > std::replace(output->begin(), output->end(), '/', '_'); Done, both here and in source copy location. Neat.
	117 if ((*output)[i] == '+')

	118 (*output)[i] = '-';

	119 if ((*output)[i] == '/')

	120 (*output)[i] = '_';

	121 }

	122 }

	123

	124 bool ResponseContainsHeaderValue(const net::HttpResponseHeaders& headers,
	mef 2014/07/23 18:34:28 Seems the same as HttpResponseHeaders::HasHeaderVa Seems the same as HttpResponseHeaders::HasHeaderValue. Randy Smith (Not in Mondays) 2014/07/29 23:44:20 Thanks for the catch. Done. Show quoted text On 2014/07/23 18:34:28, mef wrote: > Seems the same as HttpResponseHeaders::HasHeaderValue. Thanks for the catch. Done.
	125 const char* header,

	126 const char* value) {

	127 std::string iterated_value;

	128 void *iter = NULL;

	129 while (headers.EnumerateHeader(&iter, header, &iterated_value)) {

	130 if (!base::strcasecmp(iterated_value.c_str(), value))

	131 return true;

	132 }

	133 return false;

	134 }

	135

	136 // Class that bundles responses for an EmbeddedTestServer().

	137 // Dictionary is at <domain>/dict, data at <domain>/data.

	138 // The data is sent SDCH encoded if that's allowed by protoocol.

	139 class SdchResponseHandler {

	140 public:

	141 // Do initial preparation so that SDCH requests can be handled.

	142 explicit SdchResponseHandler(std::string domain)

	143 : cache_sdch_response_(false),

	144 weak_ptr_factory_(this) {

	145 // Dictionary

	146 sdch_dictionary_contents_ = "Domain: ";

	147 sdch_dictionary_contents_ += domain;

	148 sdch_dictionary_contents_ += "\n\n";

	149 sdch_dictionary_contents_ += kDictionaryContents;

	150

	151 // Dictionary hash for client and server.

	152 char binary_hash[32];

	153 crypto::SHA256HashString(sdch_dictionary_contents_, binary_hash,

	154 sizeof(binary_hash));

	155 SafeBase64Encode(std::string(&binary_hash[0], 6), &dictionary_client_hash_);

	156 SafeBase64Encode(std::string(&binary_hash[6], 6), &dictionary_server_hash_);

	157

	158 // Encoded response.

	159 open_vcdiff::HashedDictionary vcdiff_dictionary(

	160 kDictionaryContents, strlen(kDictionaryContents));

	161 bool result = vcdiff_dictionary.Init();

	162 DCHECK(result);

	163 open_vcdiff::VCDiffStreamingEncoder encoder(&vcdiff_dictionary, 0, false);

	164 encoded_data_ = dictionary_server_hash_;

	165 encoded_data_ += '\0';

	166 result = encoder.StartEncoding(&encoded_data_);

	167 DCHECK(result);

	168 result = encoder.EncodeChunk(

	169 kSampleData, strlen(kSampleData), &encoded_data_);

	170 DCHECK(result);

	171 result = encoder.FinishEncoding(&encoded_data_);

	172 DCHECK(result);

	173 }

	174

	175 static bool ClientIsAdvertisingSdchEncoding(const HttpRequestHeaderMap& map) {

	176 std::string value;

	177 if (!GetRequestHeader(map, "accept-encoding", &value))

	178 return false;

	179 std::string::iterator word_start = value.begin();

	180 // Scan comma separated list of encodings for case insensitive match
	mef 2014/07/23 18:34:29 Suggest: Use base/strings/string_tokenizer.h Suggest: Use base/strings/string_tokenizer.h Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Major advantage of code reviews; learning new tech Show quoted text On 2014/07/23 18:34:29, mef wrote: > Suggest: Use base/strings/string_tokenizer.h Major advantage of code reviews; learning new techniques :-}. Done. mef 2014/07/30 17:14:58 It is mutual, I've learned about it doing the revi Show quoted text On 2014/07/29 23:44:21, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > Suggest: Use base/strings/string_tokenizer.h > > Major advantage of code reviews; learning new techniques :-}. Done. It is mutual, I've learned about it doing the review. :)
	181 // for "sdch".

	182 do {

	183 std::string::iterator it(word_start);

	184 while (it != value.end() && IsAsciiAlpha(*it))

	185 ++it;

	186 if (it != word_start) {

	187 if (!base::strcasecmp(std::string(word_start,it).c_str(), "sdch"))

	188 return true;

	189 }

	190 while (it != value.end() && !IsAsciiAlpha(*it))

	191 ++it;

	192 word_start = it;

	193 } while (word_start != value.end());

	194 return false;

	195 }

	196

	197 bool ShouldRespondWithSdchEncoding(const HttpRequestHeaderMap& map) {

	198 std::string value;

	199 if (!GetRequestHeader(map, "avail-dictionary", &value))

	200 return false;

	201 return value == dictionary_client_hash_;

	202 }

	203

	204 scoped_ptr<net::test_server::HttpResponse> HandleRequest(

	205 const net::test_server::HttpRequest& request) {

	206 request_vector_.push_back(request);

	207

	208 scoped_ptr<net::test_server::BasicHttpResponse> response(

	209 new net::test_server::BasicHttpResponse);

	210 if (request.relative_url == "/data") {

	211 if (ShouldRespondWithSdchEncoding(request.headers)) {

	212 // Note that chrome doesn't advertise accepting SDCH encoding

	213 // for POSTs (because the meta-refresh hack would break a POST),

	214 // but that's not for the server to enforce.

	215 DCHECK_NE(encoded_data_, "");
	mef 2014/07/23 18:34:29 Given that this is a unit test, should DCHECKs be Given that this is a unit test, should DCHECKs be ASSERTs or EXPECTs? Randy Smith (Not in Mondays) 2014/07/29 23:44:21 My take was that if I was making statements about Show quoted text On 2014/07/23 18:34:29, mef wrote: > Given that this is a unit test, should DCHECKs be ASSERTs or EXPECTs? My take was that if I was making statements about chrome code, they should be ASSERT/EXPECT, but if I was making statements about other test code in this file they should be DCHECKs. Let me know if that' snot ok. mef 2014/07/30 17:14:58 Acknowledged. Show quoted text On 2014/07/29 23:44:21, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > Given that this is a unit test, should DCHECKs be ASSERTs or EXPECTs? > > My take was that if I was making statements about chrome code, they should be > ASSERT/EXPECT, but if I was making statements about other test code in this file > they should be DCHECKs. Let me know if that' snot ok. Acknowledged.
	216 response->set_content_type("text/html");

	217 response->set_content(encoded_data_);

	218 response->AddCustomHeader("Content-Encoding", "sdch");

	219 // We allow tests to set caching on the sdch response,

	220 // so that we can force an encoded response with no

	221 // dictionary.

	222 if (cache_sdch_response_)

	223 response->AddCustomHeader("Cache-Control", "max-age=3600");

	224 else

	225 response->AddCustomHeader("Cache-Control", "no-store");

	226 } else {

	227 response->set_content_type("text/plain");

	228 response->set_content(kSampleData);

	229 if (ClientIsAdvertisingSdchEncoding(request.headers))

	230 response->AddCustomHeader("Get-Dictionary", "/dict");

	231 // We never cache the plain data response, to make it

	232 // easy to refresh after we get the dictionary.

	233 response->AddCustomHeader("Cache-Control", "no-store");

	234 }

	235 } else {

	236 DCHECK_EQ(request.relative_url, "/dict");
	mef 2014/07/23 18:34:28 thought: Should "/dict" and such be defined consta thought: Should "/dict" and such be defined constants? I'm not sure. Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Probably worth doing, if only to reduce the chance Show quoted text On 2014/07/23 18:34:28, mef wrote: > thought: Should "/dict" and such be defined constants? I'm not sure. Probably worth doing, if only to reduce the chance of typos. Done.
	237 DCHECK_NE(sdch_dictionary_contents_, "");

	238 response->set_content_type("application/x-sdch-dictionary");

	239 response->set_content(sdch_dictionary_contents_);

	240 }

	241 std::vector<base::Closure> callbacks;

	242 callbacks.swap(callback_vector_);

	243 for (std::vector<base::Closure>::iterator it = callbacks.begin();

	244 it != callbacks.end(); ++it) {

	245 it->Run();

	246 }

	247 return response.PassAs<net::test_server::HttpResponse>();

	248 }

	249

	250 void WaitAndGetRequestVector(int num_requests,

	251 base::Closure callback,

	252 RequestVector* v) {

	253 DCHECK_LT(0, num_requests);

	254 if (static_cast<size_t>(num_requests) > request_vector_.size()) {

	255 callback_vector_.push_back(

	256 base::Bind(&SdchResponseHandler::WaitAndGetRequestVector,
	mef 2014/07/23 18:34:29 I'm a bit unhappy about polling loop here, but I'm I'm a bit unhappy about polling loop here, but I'm not sure how to make it better. Randy Smith (Not in Mondays) 2014/07/29 23:44:20 I'm confused by your comment; I don't see code her Show quoted text On 2014/07/23 18:34:29, mef wrote: > I'm a bit unhappy about polling loop here, but I'm not sure how to make it > better. I'm confused by your comment; I don't see code here that I can map to "polling loop" in my mind. We check to see if we've achieved the condition we're waiting for; if we haven't, we queue a callback to be called when the state changes to check again. IF we have, we send the response. I suppose it's a loop in the sense that this function is called every time request_vector_ changes, but I don't think of that as polling. Willing to say more about your discomfort? I'm happy to try to improve things if I understand the problem. mef 2014/07/30 17:14:58 Never mind. I've called it a polling loop because Show quoted text On 2014/07/29 23:44:20, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > I'm a bit unhappy about polling loop here, but I'm not sure how to make it > > better. > > I'm confused by your comment; I don't see code here that I can map to "polling > loop" in my mind. We check to see if we've achieved the condition we're waiting > for; if we haven't, we queue a callback to be called when the state changes to > check again. IF we have, we send the response. I suppose it's a loop in the > sense that this function is called every time request_vector_ changes, but I > don't think of that as polling. > > Willing to say more about your discomfort? I'm happy to try to improve things > if I understand the problem. Never mind. I've called it a polling loop because it seemed to be re-posting the same callback without any delay/signal until \|request_vector_.size()\| reaches \|num_requests\|. I've missed that \|callback_vector_\| is only called when new request arrives.
	257 weak_ptr_factory_.GetWeakPtr(), num_requests,

	258 callback, v));

	259 return;

	260 }

	261 *v = request_vector_;

	262 content::BrowserThread::PostTask(

	263 content::BrowserThread::UI, FROM_HERE, callback);

	264 }

	265

	266 void set_cache_sdch_response(bool cache_sdch_response) {

	267 cache_sdch_response_ = cache_sdch_response;

	268 }

	269

	270 private:

	271 bool cache_sdch_response_;

	272 std::string encoded_data_;

	273 std::string sdch_dictionary_contents_;

	274 std::string dictionary_client_hash_;

	275 std::string dictionary_server_hash_;

	276 RequestVector request_vector_;

	277 std::vector<base::Closure> callback_vector_;

	278 base::WeakPtrFactory<SdchResponseHandler> weak_ptr_factory_;

	279 };

	280

	281 class SdchBrowserTest : public InProcessBrowserTest, net::URLFetcherDelegate {

	282 public:

	283 static const char kTestHost[];

	284

	285 SdchBrowserTest()

	286 : response_handler_(kTestHost),

	287 url_request_context_getter_(NULL),

	288 fetcher_response_code_(0),

	289 url_fetch_complete_(false),

	290 waiting_(false) {}

	291

	292 // ** Helper functions for fetching data.
	mef 2014/07/23 18:34:29 nit: Do we need ? nit: Do we need ? Randy Smith (Not in Mondays) 2014/07/29 23:44:21 So I put the in to indicate that they were comm Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: Do we need ? So I put the ** in to indicate that they were comments that applied to a set of functions, not just the one following. But the fact that there's a blank line between the comments and the following function might be enough for that, and even if it isn't, it's not a big deal. Done.
	293

	294 void FetchUrlDetailed(GURL url, net::URLRequestContextGetter* getter,

	295 bool use_post) {
	mef 2014/07/23 18:34:29 nit: use_post may fit on previous line. nit: use_post may fit on previous line. Randy Smith (Not in Mondays) 2014/07/29 23:44:20 Not in my checkout :-{. Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: use_post may fit on previous line. Not in my checkout :-{. jar (doing other things) 2014/07/31 03:24:33 nit: When args in definition or declaration don't nit: When args in definition or declaration don't fit on one line, put each arg on a separate line. Randy Smith (Not in Mondays) 2014/08/11 20:33:24 Moot; use_post removed. Show quoted text On 2014/07/31 03:24:33, jar wrote: > nit: When args in definition or declaration don't fit on one line, put each arg > on a separate line. Moot; use_post removed.
	296 url_fetch_complete_ = false;

	297 fetcher_status_ = net::URLRequestStatus();

	298 fetcher_response_code_ = 0;

	299 fetcher_response_headers_ = NULL;

	300 fetcher_response_contents_ = "";

	301 fetcher_.reset(net::URLFetcher::Create(

	302 url, use_post ? net::URLFetcher::POST : net::URLFetcher::GET, this));

	303 if (use_post)

	304 fetcher_->SetUploadData("text/plain", "Simple content");

	305 fetcher_->SetRequestContext(getter);

	306 fetcher_->Start();

	307 if (!url_fetch_complete_) {

	308 waiting_ = true;

	309 content::RunMessageLoop();

	310 waiting_ = false;

	311 }

	312 CHECK(url_fetch_complete_);

	313 }

	314 void FetchUrl(GURL url) {
	mef 2014/07/23 18:34:29 nit: I think we separate methods by blank lines. nit: I think we separate methods by blank lines. Randy Smith (Not in Mondays) 2014/07/29 23:44:21 The coding style guide says "Minimize use of verti Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: I think we separate methods by blank lines. The coding style guide says "Minimize use of vertical whitespace, but this is a principle, not a rule." I've seen a lot of cases in which we put trivial getters one right after the other, so I've left the trivial getters clumped, but put in whitespace to separate out more complex functions. Let me know if you feel strongly about this--I don't, just came up with what seemed like a good compromise. mef 2014/07/30 17:14:58 SGTM, I don't feel strongly, and I'm still learnin Show quoted text On 2014/07/29 23:44:21, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > nit: I think we separate methods by blank lines. > > The coding style guide says "Minimize use of vertical whitespace, but this is a > principle, not a rule." I've seen a lot of cases in which we put trivial > getters one right after the other, so I've left the trivial getters clumped, but > put in whitespace to separate out more complex functions. > > Let me know if you feel strongly about this--I don't, just came up with what > seemed like a good compromise. SGTM, I don't feel strongly, and I'm still learning/interpreting the style guide. :) jar (doing other things) 2014/07/31 03:24:33 FWIW: I think vertical whitespace between function Show quoted text On 2014/07/30 17:14:58, mef wrote: > On 2014/07/29 23:44:21, rdsmith wrote: > > On 2014/07/23 18:34:29, mef wrote: > > > nit: I think we separate methods by blank lines. > > > > The coding style guide says "Minimize use of vertical whitespace, but this is > a > > principle, not a rule." I've seen a lot of cases in which we put trivial > > getters one right after the other, so I've left the trivial getters clumped, > but > > put in whitespace to separate out more complex functions. > > > > Let me know if you feel strongly about this--I don't, just came up with what > > seemed like a good compromise. > SGTM, I don't feel strongly, and I'm still learning/interpreting the style > guide. :) FWIW: I think vertical whitespace between function definitions is helpful in readability.... and I don't think I ever see files without it (despite the style guide suggestion to "minimize"). I'd suggest consistent use of the vertical whitespace between definitions... as you have mostly done throughout this file. Randy Smith (Not in Mondays) 2014/08/11 20:33:25 url_request_test_util.h, TestDelegate as an exampl Show quoted text On 2014/07/31 03:24:33, jar wrote: > On 2014/07/30 17:14:58, mef wrote: > > On 2014/07/29 23:44:21, rdsmith wrote: > > > On 2014/07/23 18:34:29, mef wrote: > > > > nit: I think we separate methods by blank lines. > > > > > > The coding style guide says "Minimize use of vertical whitespace, but this > is > > a > > > principle, not a rule." I've seen a lot of cases in which we put trivial > > > getters one right after the other, so I've left the trivial getters clumped, > > but > > > put in whitespace to separate out more complex functions. > > > > > > Let me know if you feel strongly about this--I don't, just came up with what > > > seemed like a good compromise. > > SGTM, I don't feel strongly, and I'm still learning/interpreting the style > > guide. :) > > FWIW: I think vertical whitespace between function definitions is helpful in > readability.... and I don't think I ever see files without it (despite the style > guide suggestion to "minimize"). > > I'd suggest consistent use of the vertical whitespace between definitions... as > you have mostly done throughout this file. url_request_test_util.h, TestDelegate as an example, but I've seen a lot of others. One line definitions often are not separated by whitespace. Having said that, I was about to bow to your and Misha's united front, but then realized I don't have any one-line definitions anymore. Moot :-}.
	315 FetchUrlDetailed(url, url_request_context_getter_, false);

	316 }

	317 int fetcher_response_code() { return fetcher_response_code_; }
	mef 2014/07/23 18:34:29 nit: const (also below). nit: const (also below). Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done. Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: const (also below). Done.
	318 const net::URLRequestStatus& fetcher_status() { return fetcher_status_; }

	319 const net::HttpResponseHeaders* fetcher_response_headers() {

	320 return fetcher_response_headers_;

	321 }

	322 std::string fetcher_response_contents() { return fetcher_response_contents_; }

	323

	324 GURL data_url() {
	mef 2014/07/23 18:34:28 nit: it's not a simple accessor, so should be GetD nit: it's not a simple accessor, so should be GetDataUrl()? Also, do you need similar GetDictUrl()? Randy Smith (Not in Mondays) 2014/07/29 23:44:20 I only used it in one place, so I inlined it. Show quoted text On 2014/07/23 18:34:28, mef wrote: > nit: it's not a simple accessor, so should be GetDataUrl()? I only used it in one place, so I inlined it. Show quoted text > Also, do you need similar GetDictUrl()? That value isn't used anywhere in this file; the dictionary fetch is dispatched in the guts of the SDCH implementaiton. jar (doing other things) 2014/07/31 03:24:33 +1 on the comment: avoid hacker style except for s +1 on the comment: avoid hacker style except for simple getters/setters or kindred. In truth, you didn't "inline it," as that is the choice of the compiler. All you did was to give the code in the class... just as you did on line 295.... but that is not an indicator of whether hacker naming style should be used. On 2014/07/29 23:44:20, rdsmith wrote: Show quoted text > On 2014/07/23 18:34:28, mef wrote: > > nit: it's not a simple accessor, so should be GetDataUrl()? > > I only used it in one place, so I inlined it. > > > Also, do you need similar GetDictUrl()? > > That value isn't used anywhere in this file; the dictionary fetch is dispatched > in the guts of the SDCH implementaiton. > > Randy Smith (Not in Mondays) 2014/08/11 20:33:25 I don't understand this comment--I think of gettin Show quoted text On 2014/07/31 03:24:33, jar wrote: > +1 on the comment: avoid hacker style except for simple getters/setters or > kindred. > > In truth, you didn't "inline it," as that is the choice of the compiler. All > you did was to give the code in the class... just as you did on line 295.... but > that is not an indicator of whether hacker naming style should be used. I don't understand this comment--I think of getting rid of a function and replacing all places where it was used by the contents of the function as being a reasonable use of the term "inlining"--but I don't think it matters as I don't think there's an AI for me in it. Show quoted text > > On 2014/07/29 23:44:20, rdsmith wrote: > > On 2014/07/23 18:34:28, mef wrote: > > > nit: it's not a simple accessor, so should be GetDataUrl()? > > > > I only used it in one place, so I inlined it. > > > > > Also, do you need similar GetDictUrl()? > > > > That value isn't used anywhere in this file; the dictionary fetch is > dispatched > > in the guts of the SDCH implementaiton. > > > > >
	325 return GURL(base::StringPrintf(

	326 "http://%s:%d/data", kTestHost, test_server_port()));

	327 }

	328

	329 // Get the data from the server. Return value is success/failure of the

	330 // data operation, \|*sdch_encoding_used\| indicates whether or not the

	331 // data was retrieved with sdch encoding.

	332 // This is done through FetchUrl(), so the various helper functions

	333 // will have valid status if it returns successfully.

	334 bool GetDataDetailed(net::URLRequestContextGetter* getter,

	335 bool use_post,
	mef 2014/07/23 18:34:29 FWIW it seems that \|use_post\| is never true any mo FWIW it seems that \|use_post\| is never true any more. Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Good point. Gone. Show quoted text On 2014/07/23 18:34:29, mef wrote: > FWIW it seems that \|use_post\| is never true any more. Good point. Gone.
	336 bool* sdch_encoding_used) {

	337 FetchUrlDetailed(data_url(), getter, use_post);

	338 EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher_status().status())

	339 << "Error code is " << fetcher_status().error();

	340 EXPECT_EQ(200, fetcher_response_code());

	341 EXPECT_EQ(kSampleData, fetcher_response_contents());

	342

	343 if (net::URLRequestStatus::SUCCESS != fetcher_status().status() \|\|

	344 200 != fetcher_response_code()) {

	345 *sdch_encoding_used = false;

	346 return false;

	347 }

	348

	349 *sdch_encoding_used = ResponseContainsHeaderValue(

	350 *fetcher_response_headers(), "Content-Encoding", "sdch");

	351

	352 if (fetcher_response_contents() != kSampleData)

	353 return false;

	354

	355 return true;

	356 }

	357 bool GetData(bool* sdch_encoding_used) {

	358 return GetDataDetailed(

	359 url_request_context_getter_, false, sdch_encoding_used);

	360 }

	361

	362 // ** Client information and control.
	jar (doing other things) 2014/07/31 03:24:33 nit: I saw mention of your convention that "" me nit: I saw mention of your convention that "" means something bigger in the comment... but this seems very non-standard (re: style guide). If you really want to say something with such meaning, you could say: // The next several methods handle client information and control. [side comment: I think I recall getting dinged for this sort of improvisational separating of regions in my readability review, long ago ;-). ] Also, even though you are allowed to list code explicitly here inside a class when defining the class in a test file, it is often MUCH more readable to not do so (especially as methods get big!). This allows a reader to get the jist of the class's use, without getting bogged down in details... and then inspect the details of the methods in a section that follows the class definition. YMMV. Randy Smith (Not in Mondays) 2014/08/11 20:33:25 I believe this comment is moot. Show quoted text On 2014/07/31 03:24:33, jar wrote: > nit: I saw mention of your convention that "**" means something bigger in the > comment... but this seems very non-standard (re: style guide). If you really > want to say something with such meaning, you could say: > > // The next several methods handle client information and control. > > [side comment: I think I recall getting dinged for this sort of improvisational > separating of regions in my readability review, long ago ;-). ] I believe this comment is moot. Show quoted text > Also, even though you are allowed to list code explicitly here inside a class > when defining the class in a test file, it is often MUCH more readable to not do > so (especially as methods get big!). This allows a reader to get the jist of the > class's use, without getting bogged down in details... and then inspect the > details of the methods in a section that follows the class definition. YMMV. Hmmm. I hear you. I'm not inclined to do anything right now (unless this was an active request) but I'll keep it in mind for future such situations.
	363

	364 int GetNumberOfDictionaryFetches(Profile* profile) {

	365 int fetches = -1;

	366 base::RunLoop run_loop;

	367 content::BrowserThread::PostTaskAndReply(

	368 content::BrowserThread::IO, FROM_HERE,

	369 base::Bind(&SdchBrowserTest::GetNumberOfDictionaryFetchesOnIOThread,

	370 base::Unretained(profile->GetRequestContext()),

	371 &fetches),

	372 run_loop.QuitClosure());

	373 run_loop.Run();
	jar (doing other things) 2014/07/31 03:24:33 This might be the (only?) way to run these tests.. This might be the (only?) way to run these tests.... but I'll tell you that running message loops, and posting such Quit messages, is fraught with peril. For instance, it is possible that some code will proceed to post additional message to the run loop, and they'll be queued after the Quic task. It is similarly (sadly) possible that other folks will try to run nested message loops... and then their Quit may be confused with your posted Quit. I'll assume that this is the preferred approach for this class of test... and you weren't being inventive here, as this is a dangerous place to play. If your tests start failing, you might want to (at a minimum) add code to CHECK() that there is nothing else pending when the run_loop returns from Run(). Randy Smith (Not in Mondays) 2014/08/11 20:33:25 I believe that this last point is exactly what the Show quoted text On 2014/07/31 03:24:33, jar wrote: > This might be the (only?) way to run these tests.... but I'll tell you that > running message loops, and posting such Quit messages, is fraught with peril. > For instance, it is possible that some code will proceed to post additional > message to the run loop, and they'll be queued after the Quic task. It is > similarly (sadly) possible that other folks will try to run nested message > loops... and then their Quit may be confused with your posted Quit. I believe that this last point is exactly what the RunLoop class was created for; this Quit will only apply to this level of nested message loop. But I don't claim to be an expert. Show quoted text > I'll assume that this is the preferred approach for this class of test... and > you weren't being inventive here, as this is a dangerous place to play. If your > tests start failing, you might want to (at a minimum) add code to CHECK() that > there is nothing else pending when the run_loop returns from Run(). I just spent some time wandering through MessageLoop and MessageLoopProxy (and RunLoop, though I don't think that's the right place) and their inheritance chains, and I can't find a way to do a check that there's nothing else pending. Sorry if I've missed it--can you give me a more precise pointer? jar (doing other things) 2014/08/13 01:28:58 <sigh> I don't think I see a way to make the asser Show quoted text On 2014/08/11 20:33:25, rdsmith wrote: > On 2014/07/31 03:24:33, jar wrote: > > This might be the (only?) way to run these tests.... but I'll tell you that > > running message loops, and posting such Quit messages, is fraught with peril. > > For instance, it is possible that some code will proceed to post additional > > message to the run loop, and they'll be queued after the Quic task. It is > > similarly (sadly) possible that other folks will try to run nested message > > loops... and then their Quit may be confused with your posted Quit. > > I believe that this last point is exactly what the RunLoop class was created > for; this Quit will only apply to this level of nested message loop. But I > don't claim to be an expert. > > > I'll assume that this is the preferred approach for this class of test... and > > you weren't being inventive here, as this is a dangerous place to play. If > your > > tests start failing, you might want to (at a minimum) add code to CHECK() that > > there is nothing else pending when the run_loop returns from Run(). > > I just spent some time wandering through MessageLoop and MessageLoopProxy (and > RunLoop, though I don't think that's the right place) and their inheritance > chains, and I can't find a way to do a check that there's nothing else pending. > Sorry if I've missed it--can you give me a more precise pointer? <sigh> I don't think I see a way to make the assertion. Quit does not only apply to the current level of message loop nesting, and the header file warns of the resulting "livelock" that can ensue. When there are other ways to go... it is always better to avoid nested message loops. Randy Smith (Not in Mondays) 2014/08/13 02:05:25 We're reading the header file differently. I take Show quoted text On 2014/08/13 01:28:58, jar wrote: > On 2014/08/11 20:33:25, rdsmith wrote: > > On 2014/07/31 03:24:33, jar wrote: > > > This might be the (only?) way to run these tests.... but I'll tell you that > > > running message loops, and posting such Quit messages, is fraught with > peril. > > > For instance, it is possible that some code will proceed to post additional > > > message to the run loop, and they'll be queued after the Quic task. It is > > > similarly (sadly) possible that other folks will try to run nested message > > > loops... and then their Quit may be confused with your posted Quit. > > > > I believe that this last point is exactly what the RunLoop class was created > > for; this Quit will only apply to this level of nested message loop. But I > > don't claim to be an expert. > > > > > I'll assume that this is the preferred approach for this class of test... > and > > > you weren't being inventive here, as this is a dangerous place to play. If > > your > > > tests start failing, you might want to (at a minimum) add code to CHECK() > that > > > there is nothing else pending when the run_loop returns from Run(). > > > > I just spent some time wandering through MessageLoop and MessageLoopProxy (and > > RunLoop, though I don't think that's the right place) and their inheritance > > chains, and I can't find a way to do a check that there's nothing else > pending. > > Sorry if I've missed it--can you give me a more precise pointer? > > <sigh> I don't think I see a way to make the assertion. > > Quit does not only apply to the current level of message loop nesting, and the > header file warns of the resulting "livelock" that can ensue. We're reading the header file differently. I take: // Quitting one RunLoop has no bearing on // the others. Quit can be called before, during or after Run. If called // before Run, Run will return immediately when called. Calling Quit after th\ e // RunLoop has already finished running has no effect. as meaning that a RunLoop Quit() will only affect the runloop it's called from, and I take the following paragraph as saying that if there are message loops nested inside this one, calling Quit on this one won't affect that one. I.e. yes, it's possible to get into trouble here. But using RunLoop helps against one common way of getting into trouble. Glancing at the implementation of RunLoop::Quit(), it fits my understanding. Show quoted text > When there are other ways to go... it is always better to avoid nested message > loops. I'm happy to follow that advice; I just don't see those other ways. I can't imagine any other way to do a synchronous call other than a RunLoop. I'm not sure this counts as a nested run loop, just because if we don't enter a runloop from the test code, no messages will be processed for the lifetime of the test. If there is a message loop being run, it's several levels above the main test code in the stack. jar (doing other things) 2014/08/14 22:33:25 Independent of the performance... I'm OK with this Independent of the performance... I'm OK with this form. On 2014/08/13 02:05:25, rdsmith wrote: Show quoted text > On 2014/08/13 01:28:58, jar wrote: > > On 2014/08/11 20:33:25, rdsmith wrote: > > > On 2014/07/31 03:24:33, jar wrote: > > > > This might be the (only?) way to run these tests.... but I'll tell you > that > > > > running message loops, and posting such Quit messages, is fraught with > > peril. > > > > For instance, it is possible that some code will proceed to post > additional > > > > message to the run loop, and they'll be queued after the Quic task. It is > > > > similarly (sadly) possible that other folks will try to run nested message > > > > loops... and then their Quit may be confused with your posted Quit. > > > > > > I believe that this last point is exactly what the RunLoop class was created > > > for; this Quit will only apply to this level of nested message loop. But I > > > don't claim to be an expert. > > > > > > > I'll assume that this is the preferred approach for this class of test... > > and > > > > you weren't being inventive here, as this is a dangerous place to play. > If > > > your > > > > tests start failing, you might want to (at a minimum) add code to CHECK() > > that > > > > there is nothing else pending when the run_loop returns from Run(). > > > > > > I just spent some time wandering through MessageLoop and MessageLoopProxy > (and > > > RunLoop, though I don't think that's the right place) and their inheritance > > > chains, and I can't find a way to do a check that there's nothing else > > pending. > > > Sorry if I've missed it--can you give me a more precise pointer? > > > > <sigh> I don't think I see a way to make the assertion. > > > > Quit does not only apply to the current level of message loop nesting, and the > > header file warns of the resulting "livelock" that can ensue. > > We're reading the header file differently. I take: > > // Quitting one RunLoop has no bearing on > // the others. Quit can be called before, during or after Run. If called > // before Run, Run will return immediately when called. Calling Quit after th\ > e > // RunLoop has already finished running has no effect. > > as meaning that a RunLoop Quit() will only affect the runloop it's called from, > and I take the following paragraph as saying that if there are message loops > nested inside this one, calling Quit on this one won't affect that one. I.e. > yes, it's possible to get into trouble here. But using RunLoop helps against > one common way of getting into trouble. Glancing at the implementation of > RunLoop::Quit(), it fits my understanding. > > > When there are other ways to go... it is always better to avoid nested message > > loops. > > I'm happy to follow that advice; I just don't see those other ways. I can't > imagine any other way to do a synchronous call other than a RunLoop. > > I'm not sure this counts as a nested run loop, just because if we don't enter > a runloop from the test code, no messages will be processed for the lifetime of > the test. If there is a message loop being run, it's several levels above the > main test code in the stack.
	374 DCHECK_NE(-1, fetches);

	375 return fetches;

	376 }

	377

	378 void BrowsingDataRemoveAndWait(int remove_mask) {

	379 BrowsingDataRemover* remover = BrowsingDataRemover::CreateForPeriod(

	380 browser()->profile(), BrowsingDataRemover::LAST_HOUR);

	381 BrowsingDataRemoverCompletionObserver completion_observer(remover);

	382 remover->Remove(remove_mask, BrowsingDataHelper::UNPROTECTED_WEB);

	383 completion_observer.BlockUntilCompletion();

	384 }

	385

	386 // Something of a cheat; nuke the dictionaries off the SdchManager without

	387 // touching the cache (which browsing data remover would do).

	388 void NukeSdchDictionaries() {

	389 base::RunLoop run_loop;

	390 content::BrowserThread::PostTaskAndReply(

	391 content::BrowserThread::IO, FROM_HERE,

	392 base::Bind(&SdchBrowserTest::NukeSdchDictionariesOnIOThread,

	393 url_request_context_getter_),

	394 run_loop.QuitClosure());

	395 run_loop.Run();

	396 }

	397

	398 // Create a second profile to work within multi-profile.

	399 Profile* CreateSecondProfile() {

	400 base::FilePath user_data_dir;

	401 PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);

	402

	403 if (!second_profile_data_dir_.CreateUniqueTempDirUnderPath(user_data_dir))

	404 return NULL;

	405

	406 Profile* second_profile =

	407 g_browser_process->profile_manager()->GetProfile(

	408 second_profile_data_dir_.path());

	409

	410 return second_profile;
	jar (doing other things) 2014/07/31 03:24:34 nit: probably wasn't worth defining a variable he nit: probably wasn't worth defining a variable here. Randy Smith (Not in Mondays) 2014/08/11 20:33:24 Semi-moot, as function has been inlined (sic?) int Show quoted text On 2014/07/31 03:24:34, jar wrote: > nit: probably wasn't worth defining a variable here. Semi-moot, as function has been inlined (sic?) into SetupSecondBrowser. I say "semi", as I still use a variable for this purpose there, but in many more places :-}.
	411 }

	412

	413 Browser* CreateBrowserOnProfile(Profile* profile) {

	414 DCHECK(profile);

	415 Browser* new_browser =

	416 new Browser(Browser::CreateParams(

	417 profile, browser()->host_desktop_type()));

	418 chrome::AddSelectedTabWithURL(new_browser,

	419 GURL(url::kAboutBlankURL),

	420 content::PAGE_TRANSITION_AUTO_TOPLEVEL);

	421 content::WaitForLoadStop(

	422 new_browser->tab_strip_model()->GetActiveWebContents());

	423 new_browser->window()->Show();

	424 return new_browser;

	425 }

	426

	427 // ** Server information and control.
	jar (doing other things) 2014/07/31 03:24:33 nit: again, kill "", and use words. nit: again, kill "", and use words. Randy Smith (Not in Mondays) 2014/08/11 20:33:25 Moot. Show quoted text On 2014/07/31 03:24:33, jar wrote: > nit: again, kill "**", and use words. Moot.
	428

	429 void WaitAndGetTestVector(int num_requests, RequestVector* result) {

	430 base::RunLoop run_loop;

	431 content::BrowserThread::PostTask(

	432 content::BrowserThread::IO, FROM_HERE,

	433 base::Bind(&SdchResponseHandler::WaitAndGetRequestVector,

	434 base::Unretained(&response_handler_),

	435 num_requests,

	436 run_loop.QuitClosure(),

	437 result));

	438 run_loop.Run();

	439 }

	440

	441 int test_server_port() { return test_server_.port(); }

	442

	443 void SetSdchCacheability(bool cache_sdch_response) {

	444 base::RunLoop run_loop;

	445 content::BrowserThread::PostTaskAndReply(

	446 content::BrowserThread::IO, FROM_HERE,

	447 base::Bind(&SdchResponseHandler::set_cache_sdch_response,

	448 base::Unretained(&response_handler_),

	449 cache_sdch_response),

	450 run_loop.QuitClosure());

	451 run_loop.Run();

	452 }

	453

	454 // Helper function for common test pattern.

	455 //

	456 // This function gets the data, confirms that the initial sending of the

	457 // data included a dictionary advertisement, that that advertisement

	458 // resulted in queueing a dictionary fetch, forces that fetch to

	459 // go through, and confirms that a follow-on data load uses SDCH

	460 // encoding. Returns true if the entire sequence of events occurred.

	461 bool ForceSdchDictionaryLoad(Browser* browser) {

	462 bool sdch_encoding_used = true;

	463 bool data_gotten = GetDataDetailed(

	464 browser->profile()->GetRequestContext(), false,

	465 &sdch_encoding_used);

	466 EXPECT_TRUE(data_gotten);

	467 if (!data_gotten) return false;

	468 EXPECT_FALSE(sdch_encoding_used);

	469

	470 // Confirm that we were told to get the dictionary

	471 const net::HttpResponseHeaders* headers = fetcher_response_headers();

	472 std::string value;

	473 bool have_dict_header =

	474 headers->EnumerateHeader(NULL, "Get-Dictionary", &value);

	475 EXPECT_TRUE(have_dict_header);

	476 if (!have_dict_header) return false;
	mef 2014/07/23 18:34:29 nit: Should this be ASSERT instead as we exit anyw nit: Should this be ASSERT instead as we exit anyway? Also below. Randy Smith (Not in Mondays) 2014/07/29 23:44:20 So this is an annoying test pattern that I'd be ha Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: Should this be ASSERT instead as we exit anyway? Also below. So this is an annoying test pattern that I'd be happy to have you guide me towards a fix for, but you can't ASSERT if you aren't in a test fixture; the return value is preempted for use by the test fixture. You can see this if you try to assert in a void returning helper function; if I asserted here I think it would compile but I'm not at all sure it would do what I want. Thus this circumlocution. mef 2014/07/30 17:14:58 Acknowledged. Show quoted text On 2014/07/29 23:44:20, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > nit: Should this be ASSERT instead as we exit anyway? Also below. > > So this is an annoying test pattern that I'd be happy to have you guide me > towards a fix for, but you can't ASSERT if you aren't in a test fixture; the > return value is preempted for use by the test fixture. You can see this if you > try to assert in a void returning helper function; if I asserted here I think it > would compile but I'm not at all sure it would do what I want. Thus this > circumlocution. Acknowledged.
	477

	478 // If the above didn't result in a dictionary fetch being queued, the

	479 // rest of the test will time out. Avoid that.

	480 int num_fetches = GetNumberOfDictionaryFetches(browser->profile());

	481 EXPECT_EQ(1, num_fetches);

	482 if (1 != num_fetches) return false;

	483

	484 // Wait until the dictionary fetch actually happens.

	485 RequestVector request_vector;

	486 WaitAndGetTestVector(2, &request_vector);

	487 EXPECT_EQ(request_vector[1].relative_url, "/dict");

	488 if (request_vector[1].relative_url != "/dict") return false;
	mef 2014/07/23 18:34:28 Maybe making "/dict" and "/data" explicit constant Maybe making "/dict" and "/data" explicit constants makes sense... Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done. Show quoted text On 2014/07/23 18:34:28, mef wrote: > Maybe making "/dict" and "/data" explicit constants makes sense... Done.
	489

	490 // Do a round trip to the server ignoring the encoding, presuming

	491 // that if we've gotten data to this thread, the dictionary's made

	492 // it into the SdchManager.

	493 data_gotten = GetDataDetailed(

	494 browser->profile()->GetRequestContext(), false, &sdch_encoding_used);

	495 EXPECT_TRUE(data_gotten);

	496 if (!data_gotten) return false;

	497

	498 // Now data fetches should be SDCH encoded.

	499 sdch_encoding_used = false;

	500 data_gotten = GetDataDetailed(

	501 browser->profile()->GetRequestContext(), false, &sdch_encoding_used);

	502 EXPECT_TRUE(data_gotten);

	503 EXPECT_TRUE(sdch_encoding_used);

	504

	505 if (!data_gotten \|\| !sdch_encoding_used) return false;

	506

	507 // Confirm the request vector looks at this point as expected.

	508 WaitAndGetTestVector(4, &request_vector);

	509 EXPECT_EQ(4u, request_vector.size());

	510 EXPECT_EQ(request_vector[2].relative_url, "/data");

	511 EXPECT_EQ(request_vector[3].relative_url, "/data");

	512 return (4u == request_vector.size() &&

	513 request_vector[2].relative_url == "/data" &&

	514 request_vector[3].relative_url == "/data");

	515 }

	516

	517 private:

	518 // InProcessBrowserTest

	519 virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE {

	520 command_line->AppendSwitchASCII(

	521 switches::kHostResolverRules,

	522 "MAP " + std::string(kTestHost) + " 127.0.0.1");

	523 }

	524

	525 virtual void SetUpOnMainThread() OVERRIDE {

	526 test_server_.RegisterRequestHandler(

	527 base::Bind(&SdchResponseHandler::HandleRequest,

	528 base::Unretained(&response_handler_)));

	529 CHECK(test_server_.InitializeAndWaitUntilReady());

	530 url_request_context_getter_ = browser()->profile()->GetRequestContext();

	531 }

	532

	533 virtual void TearDownOnMainThread() OVERRIDE {

	534 CHECK(test_server_.ShutdownAndWaitUntilComplete());

	535 }

	536

	537 static void NukeSdchDictionariesOnIOThread(
	mef 2014/07/23 18:34:29 I presume these are not overrides from InProcessBr I presume these are not overrides from InProcessBrowserTest, so maybe they should go above. Randy Smith (Not in Mondays) 2014/07/29 23:44:21 Done. Show quoted text On 2014/07/23 18:34:29, mef wrote: > I presume these are not overrides from InProcessBrowserTest, so maybe they > should go above. Done.
	538 net::URLRequestContextGetter* context_getter) {

	539 DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));

	540 net::SdchManager* sdch_manager =

	541 context_getter->GetURLRequestContext()->sdch_manager();

	542 DCHECK(sdch_manager);

	543 sdch_manager->ClearData();

	544 }

	545

	546 static void GetNumberOfDictionaryFetchesOnIOThread(

	547 net::URLRequestContextGetter* url_request_context_getter,

	548 int* result) {

	549 DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));

	550 net::SdchManager* sdch_manager =

	551 url_request_context_getter->GetURLRequestContext()->sdch_manager();

	552 DCHECK(sdch_manager);

	553 *result = sdch_manager->GetFetchesRequestedForTesting();

	554 }

	555

	556 // URLFetcherDelegate

	557 virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE {

	558 fetcher_status_ = source->GetStatus();

	559 if (fetcher_status_.status() == net::URLRequestStatus::SUCCESS) {

	560 fetcher_response_code_ = source->GetResponseCode();
	mef 2014/07/23 18:34:29 Do we really need to copy them into this class? Do we really need to copy them into this class? Randy Smith (Not in Mondays) 2014/07/29 23:44:20 Probably not, given that we seem to always check t Show quoted text On 2014/07/23 18:34:29, mef wrote: > Do we really need to copy them into this class? Probably not, given that we seem to always check them before starting a new fetch. Done.
	561 fetcher_response_headers_ = source->GetResponseHeaders();

	562 CHECK(source->GetResponseAsString(&fetcher_response_contents_));

	563 }

	564 url_fetch_complete_ = true;

	565 if (waiting_)

	566 base::MessageLoopForUI::current()->Quit();

	567 }

	568

	569 SdchResponseHandler response_handler_;

	570 net::test_server::EmbeddedTestServer test_server_;

	571 scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;

	572 scoped_ptr<net::URLFetcher> fetcher_;

	573 net::URLRequestStatus fetcher_status_;

	574 int fetcher_response_code_;

	575 net::HttpResponseHeaders* fetcher_response_headers_;

	576 std::string fetcher_response_contents_;

	577 bool url_fetch_complete_;

	578 bool waiting_;

	579 base::ScopedTempDir second_profile_data_dir_;
	mef 2014/07/23 18:34:28 Would it get deleted if profile is still open? Would it get deleted if profile is still open? Randy Smith (Not in Mondays) 2014/07/29 23:44:20 So this was a tricky question to track down, but: Show quoted text On 2014/07/23 18:34:28, mef wrote: > Would it get deleted if profile is still open? So this was a tricky question to track down, but: the profile will be closed before this destructor is called. InProcessBrowserTest::SetUp calls (indirectly) content::BrowserMain, which calls BrowserMainRunnerImpl::Shutdown at it's end, which rips down all the profiles. mef 2014/07/30 17:14:58 Acknowledged. Show quoted text On 2014/07/29 23:44:20, rdsmith wrote: > On 2014/07/23 18:34:28, mef wrote: > > Would it get deleted if profile is still open? > > So this was a tricky question to track down, but: the profile will be closed > before this destructor is called. InProcessBrowserTest::SetUp calls > (indirectly) content::BrowserMain, which calls BrowserMainRunnerImpl::Shutdown > at it's end, which rips down all the profiles. > Acknowledged.
	580 };

	581

	582 const char SdchBrowserTest::kTestHost[] = "our.test.host.com";;
	mef 2014/07/23 18:34:29 nit: ;; nit: ;; Randy Smith (Not in Mondays) 2014/07/29 23:44:20 Done. Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: ;; Done.
	583

	584 // Confirm that after getting a dictionary, calling the browsing

	585 // data remover renders it unusable. Also (in calling

	586 // ForceSdchDictionaryLoad()) servers as a smoke test for SDCH.

	587 IN_PROC_BROWSER_TEST_F(SdchBrowserTest, BrowsingDataRemover) {

	588 ASSERT_TRUE(ForceSdchDictionaryLoad(browser()));

	589

	590 // Confirm browsing data remover without removing the cache leaves

	591 // SDCH alone.

	592 BrowsingDataRemoveAndWait(BrowsingDataRemover::REMOVE_ALL &

	593 ~BrowsingDataRemover::REMOVE_CACHE);

	594 bool sdch_encoding_used = false;

	595 ASSERT_TRUE(GetData(&sdch_encoding_used));

	596 EXPECT_TRUE(sdch_encoding_used);

	597

	598 // Confirm browsing data remover removing the cache clears SDCH state.

	599 BrowsingDataRemoveAndWait(BrowsingDataRemover::REMOVE_CACHE);

	600 sdch_encoding_used = false;

	601 ASSERT_TRUE(GetData(&sdch_encoding_used));

	602 EXPECT_FALSE(sdch_encoding_used);

	603 }

	604

	605 // Confirm dictionaries not visible in other profiles.

	606 IN_PROC_BROWSER_TEST_F(SdchBrowserTest, Isolation) {

	607 ASSERT_TRUE(ForceSdchDictionaryLoad(browser()));

	608

	609 // Data fetches from incognito or separate profiles should not be SDCH

	610 // encoded.

	611 bool sdch_encoding_used = true;

	612 Browser* incognito_browser = CreateIncognitoBrowser();

	613 EXPECT_TRUE(GetDataDetailed(
	mef 2014/07/23 18:34:29 nit: Could use GetData() instead. Also below. nit: Could use GetData() instead. Also below. Randy Smith (Not in Mondays) 2014/07/29 23:44:20 Why? I'm not seeing it--I want to test the fetch Show quoted text On 2014/07/23 18:34:29, mef wrote: > nit: Could use GetData() instead. Also below. Why? I'm not seeing it--I want to test the fetch from the incognito browser. mef 2014/07/30 17:14:58 nm, I've missed that GetData() doesn't take the co Show quoted text On 2014/07/29 23:44:20, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > nit: Could use GetData() instead. Also below. > > Why? I'm not seeing it--I want to test the fetch from the incognito browser. nm, I've missed that GetData() doesn't take the context.
	614 incognito_browser->profile()->GetRequestContext(),

	615 false,

	616 &sdch_encoding_used));

	617 EXPECT_FALSE(sdch_encoding_used);

	618

	619 Browser* new_browser = CreateBrowserOnProfile(CreateSecondProfile());

	620 sdch_encoding_used = true;

	621 EXPECT_TRUE(GetDataDetailed(

	622 new_browser->profile()->GetRequestContext(),

	623 false,

	624 &sdch_encoding_used));

	625 EXPECT_FALSE(sdch_encoding_used);

	626 }

	627

	628 // Confirm a dictionary loaded in incognito isn't visible in the main profile.

	629 IN_PROC_BROWSER_TEST_F(SdchBrowserTest, ReverseIsolation) {

	630 Browser* incognito_browser = CreateIncognitoBrowser();

	631 ASSERT_TRUE(ForceSdchDictionaryLoad(incognito_browser));

	632

	633 // Data fetches on main browser should not be SDCH encoded.

	634 bool sdch_encoding_used = true;

	635 ASSERT_TRUE(GetData(&sdch_encoding_used));

	636 EXPECT_FALSE(sdch_encoding_used);
	mef 2014/07/23 18:34:28 Could it happen that SDCH dictionary is left over Could it happen that SDCH dictionary is left over from previous test on main profile? Would it make this test flaky? Randy Smith (Not in Mondays) 2014/07/29 23:44:21 I believe we create and destroy a new profile dire Show quoted text On 2014/07/23 18:34:28, mef wrote: > Could it happen that SDCH dictionary is left over from previous test on main > profile? Would it make this test flaky? I believe we create and destroy a new profile directory for each test through InProcessBrowserTest::CreateUserDataDirectory() on a scoped_tempdir, which is torn down when the InProcessBrowserTest is torn down (I believe there's a separate instance of that object for each browser test). mef 2014/07/30 17:14:58 Acknowledged. Show quoted text On 2014/07/29 23:44:21, rdsmith wrote: > On 2014/07/23 18:34:28, mef wrote: > > Could it happen that SDCH dictionary is left over from previous test on main > > profile? Would it make this test flaky? > > I believe we create and destroy a new profile directory for each test through > InProcessBrowserTest::CreateUserDataDirectory() on a scoped_tempdir, which is > torn down when the InProcessBrowserTest is torn down (I believe there's a > separate instance of that object for each browser test). Acknowledged.
	637 }

	638

	639 } // namespace

OLD	NEW