Sdch view for net-internals

net/base/sdch_dictionary_fetcher.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/sdch_dictionary_fetcher.h"
 
 #include "base/bind.h"
 #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);
 }
 
 SdchDictionaryFetcher::~SdchDictionaryFetcher() {
   DCHECK(CalledOnValidThread());
 }
 
 void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
 
   // Avoid pushing duplicate copy onto queue.  We may fetch this url again later
   // 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);
   fetch_queue_.push(dictionary_url);
   ScheduleDelayedRun();
 }
 
 void SdchDictionaryFetcher::Cancel() {
   DCHECK(CalledOnValidThread());
 
@@ skipping 33 matching lines @@
   current_fetch_->Start();
 }
 
 void SdchDictionaryFetcher::OnURLFetchComplete(
     const URLFetcher* source) {
   DCHECK(CalledOnValidThread());
   if ((200 == source->GetResponseCode()) &&
       (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