From fd7edf2acda4e3a7bc921de6aca3fcbb3599349b Mon Sep 17 00:00:00 2001 From: Henrique Nakashima Date: Wed, 29 Nov 2017 22:03:49 +0000 Subject: [PATCH] Reland "Count annotation subtypes that appear in PDF documents." Original: https://chromium-review.googlesource.com/c/chromium/src/+/740141 Revert: https://chromium-review.googlesource.com/c/chromium/src/+/788030 This reverts commit 112d7b025fc05fe5a7f6e00c2f0a6717dbe91976. Bug: 768986,788176 TBR: isherman@chromium.org Change-Id: Ie5c8889f73443b9eefccd803f15cd0c0b1cd1ce9 Reviewed-on: https://chromium-review.googlesource.com/788330 Reviewed-by: dsinclair Commit-Queue: Henrique Nakashima Cr-Commit-Position: refs/heads/master@{#520262} --- pdf/BUILD.gn | 1 + pdf/out_of_process_instance.cc | 35 +++++++++++++++++++++++-- pdf/out_of_process_instance.h | 8 ++++++ pdf/pdf_engine.cc | 20 ++++++++++++++ pdf/pdf_engine.h | 22 ++++++++++++++++ pdf/pdfium/pdfium_engine.cc | 18 ++++++++++--- pdf/pdfium/pdfium_page.cc | 22 ++++++++++++++++ pdf/pdfium/pdfium_page.h | 4 +++ pdf/preview_mode_client.cc | 3 +++ pdf/preview_mode_client.h | 2 ++ tools/metrics/histograms/enums.xml | 31 ++++++++++++++++++++++ tools/metrics/histograms/histograms.xml | 8 ++++++ 12 files changed, 168 insertions(+), 6 deletions(-) create mode 100644 pdf/pdf_engine.cc diff --git a/pdf/BUILD.gn b/pdf/BUILD.gn index 28949f8ed38f5f..34c33a4c378ec2 100644 --- a/pdf/BUILD.gn +++ b/pdf/BUILD.gn @@ -42,6 +42,7 @@ if (enable_pdf) { "paint_manager.h", "pdf.cc", "pdf.h", + "pdf_engine.cc", "pdf_engine.h", "preview_mode_client.cc", "preview_mode_client.h", diff --git a/pdf/out_of_process_instance.cc b/pdf/out_of_process_instance.cc index f257c848c27c27..e71dd14bb3c851 100644 --- a/pdf/out_of_process_instance.cc +++ b/pdf/out_of_process_instance.cc @@ -176,6 +176,10 @@ enum PDFFeatures { FEATURES_COUNT }; +// Used for UMA. Do not delete entries, and keep in sync with histograms.xml +// and pdfium/public/fpdf_annot.h. +const int kAnnotationTypesCount = 28; + PP_Var GetLinkAtPosition(PP_Instance instance, PP_Point point) { pp::Var var; void* object = pp::Instance::GetPerInstanceObject(instance, kPPPPdfInterface); @@ -1174,8 +1178,11 @@ void OutOfProcessInstance::DocumentSizeUpdated(const pp::Size& size) { dimensions.Set(kJSDocumentWidth, pp::Var(document_size_.width())); dimensions.Set(kJSDocumentHeight, pp::Var(document_size_.height())); pp::VarArray page_dimensions_array; - int num_pages = engine_->GetNumberOfPages(); - for (int i = 0; i < num_pages; ++i) { + size_t num_pages = engine_->GetNumberOfPages(); + if (page_is_processed_.size() < num_pages) + page_is_processed_.resize(num_pages); + + for (size_t i = 0; i < num_pages; ++i) { pp::Rect page_rect = engine_->GetPageRect(i); pp::VarDictionary page_dimensions; page_dimensions.Set(kJSPageX, pp::Var(page_rect.x())); @@ -1298,6 +1305,30 @@ void OutOfProcessInstance::NotifySelectedFindResultChanged( SelectedFindResultChanged(current_find_index); } +void OutOfProcessInstance::NotifyPageBecameVisible( + const PDFEngine::PageFeatures* page_features) { + if (!page_features || !page_features->IsInitialized() || + page_features->index >= static_cast(page_is_processed_.size()) || + page_is_processed_[page_features->index]) { + return; + } + + for (const int annotation_type : page_features->annotation_types) { + DCHECK_GE(annotation_type, 0); + DCHECK_LT(annotation_type, kAnnotationTypesCount); + if (annotation_type < 0 || annotation_type >= kAnnotationTypesCount) + continue; + + if (annotation_types_counted_.find(annotation_type) == + annotation_types_counted_.end()) { + HistogramEnumeration("PDF.AnnotationType", annotation_type, + kAnnotationTypesCount); + annotation_types_counted_.insert(annotation_type); + } + } + page_is_processed_[page_features->index] = true; +} + void OutOfProcessInstance::GetDocumentPassword( pp::CompletionCallbackWithOutput callback) { if (password_callback_) { diff --git a/pdf/out_of_process_instance.h b/pdf/out_of_process_instance.h index 83930513eb444b..99b58b2a46ba35 100644 --- a/pdf/out_of_process_instance.h +++ b/pdf/out_of_process_instance.h @@ -108,6 +108,8 @@ class OutOfProcessInstance : public pp::Instance, void UpdateTickMarks(const std::vector& tickmarks) override; void NotifyNumberOfFindResultsChanged(int total, bool final_result) override; void NotifySelectedFindResultChanged(int current_find_index) override; + void NotifyPageBecameVisible( + const PDFEngine::PageFeatures* page_features) override; void GetDocumentPassword( pp::CompletionCallbackWithOutput callback) override; void Alert(const std::string& message) override; @@ -410,6 +412,12 @@ class OutOfProcessInstance : public pp::Instance, // toolbar. int top_toolbar_height_in_viewport_coords_; + // Whether each page had its features processed. + std::vector page_is_processed_; + + // Annotation types that were already counted for this document. + std::set annotation_types_counted_; + // The current state of accessibility: either off, enabled but waiting // for the document to load, or fully loaded. enum AccessibilityState { diff --git a/pdf/pdf_engine.cc b/pdf/pdf_engine.cc new file mode 100644 index 00000000000000..05c51319cb2d9a --- /dev/null +++ b/pdf/pdf_engine.cc @@ -0,0 +1,20 @@ +// Copyright 2017 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 "pdf/pdf_engine.h" + +namespace chrome_pdf { + +PDFEngine::PageFeatures::PageFeatures(){}; + +PDFEngine::PageFeatures::PageFeatures(const PageFeatures& other) + : index(other.index), annotation_types(other.annotation_types) {} + +PDFEngine::PageFeatures::~PageFeatures(){}; + +bool PDFEngine::PageFeatures::IsInitialized() const { + return index >= 0; +} + +} // namespace chrome_pdf diff --git a/pdf/pdf_engine.h b/pdf/pdf_engine.h index 60f14d47d7fb46..19246183a4acc6 100644 --- a/pdf/pdf_engine.h +++ b/pdf/pdf_engine.h @@ -14,6 +14,7 @@ #endif #include +#include #include #include @@ -68,6 +69,7 @@ class PDFEngine { kCount = 4, }; + // Features in a document that are relevant to measure. struct DocumentFeatures { // Number of pages in document. size_t page_count = 0; @@ -83,6 +85,22 @@ class PDFEngine { FormType form_type = FormType::kNone; }; + // Features in a page that are relevant to measure. + struct PageFeatures { + PageFeatures(); + PageFeatures(const PageFeatures& other); + ~PageFeatures(); + + // Whether the instance has been initialized and filled. + bool IsInitialized() const; + + // 0-based page index in the document. < 0 when uninitialized. + int index = -1; + + // Set of annotation types found in page. + std::set annotation_types; + }; + // The interface that's provided to the rendering engine. class Client { public: @@ -129,6 +147,10 @@ class PDFEngine { // Updates the index of the currently selected search item. virtual void NotifySelectedFindResultChanged(int current_find_index) = 0; + // Notifies a page became visible. + virtual void NotifyPageBecameVisible( + const PDFEngine::PageFeatures* page_features) = 0; + // Prompts the user for a password to open this document. The callback is // called when the password is retrieved. virtual void GetDocumentPassword( diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc index 8befffba0ad882..84513505aebaa4 100644 --- a/pdf/pdfium/pdfium_engine.cc +++ b/pdf/pdfium/pdfium_engine.cc @@ -1228,8 +1228,11 @@ void PDFiumEngine::OnPendingRequestComplete() { for (int pending_page : pending_pages_) { if (CheckPageAvailable(pending_page, &still_pending)) { update_pages = true; - if (IsPageVisible(pending_page)) + if (IsPageVisible(pending_page)) { + client_->NotifyPageBecameVisible( + pages_[pending_page]->GetPageFeatures()); client_->Invalidate(GetPageScreenRect(pending_page)); + } } } pending_pages_.swap(still_pending); @@ -3106,16 +3109,23 @@ void PDFiumEngine::CalculateVisiblePages() { pending_pages_.clear(); doc_loader_->ClearPendingRequests(); - visible_pages_.clear(); + std::vector formerly_visible_pages; + std::swap(visible_pages_, formerly_visible_pages); + pp::Rect visible_rect(plugin_size_); - for (size_t i = 0; i < pages_.size(); ++i) { + for (int i = 0; i < static_cast(pages_.size()); ++i) { // Check an entire PageScreenRect, since we might need to repaint side // borders and shadows even if the page itself is not visible. // For example, when user use pdf with different page sizes and zoomed in // outside page area. if (visible_rect.Intersects(GetPageScreenRect(i))) { visible_pages_.push_back(i); - CheckPageAvailable(i, &pending_pages_); + if (CheckPageAvailable(i, &pending_pages_)) { + auto it = std::find(formerly_visible_pages.begin(), + formerly_visible_pages.end(), i); + if (it == formerly_visible_pages.end()) + client_->NotifyPageBecameVisible(pages_[i]->GetPageFeatures()); + } } else { // Need to unload pages when we're not using them, since some PDFs use a // lot of memory. See http://crbug.com/48791 diff --git a/pdf/pdfium/pdfium_page.cc b/pdf/pdfium/pdfium_page.cc index cd7cdd7c9d8d5c..c3fffb7fe552b0 100644 --- a/pdf/pdfium/pdfium_page.cc +++ b/pdf/pdfium/pdfium_page.cc @@ -19,6 +19,7 @@ #include "pdf/pdfium/pdfium_api_string_buffer_adapter.h" #include "pdf/pdfium/pdfium_engine.h" #include "printing/units.h" +#include "third_party/pdfium/public/fpdf_annot.h" using printing::ConvertUnitDouble; using printing::kPointsPerInch; @@ -572,6 +573,27 @@ pp::Rect PDFiumPage::PageToScreen(const pp::Point& offset, new_size_y.ValueOrDie()); } +const PDFEngine::PageFeatures* PDFiumPage::GetPageFeatures() { + // If page_features_ is cached, return the cached features. + if (page_features_.IsInitialized()) + return &page_features_; + + FPDF_PAGE page = GetPage(); + if (!page) + return nullptr; + + // Initialize and cache page_features_. + page_features_.index = index_; + int annotation_count = FPDFPage_GetAnnotCount(page); + for (int i = 0; i < annotation_count; ++i) { + FPDF_ANNOTATION annotation = FPDFPage_GetAnnot(page, i); + FPDF_ANNOTATION_SUBTYPE subtype = FPDFAnnot_GetSubtype(annotation); + page_features_.annotation_types.insert(subtype); + } + + return &page_features_; +} + PDFiumPage::ScopedLoadCounter::ScopedLoadCounter(PDFiumPage* page) : page_(page) { page_->loading_count_++; diff --git a/pdf/pdfium/pdfium_page.h b/pdf/pdfium/pdfium_page.h index 7cc60e71fdd505..2cb0e58c38ca55 100644 --- a/pdf/pdfium/pdfium_page.h +++ b/pdf/pdfium/pdfium_page.h @@ -10,6 +10,7 @@ #include "base/optional.h" #include "base/strings/string16.h" +#include "pdf/pdf_engine.h" #include "ppapi/cpp/rect.h" #include "third_party/pdfium/public/fpdf_doc.h" #include "third_party/pdfium/public/fpdf_formfill.h" @@ -107,6 +108,8 @@ class PDFiumPage { double bottom, int rotation) const; + const PDFEngine::PageFeatures* GetPageFeatures(); + int index() const { return index_; } pp::Rect rect() const { return rect_; } void set_rect(const pp::Rect& r) { rect_ = r; } @@ -164,6 +167,7 @@ class PDFiumPage { bool calculated_links_; std::vector links_; bool available_; + PDFEngine::PageFeatures page_features_; }; } // namespace chrome_pdf diff --git a/pdf/preview_mode_client.cc b/pdf/preview_mode_client.cc index 4f6d3c318b7d1f..c824fdafe58c40 100644 --- a/pdf/preview_mode_client.cc +++ b/pdf/preview_mode_client.cc @@ -59,6 +59,9 @@ void PreviewModeClient::NotifySelectedFindResultChanged( NOTREACHED(); } +void PreviewModeClient::NotifyPageBecameVisible( + const PDFEngine::PageFeatures* page_features) {} + void PreviewModeClient::GetDocumentPassword( pp::CompletionCallbackWithOutput callback) { callback.Run(PP_ERROR_FAILED); diff --git a/pdf/preview_mode_client.h b/pdf/preview_mode_client.h index e39f62660ba783..807cf800b405ad 100644 --- a/pdf/preview_mode_client.h +++ b/pdf/preview_mode_client.h @@ -39,6 +39,8 @@ class PreviewModeClient : public PDFEngine::Client { void UpdateTickMarks(const std::vector& tickmarks) override; void NotifyNumberOfFindResultsChanged(int total, bool final_result) override; void NotifySelectedFindResultChanged(int current_find_index) override; + void NotifyPageBecameVisible( + const PDFEngine::PageFeatures* page_features) override; void GetDocumentPassword( pp::CompletionCallbackWithOutput callback) override; void Alert(const std::string& message) override; diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index ee30314df81db8..72e18048e0227e 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -4776,6 +4776,37 @@ uploading your change for review. These are checked by presubmit scripts. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 21a4a217a3ddfa..9d7a910a2ff57f 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -59768,6 +59768,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + hnakashima@chromium.org + + Tracks documents opened in the PDF viewer that displayed one or more pages + with the given annotation type. + + + tsergeant@chromium.org