[Blimp Client] Fixes up URL before sending it to Engine.

blimp/client/feature/navigation_feature.cc

 // 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 "blimp/client/feature/navigation_feature.h"
 
 #include <map>
 #include <string>
 
 #include "blimp/common/create_blimp_message.h"
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/common/proto/navigation.pb.h"
+#include "components/url_formatter/url_fixer.h"
 #include "net/base/net_errors.h"
 #include "url/gurl.h"
 
 namespace blimp {
 namespace client {
 
 NavigationFeature::NavigationFeature() {}
 
 NavigationFeature::~NavigationFeature() {}
 
 void NavigationFeature::set_outgoing_message_processor(
     scoped_ptr<BlimpMessageProcessor> processor) {
   outgoing_message_processor_ = std::move(processor);
 }
 
 void NavigationFeature::SetDelegate(int tab_id,
                                     NavigationFeatureDelegate* delegate) {
   DCHECK(!FindDelegate(tab_id));
   delegates_[tab_id] = delegate;
 }
 
 void NavigationFeature::RemoveDelegate(int tab_id) {
   DelegateMap::iterator it = delegates_.find(tab_id);
   if (it != delegates_.end())
     delegates_.erase(it);
 }
 
 void NavigationFeature::NavigateToUrlText(int tab_id,
                                           const std::string& url_text) {

[Kevin M, 2016/04/07 20:17:00]

Add unit tests for fixing up the URL.

[haibinlu, 2016/04/07 22:28:59]

This is a temp (and wrong) fix for v0.5. not sure if a unit test is necessary.

+  // Simplified heuristic for v0.5 only.

[Kevin M, 2016/04/07 20:12:30]

Add explanation as to *what* this is doing.

[haibinlu, 2016/04/07 22:28:59]

Done.

+  // TODO(haibinlu): Remove once omnibox is used.
+  GURL url = url_formatter::FixupURL(url_text, "");
+  if (!url.is_valid() || url.host().find('.') == std::string::npos) {

[Kevin M, 2016/04/07 20:12:30]

Add comment on the reason and behavior of this heuristic.

[haibinlu, 2016/04/07 22:28:59]

Done.

+    url = url_formatter::FixupURL("https://www.google.com/#q=" + url_text, "");

[Kevin M, 2016/04/07 20:12:30]

url_text needs to be querystring encoded in this context.

[haibinlu, 2016/04/07 22:28:59]

FixupURL will do that.

[Kevin M, 2016/04/07 23:00:19]

What if the searched text was "1+2"? Would FixupURL turn that into 1+2 or 1%2B2?
I imagine it would leave it as the former, whereas we want the latter.

[haibinlu, 2016/04/07 23:55:09]

you are right. it needs encoding.

+  }
+
+  DCHECK(url.is_valid());

[Kevin M, 2016/04/07 20:12:30]

DCHECK is a poor fit here ("url" is built from user input; DCHECKs don't exist
in release builds). Can we just log a message and drop the navigation request?

[haibinlu, 2016/04/07 22:28:59]

removed the DCHECK. with FixupURL on google search, it should be valid.

   NavigationMessage* navigation_message;
   scoped_ptr<BlimpMessage> blimp_message =
       CreateBlimpMessage(&navigation_message, tab_id);
   navigation_message->set_type(NavigationMessage::LOAD_URL);
-  navigation_message->mutable_load_url()->set_url(url_text);
+  navigation_message->mutable_load_url()->set_url(url.spec());
   outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
                                               net::CompletionCallback());
 }
 
 void NavigationFeature::Reload(int tab_id) {
   NavigationMessage* navigation_message;
   scoped_ptr<BlimpMessage> blimp_message =
       CreateBlimpMessage(&navigation_message, tab_id);
   navigation_message->set_type(NavigationMessage::RELOAD);
 
@@ skipping 71 matching lines @@
 NavigationFeature::NavigationFeatureDelegate* NavigationFeature::FindDelegate(
     const int tab_id) {
   DelegateMap::const_iterator it = delegates_.find(tab_id);
   if (it != delegates_.end())
     return it->second;
   return nullptr;
 }
 
 }  // namespace client
 }  // namespace blimp