Merge lp:~zaspire/oxide/lp_1259219 into lp:~oxide-developers/oxide/oxide.trunk

[Maxim Ermilov]

[lp:~zaspire/oxide/lp_1259219]

435. By Chris Coulson on 2014-03-24

BrowserContext::GetCachePath() and BrowserContext::GetPath() no longer return an empty path for OTR contexts, and BrowserContext::IsOffTheRecord() always returns false for the main context, since r432

436. By Olivier Tilloy on 2014-03-24

Implement a dummy AccessTokenStore to prevent geolocation requests from crashing the renderer.

437. By Chris Coulson on 2014-03-24

Update exclude list for tarball

438. By Chris Coulson on 2014-03-25

This provides some improvements to the changes in r425-r431. It:
- Makes IOThreadGlobals::Data publically available as Globals, and renames
  IOThreadGlobals to IOThread. This provides a proper API split for accessing
  things that belong to the IO thread
- Reverts the change that made IOThread a singleton - we want to control when
  to destroy it
- Destroys the system URL request context before the IO thread has gone away
  (fixes LP: #1296668)
- Gets rid of URLRequestContextInitializer - this was overcomplicated. Now, we
  subclass URLRequestContextGetter instead
- Move the initialization for common paramters out of URLRequestContextGetter
  in to the constructor for URLRequestContext so that we don't need to rely
  on URLRequestContextGetter to create the request context. This was a bit
  weird anyway

[Chris Coulson (chrisccoulson)]

Hi,

Thanks for working on this. I do have some concerns with the current approach though.

First of all, on coding style:
- We try to stick with an 80 character line length where possible. I do use vertical splits in vim quite a bit, and longer lines makes the code quite difficult to read. Of course, we make exceptions where it's not really possible to stick within 80 characters. There's plenty of those already in the tree.

Some comments for the actual API:
- OxideQQuickNavigationRequest could probably go in qt/core/api (called OxideQNavigationRequest), as it seems like the sort of thing that could be shared with a QWidget implementation, should we decide to build one.
- It's not really obvious what shouldFork means, but I think that's probably because it's not really the correct API in chromium for this mechanism. More on that in a bit :)
- It's missing something akin to Chromium's PageTransition, which details what type of navigation it is - I wouldn't expose everything in PageTransition, but I'd probably start by looking at the types currently exposed by QtWebkit's NavigationType.
- It should probably also expose something based on the WindowOpenDisposition - ie, whether this navigation is requested in the current view or whether a new tab or window will be requested. At the moment, we don't handle creating new view's, but I'm currently working on that as part of bug 1240749 and that will also handle anchor elements with target="_blank".
- Whilst the initial implementation should only handle this for top-level frames, it might be prudent to future-proof it by either including a frame property, or perhaps just a boolean property which says whether this navigation is in a top-level frame. QtWebkit currently does the latter, and I think that would be fine.

For the general implementation:
- There are already 2 BrowserMessageFilter's handling single IPC messages. If we're going to add another one, I think that we should probably just merge these in to a single class (perhaps keeping ScriptMessageDispatcherBrowser separate, as that does contain all of the routing logic for script messages, so it's quite a special case). However, we probably don't need this class anyway.

- I'm not sure that ContentRendererClient::ShouldFork() is the right way of doing this. This is really for embedders to state that a navigation should be performed in a new process, rather than asking the embedder whether the navigation should be handled at all.

Currently, when returning true from ShouldFork() (which appears to be the value to reject the navigation), the renderer requests the browser to do the navigation anyway. This doesn't do anything because we don't implement WebContentsDelegate::OpenURLFromTab(), but I think we should.

The way I think this should probably work is:
- We set RendererPreferences::browser_handles_all_top_level_requests to true. This will make RenderFrameImpl::DecidePolicyForNavigation() delegate all top-level navigations to the browser process.
- We implement WebContentsDelegate::OpenURLFromTab() - from here, we can dispatch the onNavigationRequest, and then handle the actual navigation by using NavigationController::LoadURLWithParams(). Y...

review: Needs Fixing

[Chris Coulson (chrisccoulson)]

Olivier, I added you as well just in case you have any feedback, particularly on the API design

[lp:~zaspire/oxide/lp_1259219]

439. By Olivier Tilloy on 2014-03-25

Implement the JavaScriptDialogManager interface.

[Olivier Tilloy (osomon)]

I don’t have much to add to Chris’ thorough review. Just a couple of comments:

 - 'shouldFork' is indeed not self-explanatory as a property name, how about 'accept'? Or an enum instead of a boolean, as is done in QtWebKit?

 - Looking at QtWebKit’s implementation (it looks like it’s not implemented in QtWebEngine yet), the NavigationRequest object exposes more information (besides the 'isMainFrame' property suggested by Chris) including navigation type, mouse buttons and keyboard modifiers. I don’t know how relevant/useful those properties are though, just pointing them out in case we want to expose them too.

 - In OxideQQuickNavigationRequest, the 'url' property could be marked CONSTANT FINAL

[lp:~zaspire/oxide/lp_1259219]

440. By Maxim Ermilov on 2014-03-28

add OxideQQuickWebView::navigationRequested

[Maxim Ermilov (zaspire)]

(branch updated)

[Chris Coulson (chrisccoulson)]

Thanks for the update. There are still some issues though:

A couple of fairly major ones:
- It looks like this will block navigations if the embedder doesn't implement onNavigationRequested. This seems wrong (and should break some of the existing automated tests too).
- It's not setting LoadURLParams::override_user_agent, meaning that we lose any custom user agent string (even the default that is set by WebContext.userAgent) after the first navigation.
- Setting LoadURLParams::should_replace_current_entry looks wrong. This will result in the new navigation always replacing the current entry in the back/forward list.

In addition to this, I have some other minor comments:
- WebContentsDelegate::OpenURLFromTab should be implemented in oxide::WebView really (then you wouldn't need to expose web_contents_ to derived classes).
- This doesn't address some of my other comments, particularly that the new API should probably go in qt/core/api and it doesn't expose the navigation type or whether the navigation is even going to happen in the current webview. Although, it doesn't look like Chromium gives us the navigation type at the moment, so we could probably leave that for now.

I'll merge this in to https://code.launchpad.net/~chrisccoulson/oxide/window-opening for now and fix up the remaining issues there, as I need this in order to finish off that work

[Chris Coulson (chrisccoulson)]

Ooh, one other thing I forgot to mention :)

Moving oxide::WebView::web_contents_ results in our reference to its BrowserContext being released before the WebContents is deleted, which results in a use-after-free. There did used to be a comment stating that the order of these members shouldn't be changed, but that seems to have disappeared at some point

=== modified file 'qt/core/glue/oxide_qt_web_view_adapter.h'
--- qt/core/glue/oxide_qt_web_view_adapter.h	2014-03-04 15:45:37 +0000
+++ qt/core/glue/oxide_qt_web_view_adapter.h	2014-03-28 01:21:58 +0000
@@ -99,6 +99,7 @@
   virtual void NavigationEntryCommitted() = 0;
   virtual void NavigationListPruned(bool from_front, int count) = 0;
   virtual void NavigationEntryChanged(int index) = 0;
+  virtual bool NavigationRequested(const QString &url) = 0;
 
   virtual WebFrameAdapter* CreateWebFrame() = 0;
 

=== modified file 'qt/core/glue/private/oxide_qt_web_view_adapter_p.cc'
--- qt/core/glue/private/oxide_qt_web_view_adapter_p.cc	2014-03-26 21:25:15 +0000
+++ qt/core/glue/private/oxide_qt_web_view_adapter_p.cc	2014-03-28 01:21:58 +0000
@@ -22,6 +22,8 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "url/gurl.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 
 #include "qt/core/api/oxideqloadevent.h"
 #include "qt/core/api/oxideqloadevent_p.h"
@@ -39,6 +41,29 @@
 WebViewAdapterPrivate::WebViewAdapterPrivate(WebViewAdapter* adapter) :
     a(adapter) {}
 
+content::WebContents* WebViewAdapterPrivate::OpenURLFromTab(content::WebContents* source,
+                                                            const content::OpenURLParams& p) {
+  if (a->NavigationRequested(QString::fromStdString(p.url.spec()))) {
+    content::NavigationController& controller = web_contents_->GetController();
+
+    content::NavigationController::LoadURLParams params(p.url);
+
+    params.referrer = p.referrer;
+    params.redirect_chain = p.redirect_chain;
+    params.should_replace_current_entry = true;
+    params.is_renderer_initiated = p.is_renderer_initiated;
+    params.extra_headers = p.extra_headers;
+    params.transition_type = p.transition;
+    params.frame_tree_node_id = p.frame_tree_node_id;
+    controller.LoadURLWithParams(params);
+
+    return web_contents_.get();
+  }
+
+  return NULL;
+}
+
+
 void WebViewAdapterPrivate::OnURLChanged() {
   a->URLChanged();
 }

=== modified file 'qt/core/glue/private/oxide_qt_web_view_adapter_p.h'
--- qt/core/glue/private/oxide_qt_web_view_adapter_p.h	2014-03-04 15:45:37 +0000
+++ qt/core/glue/private/oxide_qt_web_view_adapter_p.h	2014-03-28 01:21:58 +0000
@@ -33,6 +33,9 @@
  public:
   static WebViewAdapterPrivate* Create(WebViewAdapter* adapter);
 
+  content::WebContents* OpenURLFromTab(content::WebContents* source,
+                                       const content::OpenURLParams& params) OVERRIDE;
+
   size_t GetScriptMessageHandlerCount() const FINAL;
   oxide::ScriptMessageHandler* GetScriptMessageHandlerAt(
       size_t index) const FINAL;

=== modified file 'qt/quick/api/oxideqquickwebview.cc'
--- qt/quick/api/oxideqquickwebview.cc	2014-03-20 12:04:34 +0000
+++ qt/quick/api/oxideqquickwebview.cc	2014-03-28 01:21:58 +0000
@@ -181,6 +181,15 @@
   navigationHistory.onNavigationEntryChanged(index);
 }
 
+bool OxideQQuickWebViewPrivate::NavigationRequested(const QString &url) {
+  Q_Q(OxideQQuickWebView);
+
+  OxideQQuickNavigationRequest request(url);
+  emit q->navigationRequested(&request);
+
+  return request.accept();
+}
+
 oxide::qt::WebFrameAdapter* OxideQQuickWebViewPrivate::CreateWebFrame() {
   OxideQQuickWebFrame* frame = new OxideQQuickWebFrame();
   QQmlEngine::setObjectOwnership(frame, QQmlEngine::CppOwnership);

=== modified file 'qt/quick/api/oxideqquickwebview_p.h'
--- qt/quick/api/oxideqquickwebview_p.h	2014-03-20 12:04:34 +0000
+++ qt/quick/api/oxideqquickwebview_p.h	2014-03-28 01:21:58 +0000
@@ -56,6 +56,30 @@
   OxideQQuickWebView* view_;
 };
 
+class OxideQQuickNavigationRequest: public QObject {
+  Q_OBJECT
+  Q_PROPERTY(bool accept READ accept WRITE setAccept)
+  Q_PROPERTY(QString url READ url)
+ public:
+  OxideQQuickNavigationRequest(const QString &url): accept_(false), url_(url) {}
+
+  bool accept() {
+    return accept_;
+  }
+  void setAccept(bool value) {
+    accept_ = value;
+  }
+
+  const QString& url() {
+    return url_;
+  }
+ private:
+  bool accept_;
+  const QString url_;
+};
+
+QML_DECLARE_TYPE(OxideQQuickNavigationRequest)
+
 class OxideQQuickWebView : public QQuickItem {
   Q_OBJECT
   Q_PROPERTY(QUrl url READ url WRITE setUrl NOTIFY urlChanged)
@@ -144,6 +168,7 @@
   void urlChanged();
   void titleChanged();
   void navigationHistoryChanged();
+  void navigationRequested(OxideQQuickNavigationRequest *request);
   void loadingChanged(OxideQLoadEvent* loadEvent);
   void loadProgressChanged();
   void rootFrameChanged();

=== modified file 'qt/quick/api/oxideqquickwebview_p_p.h'
--- qt/quick/api/oxideqquickwebview_p_p.h	2014-03-20 12:04:34 +0000
+++ qt/quick/api/oxideqquickwebview_p_p.h	2014-03-28 01:21:58 +0000
@@ -68,6 +68,7 @@
   void NavigationEntryCommitted() Q_DECL_FINAL;
   void NavigationListPruned(bool from_front, int count) Q_DECL_FINAL;
   void NavigationEntryChanged(int index) Q_DECL_FINAL;
+  bool NavigationRequested(const QString &url) Q_DECL_FINAL;
 
   oxide::qt::WebFrameAdapter* CreateWebFrame() Q_DECL_FINAL;
 

=== modified file 'qt/quick/oxide_qml_plugin.cc'
--- qt/quick/oxide_qml_plugin.cc	2014-03-17 15:59:29 +0000
+++ qt/quick/oxide_qml_plugin.cc	2014-03-28 01:21:58 +0000
@@ -70,6 +70,8 @@
     qmlRegisterType<OxideQQuickWebContext>(uri, 0, 1, "WebContext");
     qmlRegisterUncreatableType<OxideQQuickNavigationHistory>(uri, 0, 1, "NavigationHistory",
         "Each WebView has a NavigationHistory automatically instantiated by Oxide");
+    qmlRegisterUncreatableType<OxideQQuickNavigationRequest>(uri, 0, 1, "NavigationRequest",
+        "Cannot create separate instance of NavigationRequest");
     qmlRegisterType<OxideQWebPreferences>(uri, 0, 1, "WebPreferences");
     qmlRegisterType<OxideQQuickWebView>(uri, 0, 1, "WebView");
     qmlRegisterType<OxideQQuickWebContextDelegateWorker>(uri, 0, 1, "WebContextDelegateWorker");

=== added file 'qt/tests/qmltests/api/tst_WebView_navigation_request.qml'
--- qt/tests/qmltests/api/tst_WebView_navigation_request.qml	1970-01-01 00:00:00 +0000
+++ qt/tests/qmltests/api/tst_WebView_navigation_request.qml	2014-03-28 01:21:58 +0000
@@ -0,0 +1,59 @@
+import QtQuick 2.0
+import QtTest 1.0
+import com.canonical.Oxide 0.1
+import com.canonical.Oxide.Testing 0.1
+
+TestWebView {
+    id: webView
+    focus: true
+    width: 200
+    height: 200
+
+    SignalSpy {
+        id: spy
+        target: webView
+        signalName: "navigationHistoryChanged"
+    }
+
+    SignalSpy {
+        id: urlSpy
+        target: webView
+        signalName: "urlChanged"
+    }
+
+    onNavigationRequested: {
+        request.accept = false;
+    }
+
+    TestCase {
+        id: test
+        name: "WebView_navigation_request"
+        when: windowShown
+
+        function init() {
+            spy.clear();
+        }
+
+        readonly property var initData: [
+            { url: "http://localhost:8080/tst_WebView_navigation_request1.html", index: 0 }
+        ]
+
+        function test_WebView_navigation_request1_init_data() {
+            return initData;
+        }
+
+        function test_WebView_navigation_request1_init(data) {
+            webView.url = data.url;
+
+            verify(webView.waitForLoadSucceeded(),
+                   "Timed out waiting for load to finish");
+
+            compare(webView.url, data.url,
+                    "WebView.url is invalid after load");
+            webView.waitForLoadSucceeded();
+            compare(webView.url, data.url,
+                    "navigation request should be ignored");
+        }
+
+    }
+}

=== added file 'qt/tests/qmltests/api/tst_WebView_navigation_request1.html'
--- qt/tests/qmltests/api/tst_WebView_navigation_request1.html	1970-01-01 00:00:00 +0000
+++ qt/tests/qmltests/api/tst_WebView_navigation_request1.html	2014-03-28 01:21:58 +0000
@@ -0,0 +1,12 @@
+<html>
+<head>
+ <title>Navigation test 1</title>
+</head>
+<body>
+<script type="text/javascript">
+ open('http://google.com', '_self');
+</script>
+
+ <h1>Navigation test 1</h1>
+</body>
+</html>

=== modified file 'shared/browser/oxide_web_view.cc'
--- shared/browser/oxide_web_view.cc	2014-03-26 21:10:42 +0000
+++ shared/browser/oxide_web_view.cc	2014-03-28 01:21:58 +0000
@@ -339,6 +339,8 @@
   // HttpUserAgentSettings::GetUserAgent()
   web_contents_->SetUserAgentOverride(context->GetUserAgent());
 
+  web_contents_->GetMutableRendererPrefs()->browser_handles_non_local_top_level_requests = true;
+
   registrar_.Add(this, content::NOTIFICATION_NAV_LIST_PRUNED,
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_CHANGED,

=== modified file 'shared/browser/oxide_web_view.h'
--- shared/browser/oxide_web_view.h	2014-03-26 21:10:42 +0000
+++ shared/browser/oxide_web_view.h	2014-03-28 01:21:58 +0000
@@ -144,6 +144,7 @@
  protected:
   WebView();
 
+  scoped_ptr<content::WebContentsImpl> web_contents_;
  private:
   void DispatchLoadFailed(const GURL& validated_url,
                           int error_code,
@@ -248,7 +249,6 @@
   virtual WebPopupMenu* CreatePopupMenu(content::RenderViewHost* rvh);
 
   ScopedBrowserContext context_;
-  scoped_ptr<content::WebContentsImpl> web_contents_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<WebFrame> root_frame_;
   base::WeakPtr<WebPopupMenu> active_popup_menu_;