[Merge] lp:~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Mon Jun 2 14:54:45 UTC 2014
I have a bunch of comments, see inline.
Diff comments:
> === modified file 'src/app/webcontainer/WebApp.qml'
> --- src/app/webcontainer/WebApp.qml 2014-04-30 01:43:34 +0000
> +++ src/app/webcontainer/WebApp.qml 2014-06-02 13:05:28 +0000
> @@ -37,6 +37,7 @@
> property alias oxide: webview.withOxide
> property alias webappName: webview.webappName
> property alias webappUrlPatterns: webview.webappUrlPatterns
> + property alias blockOpenExternalUrls: webview.blockOpenExternalUrls
>
> actions: [
> Actions.Back {
> @@ -67,6 +68,7 @@
>
> WebappContainerWebview {
> id: webview
> + objectName: "webview"
> toolbar: panel.panel
>
> anchors {
>
> === modified file 'src/app/webcontainer/WebViewImplOxide.qml'
> --- src/app/webcontainer/WebViewImplOxide.qml 2014-05-06 17:21:19 +0000
> +++ src/app/webcontainer/WebViewImplOxide.qml 2014-06-02 13:05:28 +0000
> @@ -35,6 +35,14 @@
> property string localUserAgentOverride: ""
> property var webappUrlPatterns: null
>
> + // Mostly used for testing & avoid external urls to
> + // "leak" in the default browser
> + property bool blockOpenExternalUrls: false
> +
> + signal openExternalUrlTriggered(string url)
> + signal popupWindowOpened(string url)
> + signal popupWindowClosed(string url)
The type of the parameter for all those three signals should be 'url', not 'string'.
> +
> currentWebview: webview
>
> contextualActions: ActionList {
> @@ -62,7 +70,7 @@
> }
>
> function haveValidUrlPatterns() {
> - return webappUrlPatterns && webappUrlPatterns.length !== 0
> + return webview.webappUrlPatterns && webview.webappUrlPatterns.length !== 0
> }
>
> function isNewForegroundWebViewDisposition(disposition) {
> @@ -94,6 +102,12 @@
> return false;
> }
>
> + function openUrlExternally(url) {
> + webview.openExternalUrlTriggered(url)
> + if ( ! webview.blockOpenExternalUrls)
> + Qt.openUrlExternally(url)
> + }
> +
> function navigationRequestedDelegate(request) {
> var newForegroundPageRequest = isNewForegroundWebViewDisposition(request.disposition)
> var url = request.url.toString()
> @@ -112,7 +126,8 @@
> console.debug('Opening: popup window ' + url + ' in the browser window.')
>
> request.action = NavigationRequest.ActionReject
> - Qt.openUrlExternally(url);
> +
> + openUrlExternally(url)
> return;
> }
>
> @@ -147,7 +162,7 @@
>
> if (request.action === NavigationRequest.ActionReject) {
> console.debug('Opening: ' + url + ' in the browser window.')
> - Qt.openUrlExternally(url)
> + openUrlExternally(url)
> }
> }
>
> @@ -171,8 +186,9 @@
> if ( ! isNewForegroundWebViewDisposition(request.disposition) &&
> ! webview.shouldAllowNavigationTo(url)) {
> request.action = NavigationRequest.ActionReject
> - Qt.openUrlExternally(url);
> + openUrlExternally(url)
> popup.close()
> + popupWindowClosed(popupBrowser.url)
> return;
> }
>
> @@ -196,17 +212,21 @@
> // The only way to do it atm is to listen to url changed in popups & also
> // filter there.
> onUrlChanged: {
> - var _url = url.toString();
> + var _url = url.toString();
This looks like an unintended change, can you revert?
> if (_url.trim().length === 0)
> return;
>
> if (_url != 'about:blank' && ! webview.shouldAllowNavigationTo(_url)) {
> - Qt.openUrlExternally(_url);
> + openUrlExternally(url)
> popup.close()
> + popupWindowClosed(popupBrowser.url)
> }
> }
> }
> - Component.onCompleted: popup.show()
> + Component.onCompleted: {
> + popup.show()
> + popupWindowOpened(popupBrowser.url);
Can’t you bind popup.visible to the emission of the popupWindowOpened and popupWindowClosed signals, so that this is handled automatically? The current code looks error-prone, especially if additional use cases for opening/closing the popup are added in the future.
Also, as far as I can tell those signals are there purely for testing purposes, can this be made explicit with a comment?
> + }
> }
> }
>
>
> === modified file 'src/app/webcontainer/WebViewImplWebkit.qml'
> --- src/app/webcontainer/WebViewImplWebkit.qml 2014-04-24 14:15:51 +0000
> +++ src/app/webcontainer/WebViewImplWebkit.qml 2014-06-02 13:05:28 +0000
> @@ -40,6 +40,9 @@
> return webview.localUserAgentOverride.length === 0 ? undefined : webview.localUserAgentOverride
> }
>
> + // Is a no-op for QtWebkit
> + property bool blockOpenExternalUrls: false
> +
> experimental.certificateVerificationDialog: CertificateVerificationDialog {}
> experimental.authenticationDialog: AuthenticationDialog {}
> experimental.proxyAuthenticationDialog: ProxyAuthenticationDialog {}
>
> === modified file 'src/app/webcontainer/WebappContainerWebview.qml'
> --- src/app/webcontainer/WebappContainerWebview.qml 2014-04-24 14:15:51 +0000
> +++ src/app/webcontainer/WebappContainerWebview.qml 2014-06-02 13:05:28 +0000
> @@ -32,6 +32,7 @@
> property string webappName: ""
> property var currentWebview: webappContainerWebViewLoader.item
> property var toolbar: null
> + property bool blockOpenExternalUrls: false
> property var webappUrlPatterns
> property string localUserAgentOverride: ""
>
> @@ -53,7 +54,8 @@
> , url: containerWebview.url
> , webappName: containerWebview.webappName
> , webappUrlPatterns: containerWebview.webappUrlPatterns
> - , developerExtrasEnabled: containerWebview.developerExtrasEnabled})
> + , developerExtrasEnabled: containerWebview.developerExtrasEnabled
> + , blockOpenExternalUrls: blockOpenExternalUrls})
For consistency with the rest of the parameters, this should be:
, blockOpenExternalUrls: containerWebview.blockOpenExternalUrls})
> }
> }
>
>
> === modified file 'src/app/webcontainer/webapp-container.cpp'
> --- src/app/webcontainer/webapp-container.cpp 2014-05-08 16:51:08 +0000
> +++ src/app/webcontainer/webapp-container.cpp 2014-06-02 13:05:28 +0000
> @@ -79,7 +79,8 @@
> m_storeSessionCookies(false),
> m_backForwardButtonsVisible(false),
> m_addressBarVisible(false),
> - m_localWebappManifest(false)
> + m_localWebappManifest(false),
> + m_blockOpenExternalUrls(false)
> {
> }
>
> @@ -107,6 +108,8 @@
> qDebug() << "Using" << (m_withOxide ? "Oxide" : "QtWebkit") << "as the web engine backend";
> m_window->setProperty("oxide", m_withOxide);
>
> + m_window->setProperty("blockOpenExternalUrls", m_blockOpenExternalUrls);
> +
> m_window->setProperty("webappUrlPatterns", m_webappUrlPatterns);
> QQmlContext* context = m_engine->rootContext();
> if (m_storeSessionCookies) {
> @@ -170,6 +173,11 @@
>
> void WebappContainer::parseCommandLine()
> {
> + // Used internall for testing when the browsed urls
> + // are not totally properly formed for strict url patterns
> + // (localhost:port, <ip>:port, etc.)
> + bool doFilterWebappUrlPatterns = true;
> +
> Q_FOREACH(const QString& argument, m_arguments) {
> if (argument == "--webkit") {
> // force webkit
> @@ -177,6 +185,10 @@
> } else if (argument == "--oxide") {
> // force oxide
> m_withOxide = true;
> + } else if (argument == "--do-not-filter-url-patterns") {
> + doFilterWebappUrlPatterns = false;
> + qWarning() << "NOT filtering webapp url patterns. "
> + "Only use this for internal testing, not production.";
I can already picture webapp authors using this undocumented switch to circumvent the default filtering behaviour :/
Isn’t there a more secure way of writing tests?
> } else if (argument.startsWith("--webappModelSearchPath=")) {
> m_webappModelSearchPath = argument.split("--webappModelSearchPath=")[1];
> } else if (argument.startsWith("--webapp=")) {
> @@ -188,7 +200,10 @@
> QString tail = argument.split("--webappUrlPatterns=")[1];
> if (!tail.isEmpty()) {
> QStringList includePatterns = tail.split(URL_PATTERN_SEPARATOR);
> - m_webappUrlPatterns = UrlPatternUtils::filterAndTransformUrlPatterns(includePatterns);
> + m_webappUrlPatterns =
> + doFilterWebappUrlPatterns ?
> + UrlPatternUtils::filterAndTransformUrlPatterns(includePatterns)
> + : includePatterns;
> }
> } else if (argument == "--store-session-cookies") {
> m_storeSessionCookies = true;
> @@ -198,6 +213,8 @@
> m_addressBarVisible = true;
> } else if (argument == "--local-webapp-manifest") {
> m_localWebappManifest = true;
> + } else if (argument == "--block-open-external-urls") {
> + m_blockOpenExternalUrls = true;
> }
> }
> }
>
> === modified file 'src/app/webcontainer/webapp-container.h'
> --- src/app/webcontainer/webapp-container.h 2014-05-08 16:51:08 +0000
> +++ src/app/webcontainer/webapp-container.h 2014-06-02 13:05:28 +0000
> @@ -49,6 +49,7 @@
> bool m_backForwardButtonsVisible;
> bool m_addressBarVisible;
> bool m_localWebappManifest;
> + bool m_blockOpenExternalUrls;
>
> static const QString URL_PATTERN_SEPARATOR;
> };
>
> === modified file 'src/app/webcontainer/webapp-container.qml'
> --- src/app/webcontainer/webapp-container.qml 2014-04-22 15:35:16 +0000
> +++ src/app/webcontainer/webapp-container.qml 2014-06-02 13:05:28 +0000
> @@ -33,6 +33,7 @@
> property alias webappModelSearchPath: browser.webappModelSearchPath
> property alias webappUrlPatterns: browser.webappUrlPatterns
> property alias oxide: browser.oxide
> + property alias blockOpenExternalUrls: browser.blockOpenExternalUrls
>
> contentOrientation: browser.screenOrientation
>
>
> === modified file 'tests/autopilot/webapp_container/tests/__init__.py'
> --- tests/autopilot/webapp_container/tests/__init__.py 2014-05-02 05:44:36 +0000
> +++ tests/autopilot/webapp_container/tests/__init__.py 2014-06-02 13:05:28 +0000
> @@ -43,6 +43,10 @@
> return LOCAL_BROWSER_CONTAINER_PATH_NAME
> return INSTALLED_BROWSER_CONTAINER_PATH_NAME
>
> + def setUp(self):
> + self.pointing_device = toolkit_emulators.get_pointing_device()
> + super(WebappContainerTestCaseBase, self).setUp()
> +
> def launch_webcontainer_app(self, args):
> if model() != 'Desktop':
> args.append(
> @@ -62,6 +66,13 @@
> def get_webcontainer_window(self):
> return self.app.select_single(objectName="webappContainer")
>
> + def get_webview(self):
> + return self.app.select_single(objectName="webview")
> +
> + def get_oxide_webview(self):
> + return self.get_webview().select_many(
> + 'QQuickLoader')[1].get_children()[0]
This is highly unreliable. The documentation of select_many states that you shouldn’t rely on the order of the returned objects. Can’t you use select_single instead, passing it additional parameters to match properties of the webview you’re selecting?
> +
> def get_webcontainer_webview(self):
> return self.app.select_single(objectName="webappBrowserView")
>
>
> === modified file 'tests/autopilot/webapp_container/tests/fake_servers.py'
> --- tests/autopilot/webapp_container/tests/fake_servers.py 2014-04-24 10:50:42 +0000
> +++ tests/autopilot/webapp_container/tests/fake_servers.py 2014-06-02 13:05:28 +0000
> @@ -42,6 +42,90 @@
> if self.path == '/':
> self.send_response(200)
> self.serve_content(self.basic_html_content())
> + elif self.path == '/href-link':
> + self.send_response(200)
> + html_response = """
> + <html>
> + <head>
> + <script>
> + window.onload = function() {
> + document.addEventListener('click', function() {
> + document.querySelector('div a').click();
> + });
> + };
> + </script>
> + </head>
> + <body>
> + <div style="width: 100%; height: 100%">
> + <h1><a href="target-blank">local href link</a></h1>
> + </div>
> + </body>
> + </html>
> + """
> + self.serve_content(html_response)
> + elif self.path == '/redirect':
> + self.send_response(303)
> + self.send_header("Location", "href-link")
> + self.end_headers()
> + elif self.path == '/window-open-to-redirect':
> + self.send_response(200)
> + html_response = """
> + <html>
> + <head>
> + <script>
> + window.onload = function() {
> + document.addEventListener('click', function() {
> + window.open("redirect");
> + });
> + };
> + </script>
> + </head>
> + <body>
> + <div style="width: 100%; height: 100%"></div>
> + </body>
> + </html>
> + """
> + self.serve_content(html_response)
> + elif self.path == '/window-open':
> + self.send_response(200)
> + html_response = """
> + <html>
> + <head>
> + <script>
> + window.onload = function() {
> + document.addEventListener('click', function() {
> + window.open("window-open");
> + });
> + };
> + </script>
> + </head>
> + <body>
> + <div style="width: 100%; height: 100%"></div>
> + </body>
> + </html>
> + """
> + self.serve_content(html_response)
> + elif self.path == '/target-blank':
> + self.send_response(200)
> + html_response = """
> + <html>
> + <head>
> + <script>
> + window.onload = function() {
> + document.addEventListener('click', function() {
> + document.querySelector('div a').click();
> + });
> + };
> + </script>
> + </head>
> + <body>
> + <div style="width: 100%; height: 100%">
> + <h1><a href="target-blank" target="_blank">target-blank</a></h1>
> + </div>
> + </body>
> + </html>
> + """
> + self.serve_content(html_response)
> else:
> self.send_error(404)
>
>
> === added file 'tests/autopilot/webapp_container/tests/test_navigation_filtering.py'
> --- tests/autopilot/webapp_container/tests/test_navigation_filtering.py 1970-01-01 00:00:00 +0000
> +++ tests/autopilot/webapp_container/tests/test_navigation_filtering.py 2014-06-02 13:05:28 +0000
> @@ -0,0 +1,94 @@
> +# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
> +# Copyright 2014 Canonical
> +#
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 3, as published
> +# by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +from __future__ import absolute_import
> +
> +from testtools import skipUnless
> +from testtools.matchers import Equals, Contains
> +from autopilot.matchers import Eventually
> +from autopilot import platform
> +
> +from webapp_container.tests import WebappContainerTestCaseWithLocalContentBase
> +
> +
> +class WebappContainerNavivationFilteringTestCase(
> + WebappContainerTestCaseWithLocalContentBase):
> +
> + def test_no_patterns(self):
> + args = []
> + self.launch_webcontainer_app_with_local_http_server(args, "/href-link")
> +
> + webview = self.get_oxide_webview()
> + url_watcher = webview.watch_signal('urlChanged()')
> +
> + self.assertThat(lambda: webview.url,
> + Eventually(Contains("/href-link")))
> +
> + self.pointing_device.click_object(webview)
> + self.assertThat(lambda: url_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(lambda: webview.url,
> + Eventually(Contains("/target-blank")))
> +
> + def test_url_pattern_filtering(self):
> + args = ['--do-not-filter-url-patterns',
> + '--webappUrlPatterns=' + self.base_url + '/href-link',
> + '--block-open-external-urls']
> + self.launch_webcontainer_app_with_local_http_server(args, "/href-link")
> +
> + webview = self.get_oxide_webview()
> + self.assertThat(lambda: webview.url,
> + Eventually(Contains("/href-link")))
> +
> + external_open_watcher = webview.watch_signal(
> + 'openExternalUrlTriggered(QString)')
> + url_watcher = webview.watch_signal('urlChanged()')
> +
> + self.pointing_device.click_object(webview)
> + self.assertThat(lambda: external_open_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(url_watcher.was_emitted,
> + Equals(False))
> +
> + @skipUnless(platform.model() == 'Desktop', "Only runs on the Desktop")
> + def test_url_pattern_filtering_in_popup(self):
> + args = ['--do-not-filter-url-patterns',
> + '--webappUrlPatterns=' + self.base_url
According to pep8 this continuation line is under-indented for visual indent.
> + + '/window-open-to-redirect,' + self.base_url + '/redirect',
> + '--block-open-external-urls']
> + self.launch_webcontainer_app_with_local_http_server(
> + args, "/window-open-to-redirect")
> +
> + webview = self.get_oxide_webview()
> + self.assertThat(lambda: webview.url,
> + Eventually(Contains("/window-open-to-redirect")))
> +
> + external_open_watcher = webview.watch_signal(
> + 'openExternalUrlTriggered(QString)')
> + url_watcher = webview.watch_signal('urlChanged()')
> + popup_watcher = webview.watch_signal('popupWindowOpened(QString)')
> + popup_closed_watcher = webview.watch_signal(
> + 'popupWindowClosed(QString)')
> +
> + self.pointing_device.click_object(webview)
> +
> + self.assertThat(lambda: popup_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(lambda: external_open_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(lambda: popup_closed_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(url_watcher.was_emitted,
> + Equals(False))
>
> === added file 'tests/autopilot/webapp_container/tests/test_navigation_popup.py'
> --- tests/autopilot/webapp_container/tests/test_navigation_popup.py 1970-01-01 00:00:00 +0000
> +++ tests/autopilot/webapp_container/tests/test_navigation_popup.py 2014-06-02 13:05:28 +0000
> @@ -0,0 +1,58 @@
> +# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
> +# Copyright 2014 Canonical
> +#
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 3, as published
> +# by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +from __future__ import absolute_import
> +
> +from testtools.matchers import Equals
> +from autopilot.matchers import Eventually
> +from autopilot import platform
> +
> +from webapp_container.tests import WebappContainerTestCaseWithLocalContentBase
> +
> +
> +class WebappContainerNavivationRequestPolicyTestCase(
> + WebappContainerTestCaseWithLocalContentBase):
> +
> + def test_popup_window_open(self):
> + args = ['--block-open-external-urls']
> + popup_should_be_opened = platform.model() == "Desktop"
> + self.launch_webcontainer_app_with_local_http_server(
> + args, "/window-open")
> +
> + webview = self.get_oxide_webview()
> + popup_watcher = webview.watch_signal('popupWindowOpened(QString)')
> + url_watcher = webview.watch_signal('urlChanged()')
> +
> + self.pointing_device.click_object(webview)
> + self.assertThat(lambda: popup_watcher.was_emitted,
> + Eventually(Equals(popup_should_be_opened)))
> + self.assertThat(lambda: url_watcher.was_emitted,
> + Eventually(Equals(False)))
> +
> + def test_popup_target_blank(self):
> + args = ['--block-open-external-urls']
> + popup_should_be_opened = platform.model() == "Desktop"
> + self.launch_webcontainer_app_with_local_http_server(
> + args, "/target-blank")
> +
> + webview = self.get_oxide_webview()
> + popup_watcher = webview.watch_signal('popupWindowOpened(QString)')
> + url_watcher = webview.watch_signal('urlChanged()')
> +
> + self.pointing_device.click_object(webview)
> + self.assertThat(lambda: popup_watcher.was_emitted,
> + Eventually(Equals(popup_should_be_opened)))
> + self.assertThat(lambda: url_watcher.was_emitted,
> + Eventually(Equals(False)))
>
--
https://code.launchpad.net/~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup/+merge/217811
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup into lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list