IOS Reading distillation: Handle Google AMP iFrame.

ios/chrome/browser/reading_list/reading_list_distiller_page.mm

 // 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.
 
 #include "ios/chrome/browser/reading_list/reading_list_distiller_page.h"
 
 #include "base/bind.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/favicon/ios/web_favicon_driver.h"
+#include "components/google/core/browser/google_util.h"
 #include "ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h"
+#import "ios/web/public/navigation_item.h"
+#import "ios/web/public/navigation_manager.h"
+#include "ios/web/public/ssl_status.h"
+#import "ios/web/public/web_state/js/crw_js_injection_receiver.h"
 #import "ios/web/public/web_state/web_state.h"
+#import "net/base/mac/url_conversions.h"
+#include "net/cert/cert_status_flags.h"
+#include "url/url_constants.h"
 
 namespace {
 // The delay between the page load and the distillation in seconds.
 const int64_t kDistillationDelayInSeconds = 2;
+const char* kJavaScriptNavigateToIframe =

[jif, 2017/01/17 17:43:10]

s/kJavaScriptNavigateToIframe/kGetIframeURLJavaScript/

[Olivier, 2017/01/18 09:30:13]

mmm, I obviously did not upload my cleaning PS...
Done

+    "document.getElementsByTagName('iframe')[0].src;";
 }  // namespace
 
 namespace reading_list {
 
 ReadingListDistillerPage::ReadingListDistillerPage(
     web::BrowserState* browser_state,
     FaviconWebStateDispatcher* web_state_dispatcher)
     : dom_distiller::DistillerPageIOS(browser_state),
       web_state_dispatcher_(web_state_dispatcher),
       weak_ptr_factory_(this) {}
@@ skipping 22 matching lines @@
                                                   const base::Value* value) {
   std::unique_ptr<web::WebState> old_web_state = DetachWebState();
   if (old_web_state) {
     web_state_dispatcher_->ReturnWebState(std::move(old_web_state));
   }
   DistillerPageIOS::OnDistillationDone(page_url, value);
 }
 
 void ReadingListDistillerPage::OnLoadURLDone(
     web::PageLoadCompletionStatus load_completion_status) {
   if (load_completion_status == web::PageLoadCompletionStatus::FAILURE) {

[jif, 2017/01/17 17:43:10]

If somebody adds a 3rd status to PageLoadCompletionStatus, this code will break
silently.

Either compare load_completion_status to web::PageLoadCompletionStatus::SUCCESS
(good), or do a switch...case on |load_completion_status| (better).

[Olivier, 2017/01/18 09:30:13]

Any not success is a failure for this feature.
So compare to success.
Done.

     DistillerPageIOS::OnLoadURLDone(load_completion_status);
     return;
   }
+  if (HandleGoogleCachedAMP()) {

[jif, 2017/01/17 17:43:10]

How about this:

if (IsGoogleCachedAMP()) {
  // Workaround for Google AMP pages.
  HandleGoogleCachedAMP();
} else {
  WaitForPageLoadCompletion();
}

Also, consider renaming |IsGoogleCachedAMP| to |IsGoogleCachedAMPPage|.

[Olivier, 2017/01/18 09:30:13]

Done.

+    // The rest of the distillation will be handled by Google AMP workaround.
+    // Stop distillation here.
+    return;
+  }
+  WaitForPageLoadCompletion();
+}
+
+void ReadingListDistillerPage::WaitForPageLoadCompletion() {
   base::WeakPtr<ReadingListDistillerPage> weak_this =
       weak_ptr_factory_.GetWeakPtr();
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::Bind(&ReadingListDistillerPage::DelayedOnLoadURLDone,
-                            weak_this, load_completion_status),
+      FROM_HERE,
+      base::Bind(&ReadingListDistillerPage::DelayedOnLoadURLDone, weak_this),
       base::TimeDelta::FromSeconds(kDistillationDelayInSeconds));
 }
 
