Sdch view for net-internals

net/base/sdch_dictionary_fetcher.cc

Index: net/base/sdch_dictionary_fetcher.cc
diff --git a/net/base/sdch_dictionary_fetcher.cc b/net/base/sdch_dictionary_fetcher.cc
index aa2061eba8e1da490096104b492020374e1b4439..cc3488a1178a247a5eda3802688362482faa0e98 100644
--- a/net/base/sdch_dictionary_fetcher.cc
+++ b/net/base/sdch_dictionary_fetcher.cc
@@ -8,19 +8,22 @@
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "net/base/load_flags.h"
+#include "net/base/sdch_net_log_params.h"
 #include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
 namespace net {
 
-SdchDictionaryFetcher::SdchDictionaryFetcher(
-    SdchManager* manager,
-    URLRequestContextGetter* context)
+SdchDictionaryFetcher::SdchDictionaryFetcher(SdchManager* manager,
+                                             URLRequestContextGetter* context)
     : manager_(manager),
       weak_factory_(this),
       task_is_pending_(false),
-      context_(context) {
+      context_(context),
+      net_log_(BoundNetLog::Make(context_->GetURLRequestContext()->net_log(),
+                                 NetLog::SOURCE_SDCH_DICTIONARY_FETCHER)) {
   DCHECK(CalledOnValidThread());
   DCHECK(manager);
 }
@@ -36,13 +39,13 @@ void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   // and get a different dictionary, but there is no reason to have it in the
   // queue twice at one time.
   if (!fetch_queue_.empty() && fetch_queue_.back() == dictionary_url) {

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

Huh.  It looks to me like this line is in error; we should actually scan the
entire queue, not just look at the last entry.  Willing to make the change while
you're here?

[baranovich, 2014/08/13 19:13:47]

I think it's done more or less by purpose, since |attempted_load_| contains
everything from the |fetch_queue_|, but the last one is just shorter and works
faster on.
On 2014/08/13 17:35:45, rdsmith wrote:

-    SdchManager::SdchErrorRecovery(
-        SdchManager::DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD);
+    LogSdchProblem(SdchManager::DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD,
+                   dictionary_url);

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

I'm not sure this counts as an error that should be flashing red on the event
list.  Maybe plumb some way to log it s.t. the net error will be OK?

[baranovich, 2014/08/13 19:13:47]

Done.

     return;
   }
   if (attempted_load_.find(dictionary_url) != attempted_load_.end()) {
-    SdchManager::SdchErrorRecovery(
-        SdchManager::DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD);
+    LogSdchProblem(SdchManager::DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD,
+                   dictionary_url);

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

Similar; this could be the result of a race, and I wouldn't think should show up
as an error.

[baranovich, 2014/08/13 19:13:47]

Done.

     return;
   }
   attempted_load_.insert(dictionary_url);
@@ -96,10 +99,21 @@ void SdchDictionaryFetcher::OnURLFetchComplete(
       (source->GetStatus().status() == URLRequestStatus::SUCCESS)) {
     std::string data;
     source->GetResponseAsString(&data);
-    manager_->AddSdchDictionary(data, source->GetURL());
+    SdchManager::ProblemCodes problem;
+    manager_->AddSdchDictionary(data, source->GetURL(), &problem);
+    if (problem != SdchManager::PROBLEM_CODE_OK)
+      LogSdchProblem(problem, source->GetURL());

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

I'm a bit uncomfortable with the implication of this call, which I read as that
true will be returned if-and-only-if the returned problem is PROBLEM_CODE_OK. 
Why duplicate the information?  It seems like it's asking for skew down the
line.  

If you're not comfortable just returning a problem code, could you check the
boolean return here?

[baranovich, 2014/08/13 18:38:58]

PROBLEM_CODE_OK may be even if we haven't added the dictionary (if SDCH is
disabled, for instance).

On 2014/08/13 17:35:45, rdsmith wrote:

[Randy Smith (Not in Mondays), 2014/08/19 19:00:53]

As noted in my other comment, I think we're ok adding problem codes without
messing with the histogram, and I think that frees us up to be cleaner about API
specification, which is important to me.  My understanding of the primary reason
why we haven't just added new values for the conditions in which
AddSdchDictionary can not succeed is my comment about not changing the
histogram; if you agree with that, could you just add new values to cover those
cases?

(I'm partially guided in this by the fact that we don't ever *check* the return
value of AddSdchDictionary, which makes me think we're doing it wrong.)

   }
   current_fetch_.reset(NULL);
   ScheduleDelayedRun();
 }
 
+void SdchDictionaryFetcher::LogSdchProblem(SdchManager::ProblemCodes problem,
+                                           GURL url) const {
+  SdchManager::SdchErrorRecovery(problem);
+  net_log_.AddEvent(
+      NetLog::TYPE_SDCH_DICTIONARY_FETCH_ERROR,
+      base::Bind(&NetLogSdchDictionaryFetchProblemCallback, problem, &url));
+}
+
 }  // namespace net