Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
380003002:
Improve testing for SDCH. (Closed)
|
Created:
6 years, 5 months ago by Randy Smith (Not in Mondays) Modified:
6 years, 4 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionImprove testing for SDCH.
Added some end-to-end browser tests, and added a bit of extra checking
for the filter.
The extra files for encoding dictionaries from sdch/open-vcdiff were
added so that the tests could create an SDCH dictionary when run.
IMO, this is more flexible for future tests and clearer to read and
maintain than just hard-coding a dictionary inline in the tests.
BUG=None
R=mef@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290638
Patch Set 1 #
Total comments: 6
Patch Set 2 : Results of self-review. #
Total comments: 36
Patch Set 3 : Incorporate review comments. #Patch Set 4 : Incorporated all comments. #Patch Set 5 : Incorporated all comments, shifted some testing to unit tests. #Patch Set 6 : Fixed braino: Added SDHC enable to the tests that need it. #
Total comments: 97
Patch Set 7 : Next round of comments incorporated. #
Total comments: 4
Patch Set 8 : Incorporated comments. #Patch Set 9 : Sync'd to r288030. #
Total comments: 8
Patch Set 10 : Incorporated comments. #Patch Set 11 : sync'd to r288872 #Patch Set 12 : Fix silly compile errors. #Patch Set 13 : Change SdchDictionaryFetcher constructor to take a scoped_refptr<>. #Patch Set 14 : Sync'd to r290233. #
Total comments: 3
Patch Set 15 : Fixed Isolation test for ChromeOS. #Messages
Total messages: 26 (0 generated)
Misha: Would you be willing to be primary reviewer on this? Jochen: Would you take a look at the chrome/browser/DEPS change? I'd like to get early feedback on that, because if I can't do it, I've got to rethink my approach to this CL.
I'll be happy to review although I should admit that I know preciously little about vcdiff. Little nits before I go over sdch_browsertest.cc. https://codereview.chromium.org/380003002/diff/1/chrome/browser/net/sdch_brow... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/1/chrome/browser/net/sdch_brow... chrome/browser/net/sdch_browsertest.cc:51: // Scanns in a case-insensitive way for |header| in |map|, nit: Scanns. https://codereview.chromium.org/380003002/diff/1/chrome/browser/net/sdch_brow... chrome/browser/net/sdch_browsertest.cc:59: it != map.end(); ++it) { I'm surprised that we don't have case-insensitive string map. https://codereview.chromium.org/380003002/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/1/net/base/sdch_manager.cc#new... net/base/sdch_manager.cc:222: : fetches_requested_(0) { nit: this should fit on previous line. https://codereview.chromium.org/380003002/diff/1/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/380003002/diff/1/net/base/sdch_manager.h#newc... net/base/sdch_manager.h:339: int GetFetchesRequestedForTesting() const; I think code guide suggest calling simple getter fetches_requested() and making it inline. May also add a small comment on public method. https://codereview.chromium.org/380003002/diff/1/net/filter/mock_filter_conte... File net/filter/mock_filter_context.cc (right): https://codereview.chromium.org/380003002/diff/1/net/filter/mock_filter_conte... net/filter/mock_filter_context.cc:23: context_.reset(); I like. :-) https://codereview.chromium.org/380003002/diff/1/net/filter/sdch_filter_unitt... File net/filter/sdch_filter_unittest.cc (right): https://codereview.chromium.org/380003002/diff/1/net/filter/sdch_filter_unitt... net/filter/sdch_filter_unittest.cc:201: filter_context()->SetRequestTime(base::Time()); Um, so SetRequestTime checks for null? Should there be check that matches a comment that 'Everything that does check it for null should return null'?
I gtg, so I haven't gotten to the meat of tests, but I'm wondering if it is possible to do this in net_unittests as opposed to browser_tests? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:55: bool RequestContainsHeader(const HttpRequestHeaderMap& map, Should it be called GetRequestHeader? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:113: for (size_t i = 0; i < dictionary_server_hash_.size(); ++i) { Why do we need this translation? And if we do, I suggest making a function. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:143: while (it != value.end() && IsAsciiAlpha(*it)) This construct with nested loops is a bit hard to follow. Are we looking for 'sdch' in accept-encoding header, or something more? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:168: // TODO(rdsmith): This doesn't appear to be from the spec; figure out Umm, so we make 32 byte hash, but only use first 6 bytes? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:171: for (size_t i = 0; i < client_hash.size(); ++i) { one more translation that deserves a function. :-) https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:182: // We explicitly allow either GET or POST methods. Not sure how about meaning/relevance of this comment. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:216: callbacks.swap(callback_vector_); why swap? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:256: const char SdchResponseHandler::kData[] = "<html><body><pre>" Maybe they don't have to be members of the SdchResponseHandler, so they could go to the top of the file? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:358: if (net::URLRequestStatus::SUCCESS != fetcher_status().status() nits: I think || goes to first line and constant goes on the right side of comparison. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:366: "Content-Encoding", "sdch"); nit: would that fit on previous line? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:368: if (SdchResponseHandler::kData != fetcher_response_contents()) Suggest: return fetcher_response_contents() == SdchResponseHandler::kData; https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:387: base::MessageLoopForUI::current()->QuitClosure()); I've been told that RunLoops are preferred to MessageLoops for testing. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:417: if (!second_profile_data_dir_.CreateUniqueTempDirUnderPath(user_data_dir)) Does it ever get cleaned/deleted? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:423: if (!second_profile) it would be NULL anyway... https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:430: Browser* new_browser = DCHECK(profile)?
[Responding to this before the comments, as the resolution affects whether it's worth putting time on this at all :-}.] So I wanted at least one end-to-end smoke test, because I almost committed a stupid that would have broken SDCH in chrome in profile_impl_io_data.cc on one of my earlier CLs. Beyond that, my goal is for these all to test things that are "higher level" than can be easily tested in net_unittests: * Simple: Smoke test. * BrowsingDataRemover: We can test this at the SdchManager::ClearData() level (and I do), but I don't think there's any way to test the integration with BrowsingDataRemover at that level. * Meta-refresh: I could probably skip this one, since it's also tested at the sdch_filter_unittest.cc level. I'll nuke it. * Isolation: I don't see a way to test isolation between different profiles without different profiles. (Yes, I've tested that multiple SdchManagers are isolated from each other, but not the linkage between Profiles and SdchManagers). * NoPost: So this is testing code in URLRequestHttpJob, but when I glanced at that classes unit test, it looked like it was pretty darn minimal. But I'll look into adding a NoPost test to it.
On 2014/07/10 17:57:21, rdsmith wrote: > [Responding to this before the comments, as the resolution affects whether it's > worth putting time on this at all :-}.] Whoops, sorry, if it wasn't clear, I was responding to why browser tests than unit tests. > > So I wanted at least one end-to-end smoke test, because I almost committed a > stupid that would have broken SDCH in chrome in profile_impl_io_data.cc on one > of my earlier CLs. Beyond that, my goal is for these all to test things that > are "higher level" than can be easily tested in net_unittests: > * Simple: Smoke test. > * BrowsingDataRemover: We can test this at the SdchManager::ClearData() level > (and I do), but I don't think there's any way to test the integration with > BrowsingDataRemover at that level. > * Meta-refresh: I could probably skip this one, since it's also tested at the > sdch_filter_unittest.cc level. I'll nuke it. > * Isolation: I don't see a way to test isolation between different profiles > without different profiles. (Yes, I've tested that multiple SdchManagers are > isolated from each other, but not the linkage between Profiles and > SdchManagers). > * NoPost: So this is testing code in URLRequestHttpJob, but when I glanced at > that classes unit test, it looked like it was pretty darn minimal. But I'll > look into adding a NoPost test to it.
https://codereview.chromium.org/380003002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/380003002/diff/20001/chrome/browser/DEPS#newc... chrome/browser/DEPS:87: "+sdch/open-vcdiff", # Just for tests. should be in chrome/browser/net/DEPS, and maybe a per-file rule?
Randy, thanks for the explanation, I think it makes sense. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:578: // Wait until the dictionary fetch actually happens. There seems to be a bit of boilerplate that is repeated in every test. Would it make sense to factor it out? https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:700: incognito_browser->profile()->GetRequestContext(), Would dictionary be retrieved in IncognitoBrowser? If so, should we also check that it doesn't leak back into main profile, or is it covered by 'SecondProfile' scenario?
All comments incorporated, and several tests eliminated or moved to unit tests. Jochen, Misha: WDYT? Jim: I decided to give you a break and ask Misha to do the heavy lifting on this CL, but I'd like you to examine the changes to sdch/* and the addition of sdch/open-vcdiff to the chrome/browser/net/DEPS file (you're also welcome to comment generally if you want, it's just not required). WRT the additions to sdch/*, I'm assuming that since the target type is a static library, adding more .cc files to the library won't bulk out any binaries that don't specifically refer to symbols in those files, i.e. only the test executable, which I don't care about. Please let me know if you see any problems with that plan. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/380003002/diff/20001/chrome/browser/DEPS#newc... chrome/browser/DEPS:87: "+sdch/open-vcdiff", # Just for tests. On 2014/07/11 09:30:08, jochen wrote: > should be in chrome/browser/net/DEPS, and maybe a per-file rule? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:55: bool RequestContainsHeader(const HttpRequestHeaderMap& map, On 2014/07/10 17:08:17, mef wrote: > Should it be called GetRequestHeader? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:113: for (size_t i = 0; i < dictionary_server_hash_.size(); ++i) { On 2014/07/10 17:08:17, mef wrote: > Why do we need this translation? And if we do, I suggest making a function. Sorry, didn't come back and deal with a TODO. I've moved this into a function and documented the need. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:143: while (it != value.end() && IsAsciiAlpha(*it)) On 2014/07/10 17:08:17, mef wrote: > This construct with nested loops is a bit hard to follow. > Are we looking for 'sdch' in accept-encoding header, or something more? Just that. I've added a comment for clarity; let me know what you think. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:168: // TODO(rdsmith): This doesn't appear to be from the spec; figure out On 2014/07/10 17:08:17, mef wrote: > Umm, so we make 32 byte hash, but only use first 6 bytes? The spec says that the dictionary is identified by the first 96 bits (== 12 bytes) and that the client and server identifiers each get six bytes. So "Yes" :-} :-|. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:171: for (size_t i = 0; i < client_hash.size(); ++i) { On 2014/07/10 17:08:17, mef wrote: > one more translation that deserves a function. :-) Actually, it's redundant with the earlier one :-}. I've removed it. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:182: // We explicitly allow either GET or POST methods. On 2014/07/10 17:08:17, mef wrote: > Not sure how about meaning/relevance of this comment. It used to be a DCHECK that we only accept GET. OTOH, post handling is a bit weird. I've pumped up the comment and moved it to a more appropriate place. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:216: callbacks.swap(callback_vector_); On 2014/07/10 17:08:17, mef wrote: > why swap? It means that there isn't any racing on access to the callback_vector_ if there are re-entrant callers. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:256: const char SdchResponseHandler::kData[] = "<html><body><pre>" On 2014/07/10 17:08:17, mef wrote: > Maybe they don't have to be members of the SdchResponseHandler, so they could go > to the top of the file? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:358: if (net::URLRequestStatus::SUCCESS != fetcher_status().status() On 2014/07/10 17:08:17, mef wrote: > nits: I think || goes to first line and constant goes on the right side of > comparison. Whoops, quite right. Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:366: "Content-Encoding", "sdch"); On 2014/07/10 17:08:17, mef wrote: > nit: would that fit on previous line? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:368: if (SdchResponseHandler::kData != fetcher_response_contents()) On 2014/07/10 17:08:17, mef wrote: > Suggest: return fetcher_response_contents() == SdchResponseHandler::kData; Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:387: base::MessageLoopForUI::current()->QuitClosure()); On 2014/07/10 17:08:17, mef wrote: > I've been told that RunLoops are preferred to MessageLoops for testing. Huh. Neat. Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:417: if (!second_profile_data_dir_.CreateUniqueTempDirUnderPath(user_data_dir)) On 2014/07/10 17:08:17, mef wrote: > Does it ever get cleaned/deleted? second_profile_data_dir_ is a ScopedTempDir, so when the member is deleted, the directory should too. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:423: if (!second_profile) On 2014/07/10 17:08:18, mef wrote: > it would be NULL anyway... Good point; done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:430: Browser* new_browser = On 2014/07/10 17:08:18, mef wrote: > DCHECK(profile)? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:578: // Wait until the dictionary fetch actually happens. On 2014/07/11 14:34:36, mef wrote: > There seems to be a bit of boilerplate that is repeated in every test. Would it > make sense to factor it out? Done. https://codereview.chromium.org/380003002/diff/20001/chrome/browser/net/sdch_... chrome/browser/net/sdch_browsertest.cc:700: incognito_browser->profile()->GetRequestContext(), On 2014/07/11 14:34:36, mef wrote: > Would dictionary be retrieved in IncognitoBrowser? If so, should we also check > that it doesn't leak back into main profile, or is it covered by 'SecondProfile' > scenario? It would be retrieved. Incognito profiles and second profiles have very different links and lack of links with main profiles, so I think we should check it separately. Will do.
deps change and gypi change lgtm
Looks pretty good, just a bunch of nits. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:93: // Scanns in a case-insensitive way for |header| in |map|, nit: Scanns https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:98: const char* header, nit: alignment https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:116: for (size_t i = 0; i < output->size(); ++i) { It seems that common practice is to use std::replace, like in: std::replace(output->begin(), output->end(), '+', '-'); std::replace(output->begin(), output->end(), '/', '_'); https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:124: bool ResponseContainsHeaderValue(const net::HttpResponseHeaders& headers, Seems the same as HttpResponseHeaders::HasHeaderValue. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:180: // Scan comma separated list of encodings for case insensitive match Suggest: Use base/strings/string_tokenizer.h https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:215: DCHECK_NE(encoded_data_, ""); Given that this is a unit test, should DCHECKs be ASSERTs or EXPECTs? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:236: DCHECK_EQ(request.relative_url, "/dict"); thought: Should "/dict" and such be defined constants? I'm not sure. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:256: base::Bind(&SdchResponseHandler::WaitAndGetRequestVector, I'm a bit unhappy about polling loop here, but I'm not sure how to make it better. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:292: // ** Helper functions for fetching data. nit: Do we need **? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:295: bool use_post) { nit: use_post may fit on previous line. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:314: void FetchUrl(GURL url) { nit: I think we separate methods by blank lines. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:317: int fetcher_response_code() { return fetcher_response_code_; } nit: const (also below). https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:324: GURL data_url() { nit: it's not a simple accessor, so should be GetDataUrl()? Also, do you need similar GetDictUrl()? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:335: bool use_post, FWIW it seems that |use_post| is never true any more. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:476: if (!have_dict_header) return false; nit: Should this be ASSERT instead as we exit anyway? Also below. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:488: if (request_vector[1].relative_url != "/dict") return false; Maybe making "/dict" and "/data" explicit constants makes sense... https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:537: static void NukeSdchDictionariesOnIOThread( I presume these are not overrides from InProcessBrowserTest, so maybe they should go above. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:560: fetcher_response_code_ = source->GetResponseCode(); Do we really need to copy them into this class? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:579: base::ScopedTempDir second_profile_data_dir_; Would it get deleted if profile is still open? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:582: const char SdchBrowserTest::kTestHost[] = "our.test.host.com";; nit: ;; https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:613: EXPECT_TRUE(GetDataDetailed( nit: Could use GetData() instead. Also below. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:636: EXPECT_FALSE(sdch_encoding_used); Could it happen that SDCH dictionary is left over from previous test on main profile? Would it make this test flaky? https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.c... net/base/sdch_manager.cc:563: int SdchManager::GetFetchesRequestedForTesting() const { nit: should this be inline in header? Also should it match the variable name (maybe rename to |fetches_count_for_testing_|? https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... File net/filter/mock_filter_context.h (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... net/filter/mock_filter_context.h:38: // Make all interfaces that might be unstable after a URLRequests nit: nl above. https://codereview.chromium.org/380003002/diff/100001/net/filter/sdch_filter_... File net/filter/sdch_filter_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/sdch_filter_... net/filter/sdch_filter_unittest.cc:201: filter_context()->SetRequestTime(base::Time()); I'm not sure what this does. Should there be some ASSERT/EXPECT here? https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... net/http/http_transaction_test_util.h:231: const net::HttpRequestInfo* http_request_info() const { nit: nl above, maybe name it |request()| to match variable name. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:58: base::WeakPtr<MockNetworkTransaction> xtion( nit: maybe name it |transaction|? https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:70: EXPECT_TRUE(get_success); nit: replace with ASSERT?
Misha, Jim: PTAL? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:93: // Scanns in a case-insensitive way for |header| in |map|, On 2014/07/23 18:34:28, mef wrote: > nit: Scanns Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:98: const char* header, On 2014/07/23 18:34:29, mef wrote: > nit: alignment Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:116: for (size_t i = 0; i < output->size(); ++i) { 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:124: bool ResponseContainsHeaderValue(const net::HttpResponseHeaders& headers, On 2014/07/23 18:34:28, mef wrote: > Seems the same as HttpResponseHeaders::HasHeaderValue. Thanks for the catch. Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:180: // Scan comma separated list of encodings for case insensitive match 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:215: DCHECK_NE(encoded_data_, ""); 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:236: DCHECK_EQ(request.relative_url, "/dict"); 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:256: base::Bind(&SdchResponseHandler::WaitAndGetRequestVector, 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:292: // ** Helper functions for fetching data. 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:295: bool use_post) { On 2014/07/23 18:34:29, mef wrote: > nit: use_post may fit on previous line. Not in my checkout :-{. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:314: void FetchUrl(GURL url) { 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:317: int fetcher_response_code() { return fetcher_response_code_; } On 2014/07/23 18:34:29, mef wrote: > nit: const (also below). Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:324: GURL data_url() { 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:335: bool use_post, On 2014/07/23 18:34:29, mef wrote: > FWIW it seems that |use_post| is never true any more. Good point. Gone. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:476: if (!have_dict_header) return false; 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:488: if (request_vector[1].relative_url != "/dict") return false; On 2014/07/23 18:34:28, mef wrote: > Maybe making "/dict" and "/data" explicit constants makes sense... Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:537: static void NukeSdchDictionariesOnIOThread( On 2014/07/23 18:34:29, mef wrote: > I presume these are not overrides from InProcessBrowserTest, so maybe they > should go above. Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:560: fetcher_response_code_ = source->GetResponseCode(); 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:579: base::ScopedTempDir second_profile_data_dir_; 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:582: const char SdchBrowserTest::kTestHost[] = "our.test.host.com";; On 2014/07/23 18:34:29, mef wrote: > nit: ;; Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:613: EXPECT_TRUE(GetDataDetailed( 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:636: EXPECT_FALSE(sdch_encoding_used); 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). https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.c... net/base/sdch_manager.cc:563: int SdchManager::GetFetchesRequestedForTesting() const { On 2014/07/23 18:34:29, mef wrote: > nit: should this be inline in header? Also should it match the variable name > (maybe rename to |fetches_count_for_testing_|? I think there's some auto-analysis of method names that warns if we call a ForTesting() method outside of a test, so I'd like to keep the CamelCase name. I've changed the variable name to fetches_count_for_testing_, made the method name match that, and moved the implementation into the header. https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... File net/filter/mock_filter_context.h (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... net/filter/mock_filter_context.h:38: // Make all interfaces that might be unstable after a URLRequests On 2014/07/23 18:34:30, mef wrote: > nit: nl above. Done. https://codereview.chromium.org/380003002/diff/100001/net/filter/sdch_filter_... File net/filter/sdch_filter_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/sdch_filter_... net/filter/sdch_filter_unittest.cc:201: filter_context()->SetRequestTime(base::Time()); On 2014/07/23 18:34:30, mef wrote: > I'm not sure what this does. Should there be some ASSERT/EXPECT here? Added an explanatory comment (I'm trying to make things as unreliable as they might get in the wild for the SdchFilter destructor). https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... net/http/http_transaction_test_util.h:231: const net::HttpRequestInfo* http_request_info() const { On 2014/07/23 18:34:30, mef wrote: > nit: nl above, maybe name it |request()| to match variable name. The nl doesn't fit to my eye with the relationship of the two methods above--I take this as "simple accessors are clustered together without newlines separating them", but if you feel strongly I'll change it. I've changed the name. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:58: base::WeakPtr<MockNetworkTransaction> xtion( On 2014/07/23 18:34:30, mef wrote: > nit: maybe name it |transaction|? Done. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:70: EXPECT_TRUE(get_success); On 2014/07/23 18:34:30, mef wrote: > nit: replace with ASSERT? See comment elsewhere in this review--I don't think that works in helper routines.
lgtm https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:180: // Scan comma separated list of encodings for case insensitive match 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. :) https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:215: DCHECK_NE(encoded_data_, ""); 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:256: base::Bind(&SdchResponseHandler::WaitAndGetRequestVector, 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:314: void FetchUrl(GURL url) { 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. :) https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:476: if (!have_dict_header) return false; 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:579: base::ScopedTempDir second_profile_data_dir_; 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:613: EXPECT_TRUE(GetDataDetailed( 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:636: EXPECT_FALSE(sdch_encoding_used); 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. https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/base/sdch_manager.c... net/base/sdch_manager.cc:563: int SdchManager::GetFetchesRequestedForTesting() const { On 2014/07/29 23:44:21, rdsmith wrote: > On 2014/07/23 18:34:29, mef wrote: > > nit: should this be inline in header? Also should it match the variable name > > (maybe rename to |fetches_count_for_testing_|? > > I think there's some auto-analysis of method names that warns if we call a > ForTesting() method outside of a test, so I'd like to keep the CamelCase name. > I've changed the variable name to fetches_count_for_testing_, made the method > name match that, and moved the implementation into the header. I see, nice to know. https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... File net/http/http_transaction_test_util.h (right): https://codereview.chromium.org/380003002/diff/100001/net/http/http_transacti... net/http/http_transaction_test_util.h:231: const net::HttpRequestInfo* http_request_info() const { On 2014/07/29 23:44:22, rdsmith wrote: > On 2014/07/23 18:34:30, mef wrote: > > nit: nl above, maybe name it |request()| to match variable name. > > The nl doesn't fit to my eye with the relationship of the two methods above--I > take this as "simple accessors are clustered together without newlines > separating them", but if you feel strongly I'll change it. I've changed the > name. Acknowledged. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:70: EXPECT_TRUE(get_success); On 2014/07/29 23:44:22, rdsmith wrote: > On 2014/07/23 18:34:30, mef wrote: > > nit: replace with ASSERT? > > See comment elsewhere in this review--I don't think that works in helper > routines. Hmm, I thought that the only difference between EXPECT_TRUE and ASSERT_TRUE is fatality. https://codereview.chromium.org/380003002/diff/120001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/120001/net/base/sdch_manager.c... net/base/sdch_manager.cc:83: if (target_url.SchemeIsSecure() != url_.SchemeIsSecure()) Is there a problem with advertising a dictionary retrieved over https for http request?
... just a pile of nits I think... https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:102: nit: minimize vertical whitespace... and I doubt it helps here. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:295: bool use_post) { nit: When args in definition or declaration don't fit on one line, put each arg on a separate line. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:314: void FetchUrl(GURL url) { 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:324: GURL data_url() { +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: > 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. > > https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:362: // ** Client information and control. 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:373: run_loop.Run(); 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(). https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:410: return second_profile; nit: probably wasn't worth defining a variable here. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:427: // ** Server information and control. nit: again, kill "**", and use words. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_io_data.cc:266: sdch_manager_->set_sdch_fetcher(fetcher.Pass()); nit: What was the significance of this change? Why put it in a variable first? Was the code too long to be readable? If so, perhaps the real offender is the argument starting at line 262, and perhaps that should go in a varible... and then you won't need a multi-line comment in the *middle* of a function call. WDYT? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:507: sdch_manager_->set_sdch_fetcher(fetcher.Pass()); nit: perhaps putting results from new at line 503 in a temporary would be a more readable cleanup? WDYT? https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... File net/filter/mock_filter_context.h (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... net/filter/mock_filter_context.h:39: // destruction be so. nit: comment seemed clumsy... or at least I had to read it many times to make sense of it. Does the comment really say "make interfaces unstable" because they would be unstable in real life after a call to URLRequests? ... or do you mean to make them invalid after a call to URLRequests? Suggest perhaps: // After URLRequests is called, some interfaces may become unstable. This method is used to signal that state, so we can tag calls to those interfaces as coding errors. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:85: context_.SetSdchManager((scoped_ptr<SdchManager>(new SdchManager)).Pass()); nit: Do you need extra parens? Wouldn't it be readable as: context_.SetSdchManager(scoped_ptr<SdchManager>(new SdchManager).Pass()); https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:162: req_.set_method("GET"); // Redundant with default. nit: 2 spaces before comments... unless you're lining them up.
Thanks, Misha! Jim: Thank you for the comments, and I'll go through them, but because I didn't see any on the sdch/ directory I wanted to explicitly make sure you had looked at those changes and were good with them. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:70: EXPECT_TRUE(get_success); On 2014/07/30 17:14:58, mef wrote: > On 2014/07/29 23:44:22, rdsmith wrote: > > On 2014/07/23 18:34:30, mef wrote: > > > nit: replace with ASSERT? > > > > See comment elsewhere in this review--I don't think that works in helper > > routines. > Hmm, I thought that the only difference between EXPECT_TRUE and ASSERT_TRUE is > fatality. The issue is how that fatality is implemented. We don't crash the program; we want to execute the rest of the test harness (& e.g. the next test). But we don't want to execute any more test code. So unlike the EXPECT_* macros, the ASSERT* macros turn into "return X;", and the type and nature of that return have an implicit assumption that they're returning from a function declared with TEST*(...). I think (reading the code) that the type of that return is void, so if you're inside a helper function that doesn't return a type, it will compile. But you'll then continue on to execute all the code following the call to that helper function. And if the helper function doesn't return void, you won't compile. (I may have the compile-friendly type wrong, but it doesn't really matter; it's not that things work in that case, they just fail more subtly :-}.) See the definition of ASSERT_TRUE in testing/gtest/include/gtest/gtest.h; it resovles to GTEST_TEST_BOOLEAN_ (testing/gtest/include/gtest/internal/gtest-internal.h) with a "fail" argument of GTEST_FATAL_FAILURE_ (same file). GTEST_FATAL_FAILURE involves a return statement, which is executed if the check fails. https://codereview.chromium.org/380003002/diff/120001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/380003002/diff/120001/net/base/sdch_manager.c... net/base/sdch_manager.cc:83: if (target_url.SchemeIsSecure() != url_.SchemeIsSecure()) On 2014/07/30 17:14:58, mef wrote: > Is there a problem with advertising a dictionary retrieved over https for http > request? It leaks that we got the dictionary. "Problem" is a value judgement :-}. Chris explores this in https://groups.google.com/forum/#!topic/sdch/8EEBFYMIB78; I'm following that proposal.
You might also indicate why you chose to (or needed to) build the other files that were not being built. Was it for test *only*? (instead of using hard-wired test strings). https://codereview.chromium.org/380003002/diff/120001/sdch/sdch.gyp File sdch/sdch.gyp (right): https://codereview.chromium.org/380003002/diff/120001/sdch/sdch.gyp#newcode37 sdch/sdch.gyp:37: 'open-vcdiff/src/jsonwriter.h', I didn't think to read this carefully.... but since you asked about my thought on the "sdch" directory... and this is the only such entry... Why are we using their jsonwriter? Don't we have such functionality in Chromium? take a look in base/json/* base/json/json_writer.cc for example.
Message was sent while issue was closed.
All comments incorporated. Jim: I've updated the CL description to try and answer why I used the vcdiff encoding code rather than hard-encode a dictionary; let me know what you think. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:102: On 2014/07/31 03:24:34, jar wrote: > nit: minimize vertical whitespace... and I doubt it helps here. Yeah, typo. Done. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:295: bool use_post) { 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:314: void FetchUrl(GURL url) { 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 :-}. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:324: GURL data_url() { 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. > > 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. > > > > > https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:362: // ** Client information and control. 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. > 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. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:373: run_loop.Run(); 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? https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:410: return second_profile; 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 :-}. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:427: // ** Server information and control. On 2014/07/31 03:24:33, jar wrote: > nit: again, kill "**", and use words. Moot. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_io_data.cc:266: sdch_manager_->set_sdch_fetcher(fetcher.Pass()); On 2014/07/31 03:24:34, jar wrote: > nit: What was the significance of this change? Why put it in a variable first? > Was the code too long to be readable? > > If so, perhaps the real offender is the argument starting at line 262, and > perhaps that should go in a varible... and then you won't need a multi-line > comment in the *middle* of a function call. WDYT? The main issue was that consing up a scoped_ptr<> in the middle of a setter looked like a lot of syntactic sugar to me. But I've rewritten it and I think it's a bit better, even if not trivial to understand. Let me know if the new version is a problem. https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:507: sdch_manager_->set_sdch_fetcher(fetcher.Pass()); On 2014/07/31 03:24:34, jar wrote: > nit: perhaps putting results from new at line 503 in a temporary would be a more > readable cleanup? WDYT? See earlier reply. https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... File net/filter/mock_filter_context.h (right): https://codereview.chromium.org/380003002/diff/100001/net/filter/mock_filter_... net/filter/mock_filter_context.h:39: // destruction be so. On 2014/07/31 03:24:34, jar wrote: > nit: comment seemed clumsy... or at least I had to read it many times to make > sense of it. Does the comment really say "make interfaces unstable" because > they would be unstable in real life after a call to URLRequests? ... or do > you mean to make them invalid after a call to URLRequests? s/URLRequests/URLRequest's destructor/. I mean to make them invalid after a call to the destructor because they may be so in real life. > > Suggest perhaps: > > // After URLRequests is called, some interfaces may become unstable. This method > is used to signal that state, so we can tag calls to those interfaces as coding > errors. Done. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:85: context_.SetSdchManager((scoped_ptr<SdchManager>(new SdchManager)).Pass()); On 2014/07/31 03:24:34, jar wrote: > nit: Do you need extra parens? Wouldn't it be readable as: > > context_.SetSdchManager(scoped_ptr<SdchManager>(new SdchManager).Pass()); Huh, it compiles. Thanks for the catch; done. https://codereview.chromium.org/380003002/diff/100001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:162: req_.set_method("GET"); // Redundant with default. On 2014/07/31 03:24:34, jar wrote: > nit: 2 spaces before comments... unless you're lining them up. Done. https://codereview.chromium.org/380003002/diff/120001/sdch/sdch.gyp File sdch/sdch.gyp (right): https://codereview.chromium.org/380003002/diff/120001/sdch/sdch.gyp#newcode37 sdch/sdch.gyp:37: 'open-vcdiff/src/jsonwriter.h', On 2014/07/31 23:19:23, jar wrote: > I didn't think to read this carefully.... but since you asked about my thought > on the "sdch" directory... and this is the only such entry... To be explicit, the things I need sdch/OWNERS review on are this change and the inclusion of it in chrome/browser/net/DEPS. > Why are we using their jsonwriter? Don't we have such functionality in > Chromium? > > take a look in base/json/* > > base/json/json_writer.cc for example. The issue is that, in adding the vcencoder.cc file, I ended up with a undefined symbol which was to the jsonwriter. I'm very reluctant to try and hook the vcencoder.cc file up to our jsonwriter if it was written in parallel with and to depend on the SDCH jsonwriter file.
Message was sent while issue was closed.
Comments... nits... and then LGTM https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:373: run_loop.Run(); 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. https://codereview.chromium.org/380003002/diff/160001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/160001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:291: int fetcher_response_code() const { nit: please use bouncy caps, instead of hacker style. FetcherResponseCode Generally the hacker style is only used for the simplest of getters and setters. Same nit below on lines 296, and 301. If you really want to argue, I can live with it, but I find it confusing when I can't read a hacker style function name and immediately mentally convert it to code. https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... File net/base/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... net/base/sdch_dictionary_fetcher.h:35: // SdchDictionaryFetcher takes a reference to |*context|. nit: I had to read this several times, and look at code (members) to understand the comment. The use of the word "reference" was confusing. Could you say: // The constructor takes ownership of |*context| and will destroy it when done. .or... ...destroy it when this is destroyed. or simply.... // The constructor takes ownership of |*context|. https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_manager.h... net/base/sdch_manager.h:339: int GetFetchesCountForTesting() const { nit: FWIW: This is a place where hacker style would be common: get_fetches_count_for_testing()
https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:373: run_loop.Run(); 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. https://codereview.chromium.org/380003002/diff/160001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/160001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:291: int fetcher_response_code() const { On 2014/08/13 01:28:58, jar wrote: > nit: please use bouncy caps, instead of hacker style. > > FetcherResponseCode > > Generally the hacker style is only used for the simplest of getters and setters. > > > Same nit below on lines 296, and 301. > > If you really want to argue, I can live with it, but I find it confusing when I > can't read a hacker style function name and immediately mentally convert it to > code. No argument at all; sorry they looked like this. It was historical; they started out simple and got more complex. I did convert fetcher_status() as well, just for consistency. Done. https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... File net/base/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... net/base/sdch_dictionary_fetcher.h:35: // SdchDictionaryFetcher takes a reference to |*context|. On 2014/08/13 01:28:59, jar wrote: > nit: I had to read this several times, and look at code (members) to understand > the comment. The use of the word "reference" was confusing. Could you say: > > // The constructor takes ownership of |*context| and will destroy it when done. > > .or... > > ...destroy it when this is destroyed. > > or simply.... > > // The constructor takes ownership of |*context|. So I'll say that if you ask me again, but it because URLRequestContextGetter is refcounted, it's not true. Ownership could be shared between many objects. Do you still want me to say "takes ownership of"? If it was as simple as taking ownership, this would be a scoped_ptr<> and I wouldn't have bothered with a comment :-J. https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_manager.h File net/base/sdch_manager.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_manager.h... net/base/sdch_manager.h:339: int GetFetchesCountForTesting() const { On 2014/08/13 01:28:59, jar wrote: > nit: FWIW: This is a place where hacker style would be common: > get_fetches_count_for_testing() Agreed; I just know there is automated scanning and verification of *ForTesting() routines, and I don't know that that's also done for *_for_testing() routines. Want me to switch anyway?
still LGTM https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/100001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:373: run_loop.Run(); Independent of the performance... I'm OK with this form. On 2014/08/13 02:05:25, rdsmith wrote: > 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. https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... File net/base/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... net/base/sdch_dictionary_fetcher.h:35: // SdchDictionaryFetcher takes a reference to |*context|. On 2014/08/13 02:05:26, rdsmith wrote: > On 2014/08/13 01:28:59, jar wrote: > > nit: I had to read this several times, and look at code (members) to > understand > > the comment. The use of the word "reference" was confusing. Could you say: > > > > // The constructor takes ownership of |*context| and will destroy it when > done. > > > > .or... > > > > ...destroy it when this is destroyed. > > > > or simply.... > > > > // The constructor takes ownership of |*context|. > > So I'll say that if you ask me again, but it because URLRequestContextGetter is > refcounted, it's not true. Ownership could be shared between many objects. Do > you still want me to say "takes ownership of"? > > If it was as simple as taking ownership, this would be a scoped_ptr<> and I > wouldn't have bothered with a comment :-J. Ah... ok... with that context, I see how I misread/confused the comment. When I was "take a reference" I somehow misread it to be a "C++ reference" as in stuff like int &foo = goo; or similar passed in as a "reference argument." Would you be willing to write: "...adds a reference to ..." Would that work for you? ...or is this comment sooooo standard, it is only being misread by me, perhaps due to caffeine?? I'll leave it to you to decide... I can now live with either, but would (I think) prefer the suggested change. Alternatively... any chance we can pass in a refptr so that it is not necessary to make this comment?
Thanks! https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... File net/base/sdch_dictionary_fetcher.h (right): https://codereview.chromium.org/380003002/diff/160001/net/base/sdch_dictionar... net/base/sdch_dictionary_fetcher.h:35: // SdchDictionaryFetcher takes a reference to |*context|. On 2014/08/14 22:33:25, jar wrote: > On 2014/08/13 02:05:26, rdsmith wrote: > > On 2014/08/13 01:28:59, jar wrote: > > > nit: I had to read this several times, and look at code (members) to > > understand > > > the comment. The use of the word "reference" was confusing. Could you say: > > > > > > // The constructor takes ownership of |*context| and will destroy it when > > done. > > > > > > .or... > > > > > > ...destroy it when this is destroyed. > > > > > > or simply.... > > > > > > // The constructor takes ownership of |*context|. > > > > So I'll say that if you ask me again, but it because URLRequestContextGetter > is > > refcounted, it's not true. Ownership could be shared between many objects. > Do > > you still want me to say "takes ownership of"? > > > > If it was as simple as taking ownership, this would be a scoped_ptr<> and I > > wouldn't have bothered with a comment :-J. > > Ah... ok... with that context, I see how I misread/confused the comment. When I > was "take a reference" I somehow misread it to be a "C++ reference" as in stuff > like > > int &foo = goo; > > or similar passed in as a "reference argument." > > Would you be willing to write: > > "...adds a reference to ..." > > Would that work for you? ...or is this comment sooooo standard, it is only > being misread by me, perhaps due to caffeine?? > > I'll leave it to you to decide... I can now live with either, but would (I > think) prefer the suggested change. > > Alternatively... any chance we can pass in a refptr so that it is not necessary > to make this comment? There was some reason that was a hassle when I first started this, I think because of how complex the instantiation logic calling the constructor was, but with the earlier rewrite, this looks doable, and as such, definitely the right idea. Done.
Joao: You're the person who last touched the line on which I'm getting a CHECK() failure; would you be willing to give me a quick consult? Details below. https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:594: second_browser()->profile()->GetRequestContext(), &sdch_encoding_used)); I'm getting a check failure at user_cloud_policy_manager_factory_chromeos.cc::130 when running this test. I presume the problem is that you can't create a new profile on ChromeOS without tying it to an active Google account. What's the right way to handle this; should I #ifdef out this test on ChromeOS, or is there some other way to handle the issue?
Hi Randy, see inline for an explanation & a quick fix :-) https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:594: second_browser()->profile()->GetRequestContext(), &sdch_encoding_used)); On 2014/08/18 23:32:04, rdsmith wrote: > I'm getting a check failure at > user_cloud_policy_manager_factory_chromeos.cc::130 when running this test. I > presume the problem is that you can't create a new profile on ChromeOS without > tying it to an active Google account. What's the right way to handle this; > should I #ifdef out this test on ChromeOS, or is there some other way to handle > the issue? I think there's value in getting test coverage for this on ChromeOS. The problem is that multi-profiles works differently on ChromeOS, and what SetupSecondBrowser() is doing won't fly on that platform (because there is no User associated with the second Profile). The user_cloud_policy_manager_factory_chromeos.cc is just the first of many things that will break :-) IIUC this test cares about having a 2nd RequestContext and doesn't care so much about the login state of the session. So the easiest fix is to always use the same User for every Profile, instead of adding a lot of setup code just for OS_CHROMEOS. This can be easily achieved by adding this to SetUpCommandLine: #if defined(OS_CHROMEOS) command_line->AppendSwitch( chromeos::switches::kIgnoreUserProfileMappingForTests); #endif The flag comes from chromeos/chromeos_switches.h and needs the OS_CHROMEOS guard too.
https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... File chrome/browser/net/sdch_browsertest.cc (right): https://codereview.chromium.org/380003002/diff/260001/chrome/browser/net/sdch... chrome/browser/net/sdch_browsertest.cc:594: second_browser()->profile()->GetRequestContext(), &sdch_encoding_used)); On 2014/08/19 08:24:47, Joao da Silva wrote: > On 2014/08/18 23:32:04, rdsmith wrote: > > I'm getting a check failure at > > user_cloud_policy_manager_factory_chromeos.cc::130 when running this test. I > > presume the problem is that you can't create a new profile on ChromeOS without > > tying it to an active Google account. What's the right way to handle this; > > should I #ifdef out this test on ChromeOS, or is there some other way to > handle > > the issue? > > I think there's value in getting test coverage for this on ChromeOS. The problem > is that multi-profiles works differently on ChromeOS, and what > SetupSecondBrowser() is doing won't fly on that platform (because there is no > User associated with the second Profile). > > The user_cloud_policy_manager_factory_chromeos.cc is just the first of many > things that will break :-) > > IIUC this test cares about having a 2nd RequestContext and doesn't care so much > about the login state of the session. So the easiest fix is to always use the > same User for every Profile, instead of adding a lot of setup code just for > OS_CHROMEOS. This can be easily achieved by adding this to SetUpCommandLine: > > #if defined(OS_CHROMEOS) > command_line->AppendSwitch( > chromeos::switches::kIgnoreUserProfileMappingForTests); > #endif > > The flag comes from chromeos/chromeos_switches.h and needs the OS_CHROMEOS guard > too. Thanks! That did indeed fix the problem.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/380003002/280001
Message was sent while issue was closed.
Committed patchset #15 (280001) as 290638 |
