[Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the …

components/offline_pages/core/snapshot_controller.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_SNAPSHOT_CONTROLLER_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_SNAPSHOT_CONTROLLER_H_
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 49 matching lines @@
     virtual ~Client() {}
   };
 
   SnapshotController(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       SnapshotController::Client* client);
   SnapshotController(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       SnapshotController::Client* client,
       int64_t delay_after_document_available_ms,
-      int64_t delay_after_document_on_load_completed_ms);
+      int64_t delay_after_document_on_load_completed_ms,
+      bool consider_document_available_for_snapshot);
   virtual ~SnapshotController();
 
   // Resets the 'session', returning controller to initial state.
   void Reset();
 
   // Stops current session, no more Client::StartSnapshot calls will be
   // invoked from the SnapshotController until current session is Reset().
   // Called by Client, for example when it encounters an error loading the page.
   void Stop();
 
@@ skipping 15 matching lines @@
  private:
   void MaybeStartSnapshot(PageQuality updated_page_quality);
   void MaybeStartSnapshotThenStop();
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   // Client owns this class.
   SnapshotController::Client* client_;
   SnapshotController::State state_;
   int64_t delay_after_document_available_ms_;
   int64_t delay_after_document_on_load_completed_ms_;
+  bool consider_document_available_for_snapshot_;

[carlosk, 2017/04/19 23:16:57]

nit: it seems to me that prefixing with "consider" makes its intent less less
clear than it would be with "ignore".

[fgorski, 2017/04/20 16:39:05]

I too have hard time reading this variable:

document_available_triggers_snapshot_ maybe?

[chili, 2017/04/21 00:12:54]

I think I prefer a "positive" over "negative" link. i.e. If I change the name to
"ShouldIgnoreDocumentAvailableForSnapshot", then passing 'false' would imply we
shouldn't ignore - aka we should use it. Similarly, a 'true' would mean we
shouldn't use it.

Filip suggested document_available_triggers_snapshot. Is this ok with you?

[carlosk, 2017/04/21 01:10:58]

It is. I find this naming to be just as clear as the ignore*.

 
   // The expected quality of a snapshot taken at the moment this value is
   // queried.
   PageQuality current_page_quality_ = PageQuality::POOR;
 
   base::WeakPtrFactory<SnapshotController> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SnapshotController);
 };
 
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_CORE_SNAPSHOT_CONTROLLER_H_