-void ReadingListDistillerPage::DelayedOnLoadURLDone(
-    web::PageLoadCompletionStatus load_completion_status) {
-  DistillerPageIOS::OnLoadURLDone(load_completion_status);
+void ReadingListDistillerPage::DelayedOnLoadURLDone() {
+  DistillerPageIOS::OnLoadURLDone(web::PageLoadCompletionStatus::SUCCESS);

[jif, 2017/01/17 17:43:10]

There's some funny business going on here.

[jif, 2017/01/17 17:43:45]

Ignore this comment, I forgot to remove it.

[Olivier, 2017/01/18 09:30:13]

Acknowledged.

 }
+
+bool ReadingListDistillerPage::IsGoogleCachedAMP() {
+  // All google AMP pages have URL in the form "https://google_domain/amp/..."
+  // and a valid certificate.
+  // This method checks that this is strictly the case.
+  GURL url = CurrentWebState()->GetLastCommittedURL();

[jif, 2017/01/17 17:43:10]

const GURL &

[Olivier, 2017/01/18 09:30:13]

Done.

+  if (!url.is_valid() || !url.SchemeIs(url::kHttpsScheme)) {
+    return false;
+  }
+  if (!google_util::IsGoogleDomainUrl(
+          url, google_util::DISALLOW_SUBDOMAIN,
+          google_util::DISALLOW_NON_STANDARD_PORTS) ||
+      !url.path().compare(0, 4, "amp/")) {
+    return false;
+  }
+  const web::SSLStatus& ssl_status = CurrentWebState()
+                                         ->GetNavigationManager()
+                                         ->GetLastCommittedItem()
+                                         ->GetSSL();
+  if (!ssl_status.certificate ||
+      (net::IsCertStatusError(ssl_status.cert_status) &&
+       !net::IsCertStatusMinorError(ssl_status.cert_status))) {
+    return false;
+  }
+
+  return true;
+}
+
+bool ReadingListDistillerPage::HandleGoogleCachedAMP() {
+  if (!IsGoogleCachedAMP()) {
+    return false;
+  }
+  base::WeakPtr<ReadingListDistillerPage> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
+  [CurrentWebState()->GetJSInjectionReceiver()
+      executeJavaScript:@(kJavaScriptNavigateToIframe)
+      completionHandler:^(id result, NSError* error) {
+        if (!weak_this->HandleGoogleCachedAMPJavaScriptResult(result, error)) {

[jif, 2017/01/17 17:43:10]

weak_this can be null.

[Olivier, 2017/01/18 09:30:13]

Interesting question on what to do if weak_this is null.
This is an error, but we cannot call the handler with failure.
So the only think we can possibly do is... nothing. 
Added a test.

+          // If there is an error on navigation, continue normal distillation.
+          weak_this->WaitForPageLoadCompletion();
+        }
+        // If there is no error, the navigation completion will trigger a new
+        // |OnLoadURLDone| call that will resume the distillation.
+      }];
+  return true;
+}
+
+bool ReadingListDistillerPage::HandleGoogleCachedAMPJavaScriptResult(
+    id result,
+    NSError* error) {
+  if (error) {
+    return false;
+  }
+  NSURL* new_url = [NSURL URLWithString:result];

[jif, 2017/01/17 17:43:10]

can you add an DCHECK that verifies that |result| is an NSString?
Also, can you avoid using a NSURL, and only use a GURL?

[Olivier, 2017/01/18 09:30:13]

Added a cast.
If result is not a string (which is possible), the new_url will be nil.

Using NSURL provides hasSuffix, which is really convenient here :)

+  if (!new_url) {
+    return false;
+  }
+  bool is_cdn_ampproject =
+      [[new_url host] isEqualToString:@"cdn.amproject.org"];
+  bool is_cdn_ampproject_subdomain =
+      [[new_url host] hasSuffix:@".cdn.amproject.org"];
+
+  if (!is_cdn_ampproject && !is_cdn_ampproject_subdomain) {
+    return false;
+  }
+  GURL new_gurl = net::GURLWithNSURL(new_url);
+  if (!new_gurl.is_valid()) {
+    return false;
+  }
+  web::NavigationManager::WebLoadParams params(new_gurl);
+  CurrentWebState()->GetNavigationManager()->LoadURLWithParams(params);
+  return true;
+}
+
 }  // namespace reading_list