Improve syncing between dom distiller and the print preview request

components/dom_distiller/core/dom_distiller_service.cc

 // Copyright 2013 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 "components/dom_distiller/core/dom_distiller_service.h"
 
 #include <utility>
 
 #include "base/guid.h"
 #include "base/location.h"
@@ skipping 171 matching lines @@
   // If a distiller is already running for one URL, don't start another.
   if (was_created) {
     task_tracker->StartDistiller(distiller_factory_.get(),
                                  std::move(distiller_page));
     task_tracker->StartBlobFetcher();
   }
 
   return viewer_handle;
 }
 
+scoped_ptr<ViewerHandle> DomDistillerService::AddViewRequestDelegate(
+    ViewRequestDelegate* delegate,
+    const GURL& url) {
+  TaskTracker* task_tracker = nullptr;
+  GetOrCreateTaskTrackerForUrl(url, &task_tracker);

[nyquist, 2016/01/20 16:59:08]

If this returns true, doesn't that mean that there was no task tracker from
before? And if so; can we safely assume that a distillation will soon start? Or
should this function just return nullptr in that case?

[mvendramini_hp, 2016/01/21 16:54:46]

Yes, you are right - in hindsight, that was not a very good idea. nyquist what
do you think if I'd just replace that with GetTaskTrackerForUrl() instead? And
then return nullptr if it does not exist? This would still work with the current
code (since before we call AddViewRequestDelegate() we already called
DistillAndView() - which creates the TaskTracker when calling VIewUrl() on
MaybeStartDistillation(), and would have the added benefit of discouraging bad
usage in the future - please share your thoughts.

[nyquist, 2016/01/22 02:23:29]

Yeah; I think GetTaskTrackerForUrl makes more sense. You have to make it very
explicit in the API documentation though, that the code only works if there's
already a distillation going on.

And like I mentioned in the other comment; there must be a clear documentation
as to if and how the callbacks on ViewRequestDelegate is invoked.

[mvendramini_hp, 2016/01/22 12:22:31]

Done, adapted the code to not modify DomDistillerService's internal state
(tasks_) by using only GetTaskTrackerForUrl().

Also, we added some starting documentation to the header file - please check it
out.
One thing still missing in the doc: proper explanation on the order of the
callbacks - I could really use some help here (cjhopman?). As far as I know,
we'd just add the delegate to the tracker, and at that point on we'd get
callbacks. I'm not sure what kind of guarantees can be reasonably promised on
the API as far as IPC arrival goes - please advise.

+  return task_tracker->AddViewer(delegate);
+}
+
 bool DomDistillerService::GetOrCreateTaskTrackerForUrl(
     const GURL& url,
     TaskTracker** task_tracker) {
   ArticleEntry entry;
   if (store_->GetEntryByUrl(url, &entry)) {
     return GetOrCreateTaskTrackerForEntry(entry, task_tracker);
   }
 
   *task_tracker = GetTaskTrackerForUrl(url);
   if (*task_tracker) {
@@ skipping 74 matching lines @@
 void DomDistillerService::RemoveObserver(DomDistillerObserver* observer) {
   DCHECK(observer);
   store_->RemoveObserver(observer);
 }
 
 DistilledPagePrefs* DomDistillerService::GetDistilledPagePrefs() {
   return distilled_page_prefs_.get();
 }
 
 }  // namespace dom_distiller