diff options
19 files changed, 502 insertions, 208 deletions
diff --git a/build/ios/grit_whitelist.txt b/build/ios/grit_whitelist.txt index 3b9f3ee..f155898 100644 --- a/build/ios/grit_whitelist.txt +++ b/build/ios/grit_whitelist.txt @@ -328,6 +328,7 @@ IDS_DOM_DISTILLER_QUALITY_QUESTION IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_TITLE IDS_DOM_DISTILLER_VIEWER_LOADING_STRING +IDS_DOM_DISTILLER_VIEWER_LOADING_TITLE IDS_DOM_DISTILLER_VIEWER_NO_DATA_CONTENT IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE IDS_DOM_DISTILLER_VIEWER_VIEW_ORIGINAL diff --git a/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc b/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc index ea54253..c2782ee 100644 --- a/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc +++ b/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc @@ -257,6 +257,64 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, } IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, + EarlyTemplateLoad) { + dom_distiller::DomDistillerServiceFactory::GetInstance() + ->SetTestingFactoryAndUse(browser()->profile(), &Build); + + scoped_refptr<content::MessageLoopRunner> distillation_done_runner = + new content::MessageLoopRunner; + + FakeDistiller* distiller = new FakeDistiller( + false, + distillation_done_runner->QuitClosure()); + EXPECT_CALL(*distiller_factory_, CreateDistillerImpl()) + .WillOnce(testing::Return(distiller)); + + // Navigate to a URL. + GURL url(dom_distiller::url_utils::GetDistillerViewUrlFromUrl( + kDomDistillerScheme, GURL("http://urlthatlooksvalid.com"))); + chrome::NavigateParams params(browser(), url, ui::PAGE_TRANSITION_TYPED); + chrome::Navigate(¶ms); + distillation_done_runner->Run(); + + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Wait for the page load to complete (should only be template). + content::WaitForLoadStop(contents); + std::string result; + // Loading spinner should be on screen at this point. + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + contents, kGetLoadIndicatorClassName , &result)); + EXPECT_EQ("visible", result); + + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + contents, kGetContent , &result)); + EXPECT_THAT(result, Not(HasSubstr("content"))); + + // Finish distillation and make sure the spinner has been replaced by text. + std::vector<scoped_refptr<ArticleDistillationUpdate::RefCountedPageProto> > + update_pages; + scoped_ptr<DistilledArticleProto> article(new DistilledArticleProto()); + + scoped_refptr<base::RefCountedData<DistilledPageProto> > page_proto = + new base::RefCountedData<DistilledPageProto>(); + page_proto->data.set_url("http://foo.html"); + page_proto->data.set_html("<div>content</div>"); + update_pages.push_back(page_proto); + *(article->add_pages()) = page_proto->data; + + ArticleDistillationUpdate update(update_pages, true, false); + distiller->RunDistillerUpdateCallback(update); + + content::WaitForLoadStop(contents); + + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + contents, kGetContent , &result)); + EXPECT_THAT(result, HasSubstr("content")); +} + +IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, MultiPageArticle) { expect_distillation_ = false; expect_distiller_page_ = true; diff --git a/chrome/browser/dom_distiller/tab_utils_browsertest.cc b/chrome/browser/dom_distiller/tab_utils_browsertest.cc index 124aaaf..608c4c1 100644 --- a/chrome/browser/dom_distiller/tab_utils_browsertest.cc +++ b/chrome/browser/dom_distiller/tab_utils_browsertest.cc @@ -22,6 +22,7 @@ #include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_utils.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "testing/gtest/include/gtest/gtest.h" @@ -38,11 +39,17 @@ class DomDistillerTabUtilsBrowserTest : public InProcessBrowserTest { } }; +// WebContentsMainFrameHelper is used to detect if a distilled page has +// finished loading. This is done by checking how many times the title has +// been set rather than using "DidFinishLoad" directly due to the content +// being set by JavaScript. class WebContentsMainFrameHelper : public content::WebContentsObserver { public: WebContentsMainFrameHelper(content::WebContents* web_contents, const base::Closure& callback) - : callback_(callback) { + : callback_(callback), + title_set_count_(0), + loaded_distiller_page_(false) { content::WebContentsObserver::Observe(web_contents); } @@ -50,11 +57,24 @@ class WebContentsMainFrameHelper : public content::WebContentsObserver { const GURL& validated_url) override { if (!render_frame_host->GetParent() && validated_url.scheme() == kDomDistillerScheme) + loaded_distiller_page_ = true; + } + + void TitleWasSet(content::NavigationEntry* entry, + bool explicit_set) override { + // The title will be set twice on distilled pages; once for the placeholder + // and once when the distillation has finished. Watch for the second time + // as a signal that the JavaScript that sets the content has run. + title_set_count_++; + if (title_set_count_ >= 2 && loaded_distiller_page_) { callback_.Run(); + } } private: base::Closure callback_; + int title_set_count_; + bool loaded_distiller_page_; }; #if (defined(OS_LINUX) && defined(OS_CHROMEOS)) @@ -86,12 +106,15 @@ IN_PROC_BROWSER_TEST_F(DomDistillerTabUtilsBrowserTest, new_url_loaded_runner.QuitClosure())); new_url_loaded_runner.Run(); + std::string page_title; + content::ExecuteScriptAndGetValue(after_web_contents->GetMainFrame(), + "document.title")->GetAsString(&page_title); + // Verify the new URL is showing distilled content in a new WebContents. EXPECT_NE(initial_web_contents, after_web_contents); EXPECT_TRUE( after_web_contents->GetLastCommittedURL().SchemeIs(kDomDistillerScheme)); - EXPECT_EQ("Test Page Title", - base::UTF16ToUTF8(after_web_contents->GetTitle())); + EXPECT_EQ("Test Page Title", page_title); } IN_PROC_BROWSER_TEST_F(DomDistillerTabUtilsBrowserTest, @@ -128,14 +151,17 @@ IN_PROC_BROWSER_TEST_F(DomDistillerTabUtilsBrowserTest, // Verify that the source WebContents is showing the original article. EXPECT_EQ(article_url, source_web_contents->GetLastCommittedURL()); - EXPECT_EQ("Test Page Title", - base::UTF16ToUTF8(source_web_contents->GetTitle())); + std::string page_title; + content::ExecuteScriptAndGetValue(source_web_contents->GetMainFrame(), + "document.title")->GetAsString(&page_title); + EXPECT_EQ("Test Page Title", page_title); // Verify the destination WebContents is showing distilled content. EXPECT_TRUE(destination_web_contents->GetLastCommittedURL().SchemeIs( kDomDistillerScheme)); - EXPECT_EQ("Test Page Title", - base::UTF16ToUTF8(destination_web_contents->GetTitle())); + content::ExecuteScriptAndGetValue(destination_web_contents->GetMainFrame(), + "document.title")->GetAsString(&page_title); + EXPECT_EQ("Test Page Title", page_title); content::WebContentsDestroyedWatcher destroyed_watcher( destination_web_contents); diff --git a/components/components_tests.gyp b/components/components_tests.gyp index 799a988..d8e492d 100644 --- a/components/components_tests.gyp +++ b/components/components_tests.gyp @@ -150,6 +150,7 @@ 'dom_distiller/core/distiller_url_fetcher_unittest.cc', 'dom_distiller/core/dom_distiller_model_unittest.cc', 'dom_distiller/core/dom_distiller_service_unittest.cc', + 'dom_distiller/core/dom_distiller_request_view_base_unittest.cc', 'dom_distiller/core/dom_distiller_store_unittest.cc', 'dom_distiller/core/page_features_unittest.cc', 'dom_distiller/core/task_tracker_unittest.cc', diff --git a/components/dom_distiller.gypi b/components/dom_distiller.gypi index c2bb83d..cc9da80 100644 --- a/components/dom_distiller.gypi +++ b/components/dom_distiller.gypi @@ -141,6 +141,7 @@ 'dom_distiller/core/fake_distiller.h', 'dom_distiller/core/fake_distiller_page.cc', 'dom_distiller/core/fake_distiller_page.h', + 'dom_distiller/core/test_request_view_handle.h', ], }, { diff --git a/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc b/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc index 5244a19..aa01a86 100644 --- a/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc +++ b/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc @@ -417,42 +417,6 @@ IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, MarkupInfo) { EXPECT_EQ(600, markup_image2.height()); } -IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, TestTitleNeverEmpty) { - const std::string some_title = "some title"; - const std::string no_title = - l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE); - - { // Test empty title for article. - scoped_ptr<DistilledArticleProto> article_proto( - new DistilledArticleProto()); - article_proto->set_title(""); - (*(article_proto->add_pages())).set_html(""); - std::string html = viewer::GetUnsafeArticleTemplateHtml( - &article_proto.get()->pages(0), DistilledPagePrefs::LIGHT, - DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, Not(HasSubstr(some_title))); - } - - { // Test empty title for page. - scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); - page_proto->set_title(""); - page_proto->set_html(""); - std::string html = viewer::GetUnsafeArticleTemplateHtml( - page_proto.get(), DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, Not(HasSubstr(some_title))); - } - - { // Test missing title for page. - scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); - std::string html = viewer::GetUnsafeArticleTemplateHtml( - page_proto.get(), DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, Not(HasSubstr(some_title))); - } -} - IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, TestNoContentDoesNotCrash) { const std::string no_content = diff --git a/components/dom_distiller/content/dom_distiller_viewer_source.cc b/components/dom_distiller/content/dom_distiller_viewer_source.cc index f1ed1a8..b6b611c 100644 --- a/components/dom_distiller/content/dom_distiller_viewer_source.cc +++ b/components/dom_distiller/content/dom_distiller_viewer_source.cc @@ -20,6 +20,7 @@ #include "components/dom_distiller/core/feedback_reporter.h" #include "components/dom_distiller/core/task_tracker.h" #include "components/dom_distiller/core/url_constants.h" +#include "components/dom_distiller/core/url_utils.h" #include "components/dom_distiller/core/viewer.h" #include "content/public/browser/navigation_details.h" #include "content/public/browser/navigation_entry.h" @@ -28,35 +29,13 @@ #include "content/public/browser/user_metrics.h" #include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents_observer.h" +#include "grit/components_strings.h" #include "net/base/url_util.h" #include "net/url_request/url_request.h" +#include "ui/base/l10n/l10n_util.h" namespace dom_distiller { -namespace { - -class ContentDataCallback : public DistillerDataCallback { - public: - ContentDataCallback(const content::URLDataSource::GotDataCallback& callback); - // Runs the callback. - void RunCallback(std::string& data) override; - - private: - // The callback that actually gets run. - content::URLDataSource::GotDataCallback callback_; -}; - -ContentDataCallback::ContentDataCallback( - const content::URLDataSource::GotDataCallback& callback) { - callback_ = callback; -} - -void ContentDataCallback::RunCallback(std::string& data) { - callback_.Run(base::RefCountedString::TakeString(&data)); -} - -} // namespace - // Handles receiving data asynchronously for a specific entry, and passing // it along to the data callback for the data source. Lifetime matches that of // the current main frame's page in the Viewer instance. @@ -67,7 +46,6 @@ class DomDistillerViewerSource::RequestViewerHandle RequestViewerHandle(content::WebContents* web_contents, const std::string& expected_scheme, const std::string& expected_request_path, - scoped_ptr<ContentDataCallback> callback, DistilledPagePrefs* distilled_page_prefs); ~RequestViewerHandle() override; @@ -109,9 +87,8 @@ DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle( content::WebContents* web_contents, const std::string& expected_scheme, const std::string& expected_request_path, - scoped_ptr<ContentDataCallback> callback, DistilledPagePrefs* distilled_page_prefs) - : DomDistillerRequestViewBase(callback.Pass(), distilled_page_prefs), + : DomDistillerRequestViewBase(distilled_page_prefs), expected_scheme_(expected_scheme), expected_request_path_(expected_request_path), waiting_for_page_ready_(true) { @@ -173,7 +150,12 @@ void DomDistillerViewerSource::RequestViewerHandle::DidFinishLoad( if (IsErrorPage()) { waiting_for_page_ready_ = false; SendJavaScript(viewer::GetErrorPageJs()); + std::string title(l10n_util::GetStringUTF8( + IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT)); + SendJavaScript(viewer::GetSetTitleJs(title)); + SendJavaScript(viewer::GetSetTextDirectionJs(std::string("auto"))); SendJavaScript(viewer::GetShowFeedbackFormJs()); + Cancel(); // This will cause the object to clean itself up. return; } @@ -250,17 +232,21 @@ void DomDistillerViewerSource::StartDataRequest( DCHECK(web_contents); // An empty |path| is invalid, but guard against it. If not empty, assume // |path| starts with '?', which is stripped away. - scoped_ptr<ContentDataCallback> data_callback( - new ContentDataCallback(callback)); const std::string path_after_query_separator = path.size() > 0 ? path.substr(1) : ""; - RequestViewerHandle* request_viewer_handle = new RequestViewerHandle( - web_contents, scheme_, path_after_query_separator, data_callback.Pass(), - dom_distiller_service_->GetDistilledPagePrefs()); + RequestViewerHandle* request_viewer_handle = + new RequestViewerHandle(web_contents, scheme_, path_after_query_separator, + dom_distiller_service_->GetDistilledPagePrefs()); scoped_ptr<ViewerHandle> viewer_handle = viewer::CreateViewRequest( dom_distiller_service_, path, request_viewer_handle, web_contents->GetContainerBounds().size()); + GURL current_url = web_contents->GetLastCommittedURL(); + std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml( + url_utils::GetOriginalUrlFromDistillerUrl(current_url).spec(), + dom_distiller_service_->GetDistilledPagePrefs()->GetTheme(), + dom_distiller_service_->GetDistilledPagePrefs()->GetFontFamily()); + if (viewer_handle) { // The service returned a |ViewerHandle| and guarantees it will call // the |RequestViewerHandle|, so passing ownership to it, to ensure the @@ -270,6 +256,9 @@ void DomDistillerViewerSource::StartDataRequest( } else { request_viewer_handle->FlagAsErrorPage(); } + + // Place template on the page. + callback.Run(base::RefCountedString::TakeString(&unsafe_page_html)); }; std::string DomDistillerViewerSource::GetMimeType( diff --git a/components/dom_distiller/core/BUILD.gn b/components/dom_distiller/core/BUILD.gn index 565f1fe..a14ad21 100644 --- a/components/dom_distiller/core/BUILD.gn +++ b/components/dom_distiller/core/BUILD.gn @@ -98,6 +98,7 @@ source_set("test_support") { "fake_distiller.h", "fake_distiller_page.cc", "fake_distiller_page.h", + "test_request_view_handle.h", ] deps = [ @@ -120,6 +121,7 @@ source_set("unit_tests") { "distiller_unittest.cc", "distiller_url_fetcher_unittest.cc", "dom_distiller_model_unittest.cc", + "dom_distiller_request_view_base_unittest.cc", "dom_distiller_service_unittest.cc", "dom_distiller_store_unittest.cc", "page_features_unittest.cc", @@ -134,6 +136,8 @@ source_set("unit_tests") { "//base", "//components/leveldb_proto:test_support", "//components/pref_registry:test_support", + "//components/resources", + "//components/strings", "//net:test_support", "//sync", "//testing/gmock", diff --git a/components/dom_distiller/core/dom_distiller_request_view_base.cc b/components/dom_distiller/core/dom_distiller_request_view_base.cc index 8a6a30f..8a5266f 100644 --- a/components/dom_distiller/core/dom_distiller_request_view_base.cc +++ b/components/dom_distiller/core/dom_distiller_request_view_base.cc @@ -25,10 +25,8 @@ namespace dom_distiller { DomDistillerRequestViewBase::DomDistillerRequestViewBase( - scoped_ptr<DistillerDataCallback> callback, DistilledPagePrefs* distilled_page_prefs) - : callback_(callback.Pass()), - page_count_(0), + : page_count_(0), distilled_page_prefs_(distilled_page_prefs), is_error_page_(false) { } @@ -38,10 +36,6 @@ DomDistillerRequestViewBase::~DomDistillerRequestViewBase() { void DomDistillerRequestViewBase::FlagAsErrorPage() { is_error_page_ = true; - std::string error_page_html = - viewer::GetErrorPageHtml(distilled_page_prefs_->GetTheme(), - distilled_page_prefs_->GetFontFamily()); - callback_->RunCallback(error_page_html); } bool DomDistillerRequestViewBase::IsErrorPage() { @@ -51,17 +45,15 @@ bool DomDistillerRequestViewBase::IsErrorPage() { void DomDistillerRequestViewBase::OnArticleReady( const DistilledArticleProto* article_proto) { if (page_count_ == 0) { - const DistilledPageProto* cur_page; - if (article_proto->pages().size() < 1) { - cur_page = new DistilledPageProto(); + std::string text_direction; + if (article_proto->pages().size() > 0) { + text_direction = article_proto->pages(0).text_direction(); } else { - cur_page = &article_proto->pages(0); + text_direction = "auto"; } - std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml( - cur_page, distilled_page_prefs_->GetTheme(), - distilled_page_prefs_->GetFontFamily()); - callback_->RunCallback(unsafe_page_html); - // Send first page to client. + // Send first page, title, and text direction to client. + SendJavaScript(viewer::GetSetTitleJs(article_proto->title())); + SendJavaScript(viewer::GetSetTextDirectionJs(text_direction)); SendJavaScript(viewer::GetUnsafeArticleContentJs(article_proto)); // If any content was loaded, show the feedback form. SendJavaScript(viewer::GetShowFeedbackFormJs()); @@ -92,11 +84,10 @@ void DomDistillerRequestViewBase::OnArticleUpdated( SendJavaScript(viewer::GetUnsafeIncrementalDistilledPageJs(&page, false)); if (page_count_ == 0) { - // This is the first page, so send Viewer page scaffolding too. - std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml( - &page, distilled_page_prefs_->GetTheme(), - distilled_page_prefs_->GetFontFamily()); - callback_->RunCallback(unsafe_page_html); + // This is the first page, so send the title and text direction to the + // client. + SendJavaScript(viewer::GetSetTitleJs(page.title())); + SendJavaScript(viewer::GetSetTextDirectionJs(page.text_direction())); // If any content was loaded, show the feedback form. SendJavaScript(viewer::GetShowFeedbackFormJs()); } @@ -116,6 +107,9 @@ void DomDistillerRequestViewBase::OnChangeFontFamily( void DomDistillerRequestViewBase::TakeViewerHandle( scoped_ptr<ViewerHandle> viewer_handle) { viewer_handle_ = viewer_handle.Pass(); + // Getting the viewer handle means this is not an error page, show the + // loading indicator. + SendJavaScript(viewer::GetToggleLoadingIndicatorJs(false)); } } // namespace dom_distiller diff --git a/components/dom_distiller/core/dom_distiller_request_view_base.h b/components/dom_distiller/core/dom_distiller_request_view_base.h index 16b8ca7..ef0fb9f 100644 --- a/components/dom_distiller/core/dom_distiller_request_view_base.h +++ b/components/dom_distiller/core/dom_distiller_request_view_base.h @@ -21,24 +21,13 @@ namespace dom_distiller { -// This interface is used to abstract the data callback from the distiller. The -// callbacks for different platforms have different numbers of parameters -// (namely iOS and Android) which makes this necessary. -class DistillerDataCallback { - public: - virtual ~DistillerDataCallback(){}; - virtual void RunCallback(std::string& data) = 0; -}; - // Handles receiving data asynchronously for a specific entry, and passing // it along to the data callback for the data source. Lifetime matches that of // the current main frame's page in the Viewer instance. -class DomDistillerRequestViewBase - : public ViewRequestDelegate, - public DistilledPagePrefs::Observer { +class DomDistillerRequestViewBase : public ViewRequestDelegate, + public DistilledPagePrefs::Observer { public: explicit DomDistillerRequestViewBase( - scoped_ptr<DistillerDataCallback> callback, DistilledPagePrefs* distilled_page_prefs); ~DomDistillerRequestViewBase() override; @@ -68,9 +57,6 @@ class DomDistillerRequestViewBase // needs to be kept around to ensure the distillation request finishes. scoped_ptr<ViewerHandle> viewer_handle_; - // Holds the callback to where the data retrieved is sent back. - scoped_ptr<DistillerDataCallback> callback_; - // Number of pages of the distilled article content that have been rendered by // the viewer. int page_count_; diff --git a/components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc b/components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc new file mode 100644 index 0000000..352d38c --- /dev/null +++ b/components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc @@ -0,0 +1,239 @@ +// Copyright 2015 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 <vector> + +#include "components/dom_distiller/core/article_distillation_update.h" +#include "components/dom_distiller/core/test_request_view_handle.h" +#include "components/pref_registry/testing_pref_service_syncable.h" +#include "grit/components_resources.h" +#include "grit/components_strings.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "ui/base/l10n/l10n_util.h" + +using testing::HasSubstr; +using testing::Not; + +namespace dom_distiller { + +class DomDistillerRequestViewTest : public testing::Test { + protected: + void SetUp() override { + pref_service_.reset(new user_prefs::TestingPrefServiceSyncable()); + DistilledPagePrefs::RegisterProfilePrefs(pref_service_->registry()); + distilled_page_prefs_.reset(new DistilledPagePrefs(pref_service_.get())); + } + + scoped_ptr<user_prefs::TestingPrefServiceSyncable> pref_service_; + scoped_ptr<DistilledPagePrefs> distilled_page_prefs_; +}; + +TEST_F(DomDistillerRequestViewTest, TestTitleEscaped) { + const std::string no_title = + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE); + const std::string valid_title = "valid title"; + const std::string has_quotes = "\"" + valid_title + "\""; + const std::string escaped_quotes = "\\\"" + valid_title + "\\\""; + const std::string has_special_chars = "<" + valid_title + "\\"; + const std::string escaped_special_chars = "\\u003C" + valid_title + "\\\\"; + + TestRequestViewHandle handle(distilled_page_prefs_.get()); + + // Make sure title is properly escaped from quotes. + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + article_proto->set_title(has_quotes); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(escaped_quotes)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(has_quotes))); + handle.ClearJavaScriptBuffer(); + } + + // Make sure title is properly escaped from special characters. + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + article_proto->set_title(has_special_chars); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(escaped_special_chars)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), + Not(HasSubstr(has_special_chars))); + handle.ClearJavaScriptBuffer(); + } + +} + +TEST_F(DomDistillerRequestViewTest, TestTitleNeverEmpty) { + const std::string no_title = + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE); + const std::string valid_title = "valid title"; + + TestRequestViewHandle handle(distilled_page_prefs_.get()); + + // Test that the title actually gets shown. + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + article_proto->set_title(valid_title); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(valid_title)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(no_title))); + handle.ClearJavaScriptBuffer(); + } + + // Test empty string title + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + article_proto->set_title(""); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_title)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_title))); + handle.ClearJavaScriptBuffer(); + } + + // Test no title. + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_title)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_title))); + handle.ClearJavaScriptBuffer(); + } +} + +TEST_F(DomDistillerRequestViewTest, TestContentNeverEmpty) { + const std::string no_content = + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_CONTENT); + const std::string valid_content = "valid content"; + + TestRequestViewHandle handle(distilled_page_prefs_.get()); + + // Test single page content + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + (*(article_proto->add_pages())).set_html(valid_content); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(valid_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(no_content))); + handle.ClearJavaScriptBuffer(); + } + + // Test multiple page content + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + (*(article_proto->add_pages())).set_html(valid_content); + (*(article_proto->add_pages())).set_html(valid_content); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), + HasSubstr(valid_content + valid_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(no_content))); + handle.ClearJavaScriptBuffer(); + } + + // Test empty string content + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + (*(article_proto->add_pages())).set_html(""); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_content))); + handle.ClearJavaScriptBuffer(); + } + + // Test page no content + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + article_proto->add_pages(); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_content))); + handle.ClearJavaScriptBuffer(); + } + + // Test no page. + { + scoped_ptr<DistilledArticleProto> article_proto( + new DistilledArticleProto()); + + handle.OnArticleReady(article_proto.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_content))); + handle.ClearJavaScriptBuffer(); + } + + // Test empty string page update + { + std::vector<scoped_refptr<ArticleDistillationUpdate::RefCountedPageProto>> + pages; + scoped_refptr<base::RefCountedData<DistilledPageProto>> page_proto = + new base::RefCountedData<DistilledPageProto>(); + page_proto->data.set_html(""); + pages.push_back(page_proto); + + scoped_ptr<ArticleDistillationUpdate> article_update( + new ArticleDistillationUpdate(pages, false, false)); + + handle.OnArticleUpdated(*article_update.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(no_content)); + EXPECT_THAT(handle.GetJavaScriptBuffer(), Not(HasSubstr(valid_content))); + handle.ClearJavaScriptBuffer(); + } +} + +TEST_F(DomDistillerRequestViewTest, TestLoadingIndicator) { + const std::string no_content = + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_CONTENT); + // Showing the indicator does mean passing 'false' as the parameter. + const std::string show_loader = "showLoadingIndicator(false);"; + + TestRequestViewHandle handle(distilled_page_prefs_.get()); + handle.TakeViewerHandle(NULL); + + // The loading indicator should show before any content is displayed. + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(show_loader)); + handle.ClearJavaScriptBuffer(); + + std::vector<scoped_refptr<ArticleDistillationUpdate::RefCountedPageProto>> + pages; + scoped_refptr<base::RefCountedData<DistilledPageProto>> page_proto = + new base::RefCountedData<DistilledPageProto>(); + pages.push_back(page_proto); + + scoped_ptr<ArticleDistillationUpdate> article_update( + new ArticleDistillationUpdate(pages, true, false)); + + handle.OnArticleUpdated(*article_update.get()); + + EXPECT_THAT(handle.GetJavaScriptBuffer(), HasSubstr(show_loader)); +} + +} // namespace dom_distiller diff --git a/components/dom_distiller/core/html/dom_distiller_viewer.html b/components/dom_distiller/core/html/dom_distiller_viewer.html index 2aef588..b4c02aa 100644 --- a/components/dom_distiller/core/html/dom_distiller_viewer.html +++ b/components/dom_distiller/core/html/dom_distiller_viewer.html @@ -9,10 +9,11 @@ found in the LICENSE file. <meta charset="utf-8"> <meta name="viewport" content="width=device-width,initial-scale=1,maximum-scale=1,user-scalable=no"> <title>$1</title> + <!-- Placeholder for CSS. --> $2 <link href='https://fonts.googleapis.com/css?family=Open+Sans' rel='stylesheet' type='text/css'> </head> -<body dir="$8" class="$4"> +<body class="$3"> <div id="contentWrap"> <div id="mainContent"> <article> @@ -20,7 +21,7 @@ found in the LICENSE file. <div id="titleCollapse"> <div class="verticalCenterOuter"> <div class="verticalCenterInner"> - <h1 id="titleHolder">$1</h1> + <h1 id="titleHolder"></h1> </div> </div> </div> @@ -28,7 +29,7 @@ found in the LICENSE file. <div id="content"></div> </article> </div> - <div id="loadingIndicator" class="$5"> + <div id="loadingIndicator" class="visible"> <div id="loader"> <div class="circle initial"> <span class="mask"> @@ -82,7 +83,7 @@ found in the LICENSE file. </div> </div> <div id="showOriginal"> - <a href="$6">$7</a> + <a href="$4">$5</a> </div> </div> <div id="feedbackContainer" class="footerFeedback"> @@ -96,5 +97,6 @@ found in the LICENSE file. <div class="clear"></div> </div> </body> -$3 +<!-- Placeholder for JavaScript. --> +$6 </html> diff --git a/components/dom_distiller/core/javascript/dom_distiller_viewer.js b/components/dom_distiller/core/javascript/dom_distiller_viewer.js index 4a80bba..75292a2 100644 --- a/components/dom_distiller/core/javascript/dom_distiller_viewer.js +++ b/components/dom_distiller/core/javascript/dom_distiller_viewer.js @@ -49,12 +49,18 @@ function setTitle(title) { collapse.style.height = "0px"; holder.textContent = title; + document.title = title; var newHeight = Math.max(90, holder.getBoundingClientRect().height); collapse.style.transition = "height 0.2s"; collapse.style.height = newHeight + "px"; } +// Set the text direction of the document ('ltr', 'rtl', or 'auto'). +function setTextDirection(direction) { + document.body.setAttribute('dir', direction); +} + // Maps JS Font Family to CSS class and then changes body class name. // CSS classes must agree with distilledpage.css. function useFontFamily(fontFamily) { diff --git a/components/dom_distiller/core/test_request_view_handle.h b/components/dom_distiller/core/test_request_view_handle.h new file mode 100644 index 0000000..5271e59 --- /dev/null +++ b/components/dom_distiller/core/test_request_view_handle.h @@ -0,0 +1,52 @@ +// Copyright 2015 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_DOM_DISTILLER_CORE_TEST_REQUEST_VIEW_HANDLE_H_ +#define COMPONENTS_DOM_DISTILLER_CORE_TEST_REQUEST_VIEW_HANDLE_H_ + +#include <string> + +#include "components/dom_distiller/core/distilled_page_prefs.h" +#include "components/dom_distiller/core/dom_distiller_request_view_base.h" + +namespace dom_distiller { + +// TestRequestViewHandle allows the javascript buffer to be collected at any +// point and viewed. This class is for testing only. +class TestRequestViewHandle : public DomDistillerRequestViewBase { + public: + TestRequestViewHandle(DistilledPagePrefs* prefs); + ~TestRequestViewHandle() override; + + std::string GetJavaScriptBuffer(); + void ClearJavaScriptBuffer(); + + private: + void SendJavaScript(const std::string& buffer) override; + std::string buffer_; +}; + +TestRequestViewHandle::TestRequestViewHandle(DistilledPagePrefs* prefs) + : DomDistillerRequestViewBase(prefs) { +} + +TestRequestViewHandle::~TestRequestViewHandle() { + distilled_page_prefs_->RemoveObserver(this); +} + +std::string TestRequestViewHandle::GetJavaScriptBuffer() { + return buffer_; +} + +void TestRequestViewHandle::ClearJavaScriptBuffer() { + buffer_ = ""; +} + +void TestRequestViewHandle::SendJavaScript(const std::string& buffer) { + buffer_ += buffer; +} + +} // namespace dom_distiller + +#endif // COMPONENTS_DOM_DISTILLER_CORE_TEST_REQUEST_VIEW_HANDLE_H_ diff --git a/components/dom_distiller/core/viewer.cc b/components/dom_distiller/core/viewer.cc index 0244a4f..bfb10f0 100644 --- a/components/dom_distiller/core/viewer.cc +++ b/components/dom_distiller/core/viewer.cc @@ -104,19 +104,13 @@ void EnsureNonEmptyContent(std::string* content) { } std::string ReplaceHtmlTemplateValues( - const std::string& title, - const std::string& textDirection, - const std::string& loading_indicator_class, const std::string& original_url, const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family) { base::StringPiece html_template = ResourceBundle::GetSharedInstance().GetRawDataResource( IDR_DOM_DISTILLER_VIEWER_HTML); - // TODO(mdjones): Many or all of these substitutions can be placed on the - // page via JavaScript. std::vector<std::string> substitutions; - substitutions.push_back(title); // $1 std::ostringstream css; std::ostringstream script; @@ -129,16 +123,19 @@ std::string ReplaceHtmlTemplateValues( css << "<link rel=\"stylesheet\" href=\"/" << kViewerCssPath << "\">"; script << "<script src=\"" << kViewerJsPath << "\"></script>"; #endif // defined(OS_IOS) + + substitutions.push_back( + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_LOADING_TITLE)); // $1 substitutions.push_back(css.str()); // $2 - substitutions.push_back(script.str()); // $3 substitutions.push_back(GetThemeCssClass(theme) + " " + - GetFontCssClass(font_family)); // $4 - substitutions.push_back(loading_indicator_class); // $5 - substitutions.push_back(original_url); // $6 + GetFontCssClass(font_family)); // $3 + + substitutions.push_back(original_url); // $4 substitutions.push_back( - l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_VIEW_ORIGINAL)); // $7 - substitutions.push_back(textDirection); // $8 + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_VIEW_ORIGINAL)); // $5 + + substitutions.push_back(script.str()); // $6 return ReplaceStringPlaceholders(html_template, substitutions, NULL); } @@ -190,6 +187,21 @@ const std::string GetErrorPageJs() { return page_update; } +const std::string GetSetTitleJs(std::string title) { + EnsureNonEmptyTitle(&title); + base::StringValue value(title); + std::string output; + base::JSONWriter::Write(value, &output); + return "setTitle(" + output + ");"; +} + +const std::string GetSetTextDirectionJs(const std::string& direction) { + base::StringValue value(direction); + std::string output; + base::JSONWriter::Write(value, &output); + return "setTextDirection(" + output + ");"; +} + const std::string GetToggleLoadingIndicatorJs(const bool is_last_page) { if (is_last_page) return "showLoadingIndicator(true);"; @@ -198,20 +210,10 @@ const std::string GetToggleLoadingIndicatorJs(const bool is_last_page) { } const std::string GetUnsafeArticleTemplateHtml( - const DistilledPageProto* page_proto, + const std::string original_url, const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family) { - DCHECK(page_proto); - - std::string title = net::EscapeForHTML(page_proto->title()); - - EnsureNonEmptyTitle(&title); - - std::string text_direction = page_proto->text_direction(); - std::string original_url = page_proto->url(); - - return ReplaceHtmlTemplateValues(title, text_direction, "hidden", - original_url, theme, font_family); + return ReplaceHtmlTemplateValues(original_url, theme, font_family); } const std::string GetUnsafeArticleContentJs( @@ -232,15 +234,6 @@ const std::string GetUnsafeArticleContentJs( return page_update + GetToggleLoadingIndicatorJs(true); } -const std::string GetErrorPageHtml( - const DistilledPagePrefs::Theme theme, - const DistilledPagePrefs::FontFamily font_family) { - std::string title = l10n_util::GetStringUTF8( - IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_TITLE); - return ReplaceHtmlTemplateValues(title, "auto", "hidden", "", theme, - font_family); -} - const std::string GetCss() { return ResourceBundle::GetSharedInstance().GetRawDataResource( IDR_DISTILLER_CSS).as_string(); diff --git a/components/dom_distiller/core/viewer.h b/components/dom_distiller/core/viewer.h index 7b9261f..61b0e40 100644 --- a/components/dom_distiller/core/viewer.h +++ b/components/dom_distiller/core/viewer.h @@ -33,7 +33,7 @@ const std::string GetShowFeedbackFormJs(); // considered unsafe, so callers must ensure rendering it does not compromise // Chrome. const std::string GetUnsafeArticleTemplateHtml( - const DistilledPageProto* page_proto, + const std::string original_url, const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family); @@ -51,6 +51,12 @@ const std::string GetUnsafeIncrementalDistilledPageJs( const DistilledPageProto* page_proto, const bool is_last_page); +// Returns the JavaScript to set the title of the distilled article page. +const std::string GetSetTitleJs(std::string title); + +// Return the JavaScript to set the text direction of the distiller page. +const std::string GetSetTextDirectionJs(const std::string& direction); + // Returns a JavaScript blob for updating a view request with error page // contents. const std::string GetErrorPageJs(); @@ -60,11 +66,6 @@ const std::string GetErrorPageJs(); // the last page of the article (i.e. loading indicator should be removed). const std::string GetToggleLoadingIndicatorJs(const bool is_last_page); -// Returns a full HTML page which displays a generic error. -const std::string GetErrorPageHtml( - const DistilledPagePrefs::Theme theme, - const DistilledPagePrefs::FontFamily font_family); - // Returns the default CSS to be used for a viewer. const std::string GetCss(); diff --git a/components/dom_distiller_strings.grdp b/components/dom_distiller_strings.grdp index c05e9b6..79a4fe5 100644 --- a/components/dom_distiller_strings.grdp +++ b/components/dom_distiller_strings.grdp @@ -28,6 +28,9 @@ <message name="IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT" desc="The text to show in place of reading list article content if the article is not found."> Could not find the requested article. </message> + <message name="IDS_DOM_DISTILLER_VIEWER_LOADING_TITLE" desc="The text to show in place of a reading list article title while the article is loading."> + Reader Mode + </message> <message name="IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE" desc="The text to show in place of a reading list article title if there is no data found."> Failed to display article. </message> diff --git a/ios/chrome/browser/dom_distiller/distiller_viewer.cc b/ios/chrome/browser/dom_distiller/distiller_viewer.cc index d078046..1e1aab9 100644 --- a/ios/chrome/browser/dom_distiller/distiller_viewer.cc +++ b/ios/chrome/browser/dom_distiller/distiller_viewer.cc @@ -18,72 +18,45 @@ namespace dom_distiller { -namespace { - -class IOSContentDataCallback : public DistillerDataCallback { - public: - IOSContentDataCallback( - const GURL& url, - const DistillerViewer::DistillationFinishedCallback& callback, - DistillerViewer* distiller_viewer_handle); - ~IOSContentDataCallback() override{}; - void RunCallback(std::string& data) override; - - private: - // Extra param needed by the callback specified below. - GURL url_; - // The callback to be run. - const DistillerViewer::DistillationFinishedCallback callback_; - // A handle to the DistillerViewer object. - DistillerViewer* distiller_viewer_handle_; -}; - -IOSContentDataCallback::IOSContentDataCallback( - const GURL& url, - const DistillerViewer::DistillationFinishedCallback& callback, - DistillerViewer* distiller_viewer_handle) - : url_(url), - callback_(callback), - distiller_viewer_handle_(distiller_viewer_handle) { -} - -void IOSContentDataCallback::RunCallback(std::string& data) { - std::string htmlAndScript(data); - htmlAndScript += "<script>" + - distiller_viewer_handle_->GetJavaScriptBuffer() + - "</script>"; - - callback_.Run(url_, htmlAndScript); -} - -} // namespace - DistillerViewer::DistillerViewer(ios::ChromeBrowserState* browser_state, const GURL& url, const DistillationFinishedCallback& callback) : DomDistillerRequestViewBase( - scoped_ptr<DistillerDataCallback>( - new IOSContentDataCallback(url, callback, this)).Pass(), - new DistilledPagePrefs(browser_state->GetPrefs())) { + new DistilledPagePrefs(browser_state->GetPrefs())), + url_(url), + callback_(callback) { DCHECK(browser_state); DCHECK(url.is_valid()); dom_distiller::DomDistillerService* distillerService = dom_distiller::DomDistillerServiceFactory::GetForBrowserState( browser_state); - viewer_handle_ = distillerService->ViewUrl( + scoped_ptr<ViewerHandle> viewer_handle = distillerService->ViewUrl( this, distillerService->CreateDefaultDistillerPage(gfx::Size()), url); + + TakeViewerHandle(viewer_handle.Pass()); } DistillerViewer::~DistillerViewer() { } -void DistillerViewer::SendJavaScript(const std::string& buffer) { - js_buffer_ += buffer; +void DistillerViewer::OnArticleReady( + const dom_distiller::DistilledArticleProto* article_proto) { + DomDistillerRequestViewBase::OnArticleReady(article_proto); + + const std::string html = viewer::GetUnsafeArticleTemplateHtml( + url_.spec(), distilled_page_prefs_->GetTheme(), + distilled_page_prefs_->GetFontFamily()); + + std::string html_and_script(html); + std::string hide_feedback = + "document.getElementById('feedbackContainer').style.display = 'none';"; + html_and_script += "<script>" + js_buffer_ + hide_feedback + "</script>"; + callback_.Run(url_, html_and_script); } -std::string DistillerViewer::GetJavaScriptBuffer() { - return js_buffer_; +void DistillerViewer::SendJavaScript(const std::string& buffer) { + js_buffer_ += buffer; } } // namespace dom_distiller diff --git a/ios/chrome/browser/dom_distiller/distiller_viewer.h b/ios/chrome/browser/dom_distiller/distiller_viewer.h index 629e745..1cb4f84 100644 --- a/ios/chrome/browser/dom_distiller/distiller_viewer.h +++ b/ios/chrome/browser/dom_distiller/distiller_viewer.h @@ -34,17 +34,18 @@ class DistillerViewer : public DomDistillerRequestViewBase { const DistillationFinishedCallback& callback); ~DistillerViewer() override; - void SendJavaScript(const std::string& buffer) override; + void OnArticleReady( + const dom_distiller::DistilledArticleProto* article_proto) override; - std::string GetJavaScriptBuffer(); + void SendJavaScript(const std::string& buffer) override; private: // The url of the distilled page. const GURL url_; - // Interface for accessing preferences for distilled pages. - scoped_ptr<DistilledPagePrefs> distilled_page_prefs_; // JavaScript buffer. std::string js_buffer_; + // Callback to run once distillation is complete. + const DistillationFinishedCallback callback_; DISALLOW_COPY_AND_ASSIGN(DistillerViewer); }; |