Control PDF partial loading with a feature

Adds a "PdfPartialLoading" feature to control whether or not the PDF
viewer tries to use partial loading. The default state is enabled, as
this feature has shipped already, but we would like to experiment with
disabling it to address certain problems related to partial loading.

Bug: 1115149
Change-Id: Ie66fddf5b633269226bc01fca20ac7013f0c1e7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2349429
Commit-Queue: K. Moon <kmoon@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797378}

pdf/document_loader_impl.cc

@@ -13,9 +13,11 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/check_op.h"
+#include "base/feature_list.h"
 #include "base/notreached.h"
 #include "base/numerics/safe_math.h"
 #include "base/strings/string_util.h"
+#include "pdf/pdf_features.h"
 #include "pdf/url_loader_wrapper.h"
 #include "ppapi/c/pp_errors.h"
 #include "ui/gfx/range/range.h"
@@ -65,7 +67,10 @@ void DocumentLoaderImpl::Chunk::Clear() {
   chunk_data.reset();
 }
 
-DocumentLoaderImpl::DocumentLoaderImpl(Client* client) : client_(client) {}
+DocumentLoaderImpl::DocumentLoaderImpl(Client* client)
+    : client_(client),
+      partial_loading_enabled_(
+          base::FeatureList::IsEnabled(features::kPdfPartialLoading)) {}
 
 DocumentLoaderImpl::~DocumentLoaderImpl() = default;
 

pdf/document_loader_impl.h

@@ -79,7 +79,7 @@ class DocumentLoaderImpl : public DocumentLoader {
   std::unique_ptr<URLLoaderWrapper> loader_;
 
   DataStream chunk_stream_;
-  bool partial_loading_enabled_ = true;
+  bool partial_loading_enabled_;  // Default determined by `kPdfPartialLoading`.
   bool is_partial_loader_active_ = false;
 
   static constexpr uint32_t kReadBufferSize = 256 * 1024;

pdf/document_loader_impl_unittest.cc

@@ -12,6 +12,8 @@
 
 #include "base/callback.h"
 #include "base/check.h"
+#include "base/test/scoped_feature_list.h"
+#include "pdf/pdf_features.h"
 #include "pdf/ppapi_migration/callback.h"
 #include "pdf/url_loader_wrapper.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -265,9 +267,49 @@ class MockClient : public TestClient {
 
 }  // namespace
 
-using DocumentLoaderImplTest = ::testing::Test;
+class DocumentLoaderImplTest : public testing::Test {
+ protected:
+  DocumentLoaderImplTest() {
+    scoped_feature_list_.InitAndEnableFeature(features::kPdfPartialLoading);
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(DocumentLoaderImplTest, PartialLoadingFeatureDefault) {
+  scoped_feature_list_.Reset();
+  scoped_feature_list_.Init();
+
+  // Test that partial loading is enabled when feature is defaulted.
+  TestClient client;
+  client.SetCanUsePartialLoading();
+  DocumentLoaderImpl loader(&client);
+  loader.Init(client.CreateFullPageLoader(), "http://url.com");
+  loader.RequestData(1000000, 1);
+  EXPECT_FALSE(loader.is_partial_loader_active());
+  // Always send initial data from FullPageLoader.
+  client.full_page_loader_data()->CallReadCallback(kDefaultRequestSize);
+  EXPECT_TRUE(loader.is_partial_loader_active());
+}
+
+TEST_F(DocumentLoaderImplTest, PartialLoadingFeatureDisabled) {
+  scoped_feature_list_.Reset();
+  scoped_feature_list_.InitAndDisableFeature(features::kPdfPartialLoading);
+
+  // Test that partial loading is disabled when feature is disabled.
+  TestClient client;
+  client.SetCanUsePartialLoading();
+  DocumentLoaderImpl loader(&client);
+  loader.Init(client.CreateFullPageLoader(), "http://url.com");
+  loader.RequestData(1000000, 1);
+  EXPECT_FALSE(loader.is_partial_loader_active());
+  // Always send initial data from FullPageLoader.
+  client.full_page_loader_data()->CallReadCallback(kDefaultRequestSize);
+  EXPECT_FALSE(loader.is_partial_loader_active());
+}
 
 TEST_F(DocumentLoaderImplTest, PartialLoadingEnabled) {
+  // Test that partial loading is enabled. (Fixture enables PdfPartialLoading.)
   TestClient client;
   client.SetCanUsePartialLoading();
   DocumentLoaderImpl loader(&client);

pdf/pdf_features.cc

@@ -16,6 +16,10 @@ const base::Feature kAccessiblePDFHighlight = {
 const base::Feature kPdfHonorJsContentSettings = {
     "PdfHonorJsContentSettings", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// TODO(crbug.com/1064175): Remove this once partial loading is fixed.
+const base::Feature kPdfPartialLoading = {"PdfPartialLoading",
+                                          base::FEATURE_ENABLED_BY_DEFAULT};
+
 const base::Feature kPDFViewerUpdate = {"PDFViewerUpdate",
                                         base::FEATURE_DISABLED_BY_DEFAULT};
 

pdf/pdf_features.h

@@ -16,6 +16,7 @@ namespace features {
 extern const base::Feature kAccessiblePDFForm;
 extern const base::Feature kAccessiblePDFHighlight;
 extern const base::Feature kPdfHonorJsContentSettings;
+extern const base::Feature kPdfPartialLoading;
 extern const base::Feature kPDFViewerUpdate;
 extern const base::Feature kSaveEditedPDFForm;
 extern const base::Feature kTabAcrossPDFAnnotations;