diff options
6 files changed, 154 insertions, 140 deletions
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 4ad5f69..ea54253 100644 --- a/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc +++ b/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc @@ -32,8 +32,10 @@ #include "content/public/browser/web_contents.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/test/browser_test_utils.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" namespace dom_distiller { @@ -200,6 +202,29 @@ void DomDistillerViewerSourceBrowserTest::ViewSingleDistilledPage( EXPECT_EQ(expected_mime_type, contents_after_nav->GetContentsMimeType()); } +IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, + TestBadUrlErrorPage) { + GURL url("chrome-distiller://bad"); + + // Navigate to a distiller URL. + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + // Wait for the page load to complete as the first page completes the root + // document. + content::WaitForLoadStop(contents); + + ASSERT_TRUE(contents != NULL); + EXPECT_EQ(url, contents->GetLastCommittedURL()); + + std::string result; + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + contents, kGetContent , &result)); + EXPECT_THAT(result, HasSubstr(l10n_util::GetStringUTF8( + IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT))); +} + // The DomDistillerViewerSource renders untrusted content, so ensure no bindings // are enabled when the CSS resource is loaded. This CSS might be bundle with // Chrome or provided by an extension. 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 dadefe7..2c84e6b 100644 --- a/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc +++ b/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc @@ -413,109 +413,39 @@ IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, MarkupInfo) { EXPECT_EQ(600, markup_image2.height()); } -IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, - TestTitleAndContentAreNeverEmpty) { +IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, TestTitleNeverEmpty) { const std::string some_title = "some title"; - const std::string some_content = "some content"; const std::string no_title = l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE); - const std::string no_content = - l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_CONTENT); - - { // Test non-empty title and content for article. - scoped_ptr<DistilledArticleProto> article_proto( - new DistilledArticleProto()); - article_proto->set_title(some_title); - (*(article_proto->add_pages())).set_html(some_content); - std::string html = viewer::GetUnsafeArticleHtml(article_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(some_title)); - EXPECT_THAT(html, HasSubstr(some_content)); - EXPECT_THAT(html, Not(HasSubstr(no_title))); - EXPECT_THAT(html, Not(HasSubstr(no_content))); - } - { // Test empty title and content for article. + { // 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::GetUnsafeArticleHtml(article_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, HasSubstr(no_content)); - EXPECT_THAT(html, Not(HasSubstr(some_title))); - EXPECT_THAT(html, Not(HasSubstr(some_content))); - } - - { // Test missing title and non-empty content for article. - scoped_ptr<DistilledArticleProto> article_proto( - new DistilledArticleProto()); - (*(article_proto->add_pages())).set_html(some_content); - std::string html = viewer::GetUnsafeArticleHtml(article_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); + std::string html = viewer::GetUnsafeArticleTemplateHtml( + &article_proto.get()->pages(0), DistilledPagePrefs::LIGHT, + DistilledPagePrefs::SERIF); EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, HasSubstr(no_content)); EXPECT_THAT(html, Not(HasSubstr(some_title))); - EXPECT_THAT(html, Not(HasSubstr(some_content))); } - { // Test non-empty title and missing content for article. - scoped_ptr<DistilledArticleProto> article_proto( - new DistilledArticleProto()); - article_proto->set_title(some_title); - std::string html = viewer::GetUnsafeArticleHtml(article_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, HasSubstr(no_content)); - EXPECT_THAT(html, Not(HasSubstr(some_title))); - EXPECT_THAT(html, Not(HasSubstr(some_content))); - } - - { // Test non-empty title and content for page. - scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); - page_proto->set_title(some_title); - page_proto->set_html(some_content); - std::string html = viewer::GetUnsafePartialArticleHtml(page_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(some_title)); - EXPECT_THAT(html, HasSubstr(some_content)); - EXPECT_THAT(html, Not(HasSubstr(no_title))); - EXPECT_THAT(html, Not(HasSubstr(no_content))); - } - - { // Test empty title and content for page. + { // Test empty title for page. scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); page_proto->set_title(""); page_proto->set_html(""); - std::string html = viewer::GetUnsafePartialArticleHtml(page_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); + std::string html = viewer::GetUnsafeArticleTemplateHtml( + page_proto.get(), DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, HasSubstr(no_content)); EXPECT_THAT(html, Not(HasSubstr(some_title))); - EXPECT_THAT(html, Not(HasSubstr(some_content))); } - { // Test missing title and non-empty content for page. + { // Test missing title for page. scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); - page_proto->set_html(some_content); - std::string html = viewer::GetUnsafePartialArticleHtml(page_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); + std::string html = viewer::GetUnsafeArticleTemplateHtml( + page_proto.get(), DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); EXPECT_THAT(html, HasSubstr(no_title)); - EXPECT_THAT(html, HasSubstr(some_content)); EXPECT_THAT(html, Not(HasSubstr(some_title))); - EXPECT_THAT(html, Not(HasSubstr(no_content))); - } - - { // Test non-empty title and missing content for page. - scoped_ptr<DistilledPageProto> page_proto(new DistilledPageProto()); - page_proto->set_title(some_title); - std::string html = viewer::GetUnsafePartialArticleHtml(page_proto.get(), - DistilledPagePrefs::LIGHT, DistilledPagePrefs::SERIF); - EXPECT_THAT(html, HasSubstr(some_title)); - EXPECT_THAT(html, HasSubstr(no_content)); - EXPECT_THAT(html, Not(HasSubstr(no_title))); - EXPECT_THAT(html, Not(HasSubstr(some_content))); } } diff --git a/components/dom_distiller/content/dom_distiller_viewer_source.cc b/components/dom_distiller/content/dom_distiller_viewer_source.cc index a3ae496..57a72e4 100644 --- a/components/dom_distiller/content/dom_distiller_viewer_source.cc +++ b/components/dom_distiller/content/dom_distiller_viewer_source.cc @@ -46,6 +46,9 @@ class DomDistillerViewerSource::RequestViewerHandle DistilledPagePrefs* distilled_page_prefs); ~RequestViewerHandle() override; + // Flag this request as an error and send the error page template. + void flagAsErrorPage(); + // ViewRequestDelegate implementation: void OnArticleReady(const DistilledArticleProto* article_proto) override; @@ -104,6 +107,9 @@ class DomDistillerViewerSource::RequestViewerHandle // Temporary store of pending JavaScript if the page isn't ready to receive // data from distillation. std::string buffer_; + + // Flag to tell this observer that the web contents are in an error state. + bool is_error_page_; }; DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle( @@ -117,7 +123,8 @@ DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle( callback_(callback), page_count_(0), distilled_page_prefs_(distilled_page_prefs), - waiting_for_page_ready_(true) { + waiting_for_page_ready_(true), + is_error_page_(false) { content::WebContentsObserver::Observe(web_contents); distilled_page_prefs_->AddObserver(this); } @@ -126,6 +133,14 @@ DomDistillerViewerSource::RequestViewerHandle::~RequestViewerHandle() { distilled_page_prefs_->RemoveObserver(this); } +void DomDistillerViewerSource::RequestViewerHandle::flagAsErrorPage() { + is_error_page_ = true; + std::string error_page_html = viewer::GetErrorPageHtml( + distilled_page_prefs_->GetTheme(), + distilled_page_prefs_->GetFontFamily()); + callback_.Run(base::RefCountedString::TakeString(&error_page_html)); +} + void DomDistillerViewerSource::RequestViewerHandle::SendJavaScript( const std::string& buffer) { if (waiting_for_page_ready_) { @@ -150,7 +165,6 @@ void DomDistillerViewerSource::RequestViewerHandle::DidNavigateMainFrame( } Cancel(); - } void DomDistillerViewerSource::RequestViewerHandle::RenderProcessGone( @@ -174,6 +188,13 @@ void DomDistillerViewerSource::RequestViewerHandle::Cancel() { void DomDistillerViewerSource::RequestViewerHandle::DidFinishLoad( content::RenderFrameHost* render_frame_host, const GURL& validated_url) { + if (is_error_page_) { + waiting_for_page_ready_ = false; + SendJavaScript(viewer::GetErrorPageJs()); + Cancel(); // This will cause the object to clean itself up. + return; + } + if (render_frame_host->GetParent()) { return; } @@ -187,14 +208,16 @@ void DomDistillerViewerSource::RequestViewerHandle::DidFinishLoad( void DomDistillerViewerSource::RequestViewerHandle::OnArticleReady( const DistilledArticleProto* article_proto) { + // TODO(mdjones): Move this logic to super class so it can be used in both + // android and IOS. http://crbug.com/472797 if (page_count_ == 0) { - // This is a single-page article. - std::string unsafe_page_html = - viewer::GetUnsafeArticleHtml( - article_proto, - distilled_page_prefs_->GetTheme(), - distilled_page_prefs_->GetFontFamily()); + std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml( + &article_proto->pages(0), + distilled_page_prefs_->GetTheme(), + distilled_page_prefs_->GetFontFamily()); callback_.Run(base::RefCountedString::TakeString(&unsafe_page_html)); + // Send first page to client. + SendJavaScript(viewer::GetUnsafeArticleContentJs(article_proto)); } else if (page_count_ == article_proto->pages_size()) { // We may still be showing the "Loading" indicator. SendJavaScript(viewer::GetToggleLoadingIndicatorJs(true)); @@ -221,15 +244,14 @@ void DomDistillerViewerSource::RequestViewerHandle::OnArticleUpdated( article_update.GetDistilledPage(page_count_); if (page_count_ == 0) { // This is the first page, so send Viewer page scaffolding too. - std::string unsafe_page_html = viewer::GetUnsafePartialArticleHtml( + std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml( &page, distilled_page_prefs_->GetTheme(), distilled_page_prefs_->GetFontFamily()); callback_.Run(base::RefCountedString::TakeString(&unsafe_page_html)); - } else { - SendJavaScript( - viewer::GetUnsafeIncrementalDistilledPageJs(&page, false)); } + // Send the page content to the client. + SendJavaScript(viewer::GetUnsafeIncrementalDistilledPageJs(&page, false)); } } @@ -310,14 +332,7 @@ void DomDistillerViewerSource::StartDataRequest( // after receiving the callback. request_viewer_handle->TakeViewerHandle(viewer_handle.Pass()); } else { - // The service did not return a |ViewerHandle|, which means the - // |RequestViewerHandle| will never be called, so clean up now. - delete request_viewer_handle; - - std::string error_page_html = viewer::GetErrorPageHtml( - dom_distiller_service_->GetDistilledPagePrefs()->GetTheme(), - dom_distiller_service_->GetDistilledPagePrefs()->GetFontFamily()); - callback.Run(base::RefCountedString::TakeString(&error_page_html)); + request_viewer_handle->flagAsErrorPage(); } }; diff --git a/components/dom_distiller/core/html/dom_distiller_viewer.html b/components/dom_distiller/core/html/dom_distiller_viewer.html index 334cd30..1a1c6fb 100644 --- a/components/dom_distiller/core/html/dom_distiller_viewer.html +++ b/components/dom_distiller/core/html/dom_distiller_viewer.html @@ -12,16 +12,16 @@ found in the LICENSE file. $2 <link href='https://fonts.googleapis.com/css?family=Open+Sans' rel='stylesheet' type='text/css'> </head> -<body dir="$9" class="$4"> +<body dir="$8" class="$4"> <div id="mainContent"> <article> <header> <h1>$1</h1> </header> - <div id="content">$5</div> + <div id="content">$9</div> </article> </div> - <div id="loadingIndicator" class="$6"> + <div id="loadingIndicator" class="$5"> <div id="loader"> <div class="circle initial"> <span class="mask"> @@ -75,7 +75,7 @@ found in the LICENSE file. </div> </div> <div id="showOriginal"> - <a href="$7">$8</a> + <a href="$6">$7</a> </div> </body> $3 diff --git a/components/dom_distiller/core/viewer.cc b/components/dom_distiller/core/viewer.cc index 6ae11a9..6ae039e 100644 --- a/components/dom_distiller/core/viewer.cc +++ b/components/dom_distiller/core/viewer.cc @@ -90,9 +90,12 @@ const std::string GetFontCssClass(DistilledPagePrefs::FontFamily font_family) { return kSansSerifCssClass; } -void EnsureNonEmptyTitleAndContent(std::string* title, std::string* content) { +void EnsureNonEmptyTitle(std::string* title) { if (title->empty()) *title = l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_NO_DATA_TITLE); +} + +void EnsureNonEmptyContent(std::string* content) { UMA_HISTOGRAM_BOOLEAN("DomDistiller.PageHasDistilledData", !content->empty()); if (content->empty()) { *content = l10n_util::GetStringUTF8( @@ -103,11 +106,11 @@ void EnsureNonEmptyTitleAndContent(std::string* title, std::string* content) { std::string ReplaceHtmlTemplateValues( const std::string& title, const std::string& textDirection, - const std::string& content, const std::string& loading_indicator_class, const std::string& original_url, const DistilledPagePrefs::Theme theme, - const DistilledPagePrefs::FontFamily font_family) { + const DistilledPagePrefs::FontFamily font_family, + const std::string& htmlContent) { base::StringPiece html_template = ResourceBundle::GetSharedInstance().GetRawDataResource( IDR_DOM_DISTILLER_VIEWER_HTML); @@ -130,12 +133,12 @@ std::string ReplaceHtmlTemplateValues( substitutions.push_back(GetThemeCssClass(theme) + " " + GetFontCssClass(font_family)); // $4 - substitutions.push_back(content); // $5 - substitutions.push_back(loading_indicator_class); // $6 - substitutions.push_back(original_url); // $7 + substitutions.push_back(loading_indicator_class); // $5 + substitutions.push_back(original_url); // $6 substitutions.push_back( - l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_VIEW_ORIGINAL)); // $8 - substitutions.push_back(textDirection); // $9 + l10n_util::GetStringUTF8(IDS_DOM_DISTILLER_VIEWER_VIEW_ORIGINAL)); // $7 + substitutions.push_back(textDirection); // $8 + substitutions.push_back(htmlContent); // $9 return ReplaceStringPlaceholders(html_template, substitutions, NULL); } @@ -149,6 +152,7 @@ const std::string GetUnsafeIncrementalDistilledPageJs( std::string output; base::StringValue value(page_proto->html()); base::JSONWriter::Write(&value, &output); + EnsureNonEmptyContent(&output); std::string page_update("addToPage("); page_update += output + ");"; return page_update + GetToggleLoadingIndicatorJs( @@ -156,6 +160,16 @@ const std::string GetUnsafeIncrementalDistilledPageJs( } +const std::string GetErrorPageJs() { + base::StringValue value(l10n_util::GetStringUTF8( + IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT)); + std::string output; + base::JSONWriter::Write(&value, &output); + std::string page_update("addToPage("); + page_update += output + ");"; + return page_update; +} + const std::string GetToggleLoadingIndicatorJs(const bool is_last_page) { if (is_last_page) return "showLoadingIndicator(true);"; @@ -163,20 +177,51 @@ const std::string GetToggleLoadingIndicatorJs(const bool is_last_page) { return "showLoadingIndicator(false);"; } -const std::string GetUnsafePartialArticleHtml( +const std::string GetUnsafeArticleTemplateHtml( const DistilledPageProto* page_proto, const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family) { DCHECK(page_proto); + std::string title = net::EscapeForHTML(page_proto->title()); - std::ostringstream unsafe_output_stream; - unsafe_output_stream << page_proto->html(); - std::string unsafe_article_html = unsafe_output_stream.str(); - EnsureNonEmptyTitleAndContent(&title, &unsafe_article_html); + + EnsureNonEmptyTitle(&title); + + std::string text_direction = page_proto->text_direction(); std::string original_url = page_proto->url(); - return ReplaceHtmlTemplateValues( - title, page_proto->text_direction(), unsafe_article_html, "visible", - original_url, theme, font_family); + + return ReplaceHtmlTemplateValues(title, text_direction, "hidden", + original_url, theme, font_family, ""); +} + +const std::string GetUnsafeArticleContentJs( + const DistilledArticleProto* article_proto) { + DCHECK(article_proto); + if (article_proto->pages_size() == 0 || !article_proto->pages(0).has_html()) { + return ""; + } + + std::ostringstream unsafe_output_stream; + for (int page_num = 0; page_num < article_proto->pages_size(); ++page_num) { + unsafe_output_stream << article_proto->pages(page_num).html(); + } + + std::string output; + base::StringValue value(unsafe_output_stream.str()); + base::JSONWriter::Write(&value, &output); + EnsureNonEmptyContent(&output); + std::string page_update("addToPage("); + page_update += output + ");"; + 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 GetUnsafeArticleHtml( @@ -198,7 +243,8 @@ const std::string GetUnsafeArticleHtml( text_direction = article_proto->pages(0).text_direction(); } - EnsureNonEmptyTitleAndContent(&title, &unsafe_article_html); + EnsureNonEmptyTitle(&title); + EnsureNonEmptyContent(&unsafe_article_html); std::string original_url; if (article_proto->pages_size() > 0 && article_proto->pages(0).has_url()) { @@ -206,19 +252,8 @@ const std::string GetUnsafeArticleHtml( } return ReplaceHtmlTemplateValues( - title, text_direction, unsafe_article_html, "hidden", original_url, - theme, font_family); -} - -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); - std::string content = l10n_util::GetStringUTF8( - IDS_DOM_DISTILLER_VIEWER_FAILED_TO_FIND_ARTICLE_CONTENT); - return ReplaceHtmlTemplateValues( - title, "", content, "hidden", "", theme, font_family); + title, text_direction, "hidden", original_url, theme, font_family, + unsafe_article_html); } const std::string GetCss() { diff --git a/components/dom_distiller/core/viewer.h b/components/dom_distiller/core/viewer.h index 9e1ded3..0f50fbb 100644 --- a/components/dom_distiller/core/viewer.h +++ b/components/dom_distiller/core/viewer.h @@ -32,17 +32,22 @@ const std::string GetUnsafeArticleHtml( const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family); -// Returns the base Viewer HTML page based on the given |page_proto|. This is +// Returns an HTML template page based on the given |page_proto| which provides +// basic information about the page (i.e. title, text direction, etc.). This is // supposed to be displayed to the end user. The returned HTML should be // considered unsafe, so callers must ensure rendering it does not compromise -// Chrome. The difference from |GetUnsafeArticleHtml| is that this can be used -// for displaying an in-flight distillation instead of waiting for the full -// article. -const std::string GetUnsafePartialArticleHtml( +// Chrome. +const std::string GetUnsafeArticleTemplateHtml( const DistilledPageProto* page_proto, const DistilledPagePrefs::Theme theme, const DistilledPagePrefs::FontFamily font_family); +// Returns the JavaScript to place a full article's HTML on the page. The +// returned HTML should be considered unsafe, so callers must ensure +// rendering it does not compromise Chrome. +const std::string GetUnsafeArticleContentJs( + const DistilledArticleProto* article_proto); + // Returns a JavaScript blob for updating a partial view request with additional // distilled content. Meant for use when viewing a slow or long multi-page // article. |is_last_page| indicates whether this is the last page of the @@ -51,6 +56,10 @@ const std::string GetUnsafeIncrementalDistilledPageJs( const DistilledPageProto* page_proto, const bool is_last_page); +// Returns a JavaScript blob for updating a view request with error page +// contents. +const std::string GetErrorPageJs(); + // Returns a JavaScript blob for controlling the "in-progress" indicator when // viewing a partially-distilled page. |is_last_page| indicates whether this is // the last page of the article (i.e. loading indicator should be removed). |