Merge lp:~gerboland/webbrowser-app/formFactor-support into lp:webbrowser-app

[Gerry Boland]

Use formFactor property supplied by ubuntumirclient QPA where possible. Remove smallScreen heuristics

DO NOT LAND YET. This is (a) something for MWC, and (b) a request for comments if this approach looks reasonable to you

[Olivier Tilloy (osomon)]

I’m not seeing any formFactor property defined on UserAgent02, so I doubt the onFormFactorChanged handler in UbuntuWebContext.qml would work.

Note that I’m currently working on a branch that removes the 'formFactor' property from the Ubuntu.Web plugin’s context object¹, so it’s kind of ironical that you’re adding it back in another branch. I really don’t think the default user agent (and corresponding overrides) should be tied to the "form factor" (whatever that means in a world of convergence). How does mir determine whether a device is a phone or a desktop, or a tablet? Is that a solid API that we are going to support and publicize?

Even if that was a sensible thing to do, we most probably don’t want to get a mobile UX on a tablet. Well, at least not a 10" tablet. On a 7" tablet maybe. See where the screenDiagonal property comes from?

As a side note, the dependency on Qt5Gui_PRIVATE is not very welcome, given that we’ve been steadily working towards entirely removing the dependencies on private Qt packages (ask Mirv about it).

¹ https://code.launchpad.net/~osomon/webbrowser-app/remove-formFactor/+merge/285539

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1358
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2626/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6587/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1379/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1379/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1379/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6598/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2626/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

> I’m not seeing any formFactor property defined on UserAgent02, so I doubt the
> onFormFactorChanged handler in UbuntuWebContext.qml would work.

Code untested. Was struggling to cross compile & deploy, sbuild unhappy with me today.

> Note that I’m currently working on a branch that removes the 'formFactor'
> property from the Ubuntu.Web plugin’s context object¹, so it’s kind of
> ironical that you’re adding it back in another branch. I really don’t think
> the default user agent (and corresponding overrides) should be tied to the
> "form factor" (whatever that means in a world of convergence). How does mir
> determine whether a device is a phone or a desktop, or a tablet? Is that a
> solid API that we are going to support and publicize?

I obviously have no idea of your plans, this was just an attempt at replacing your screen heuristic with something more concrete.

Form Factor will be a hint to the application to tell it what mode the shell is in, on each screen. Mir supports it, shell will do its best to set it correctly. I would hope the average app author has no need for such a property, any form factor specific behaviour would be encapsulated in the UITK.

> Even if that was a sensible thing to do, we most probably don’t want to get a
> mobile UX on a tablet. Well, at least not a 10" tablet. On a 7" tablet maybe.
> See where the screenDiagonal property comes from?

Is your call. The goal of this was to indicate that you can learn if the app is running on a tablet or a desktop UI at the time.

> As a side note, the dependency on Qt5Gui_PRIVATE is not very welcome, given
> that we’ve been steadily working towards entirely removing the dependencies on
> private Qt packages (ask Mirv about it).

Qt doesn't have a nice way to export it to apps yet, hence having to resort to private Qt headers. As I mentioned in a comment, a better solution is for the UITK to do that for you. This isn't the final solution.
Thanks for the comments!

[lp:~gerboland/webbrowser-app/formFactor-support]

1359. By Gerry Boland on 2016-02-22

formFactor not in scope, fix

[Olivier Tilloy (osomon)]

> > Even if that was a sensible thing to do, we most probably don’t want
> > to get a mobile UX on a tablet. Well, at least not a 10" tablet. On
> > a 7" tablet maybe. See where the screenDiagonal property comes from?
>
> Is your call. The goal of this was to indicate that you can learn if
> the app is running on a tablet or a desktop UI at the time.

Thanks, and that’s useful, but I’m not convinced that (or at least that *alone*) is enough to decide what the default UA string should be. I have started a conversation with design about that, we need to come to a consensus. Until that happens, I’d rather keep the current implementation based on screen size (I’m not against refining it though).

[Chris Coulson (chrisccoulson)]

> > I’m not seeing any formFactor property defined on UserAgent02, so I doubt
> the
> > onFormFactorChanged handler in UbuntuWebContext.qml would work.
>
> Code untested. Was struggling to cross compile & deploy, sbuild unhappy with
> me today.
>
> > Note that I’m currently working on a branch that removes the 'formFactor'
> > property from the Ubuntu.Web plugin’s context object¹, so it’s kind of
> > ironical that you’re adding it back in another branch. I really don’t think
> > the default user agent (and corresponding overrides) should be tied to the
> > "form factor" (whatever that means in a world of convergence). How does mir
> > determine whether a device is a phone or a desktop, or a tablet? Is that a
> > solid API that we are going to support and publicize?
>
> I obviously have no idea of your plans, this was just an attempt at replacing
> your screen heuristic with something more concrete.
>
> Form Factor will be a hint to the application to tell it what mode the shell
> is in, on each screen. Mir supports it, shell will do its best to set it
> correctly. I would hope the average app author has no need for such a
> property, any form factor specific behaviour would be encapsulated in the
> UITK.
>

It sounds like this might be useful for bug 1545088, bug 1547145 and bug 1547102. How does the shell determine what the hint is?

> > Even if that was a sensible thing to do, we most probably don’t want to get
> a
> > mobile UX on a tablet. Well, at least not a 10" tablet. On a 7" tablet
> maybe.
> > See where the screenDiagonal property comes from?
>
> Is your call. The goal of this was to indicate that you can learn if the app
> is running on a tablet or a desktop UI at the time.
>
> > As a side note, the dependency on Qt5Gui_PRIVATE is not very welcome, given
> > that we’ve been steadily working towards entirely removing the dependencies
> on
> > private Qt packages (ask Mirv about it).
>
> Qt doesn't have a nice way to export it to apps yet, hence having to resort to
> private Qt headers. As I mentioned in a comment, a better solution is for the
> UITK to do that for you. This isn't the final solution.
> Thanks for the comments!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1359
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2627/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1380
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1380
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1380/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1380
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/5011
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6599
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6599/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27786

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2627/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

1359. By Gerry Boland on 2016-02-22

formFactor not in scope, fix

1358. By Gerry Boland on 2016-02-22

Use formFactor property supplied by ubuntumirclient QPA where possible. Remove smallScreen heuristics

=== modified file 'src/Ubuntu/Components/Extras/Browser/CMakeLists.txt'
--- src/Ubuntu/Components/Extras/Browser/CMakeLists.txt	2015-06-22 10:29:20 +0000
+++ src/Ubuntu/Components/Extras/Browser/CMakeLists.txt	2016-02-22 12:16:23 +0000
@@ -10,6 +10,8 @@
 
 set(PLUGIN_SRC plugin.cpp)
 
+include_directories(${Qt5Gui_PRIVATE_INCLUDE_DIRS})
+
 add_library(${PLUGIN} MODULE ${PLUGIN_SRC})
 target_link_libraries(${PLUGIN}
     Qt5::Core
 
=== modified file 'src/Ubuntu/Web/CMakeLists.txt'
--- src/Ubuntu/Web/CMakeLists.txt	2015-08-25 13:56:58 +0000
+++ src/Ubuntu/Web/CMakeLists.txt	2016-02-22 12:16:23 +0000
@@ -10,6 +10,8 @@
 
 set(PLUGIN_SRC plugin.cpp)
 
+include_directories(${Qt5Gui_PRIVATE_INCLUDE_DIRS})
+
 add_library(${PLUGIN} MODULE ${PLUGIN_SRC})
 target_link_libraries(${PLUGIN}
     Qt5::Core
 
=== modified file 'src/Ubuntu/Web/UbuntuWebContext.qml'
--- src/Ubuntu/Web/UbuntuWebContext.qml	2015-12-11 10:30:42 +0000
+++ src/Ubuntu/Web/UbuntuWebContext.qml	2016-02-22 12:16:23 +0000
@@ -70,13 +70,14 @@
     ]
 
     property QtObject __ua: UserAgent02 {
-        onSmallScreenChanged: reloadOverrides()
+        readonly property string formFactor: formFactor
+        onFormFactorChanged: reloadOverrides()
         Component.onCompleted: reloadOverrides()
 
         property string _target: ""
 
         function reloadOverrides() {
-            var target = smallScreen ? "mobile" : "desktop"
+            var target = formFactor === "MOBILE" ? "mobile" : "desktop"
             if (target == _target) return
             _target = target
             var script = "ua-overrides-%1.js".arg(target)
 
=== modified file 'src/Ubuntu/Web/UserAgent02.qml'
--- src/Ubuntu/Web/UserAgent02.qml	2015-12-10 10:43:04 +0000
+++ src/Ubuntu/Web/UserAgent02.qml	2016-02-22 12:16:23 +0000
@@ -28,10 +28,6 @@
  */
 
 QtObject {
-    // Empirical value: screens smaller than 19cm are considered small enough that a
-    // mobile UA string is used, screens bigger than that will get desktop content.
-    readonly property bool smallScreen: screenDiagonal < 190
-
     // %1: Ubuntu version, e.g. "14.04"
     // %2: optional token to specify further attributes of the platform, e.g. "like Android"
     // %3: optional hardware ID token
@@ -50,7 +46,7 @@
     // FIXME: compute at build time (using lsb_release)
     readonly property string _ubuntuVersion: "14.04"
 
-    readonly property string _attributes: smallScreen ? "like Android 4.4" : ""
+    readonly property string _attributes: formFactor === "MOBILE" ? "like Android 4.4" : ""
 
     readonly property string _hardwareID: ""
 
@@ -63,7 +59,7 @@
     //       every time we rebase on a newer chromium.
     readonly property string _chromiumVersion: "35.0.1870.2"
 
-    readonly property string _formFactor: smallScreen ? "Mobile" : ""
+    readonly property string _formFactor: formFactor === "MOBILE" ? "Mobile" : ""
 
     readonly property string _more: ""
 
 
=== modified file 'src/Ubuntu/Web/plugin.cpp'
--- src/Ubuntu/Web/plugin.cpp	2015-12-10 22:01:05 +0000
+++ src/Ubuntu/Web/plugin.cpp	2016-02-22 12:16:23 +0000
@@ -34,14 +34,15 @@
 #include <QtQml>
 #include <QtQml/QQmlInfo>
 
+#include <QtGui/qpa/qplatformnativeinterface.h>
+
 class UbuntuWebPluginContext : public QObject
 {
     Q_OBJECT
 
     Q_PROPERTY(QString cacheLocation READ cacheLocation NOTIFY cacheLocationChanged)
     Q_PROPERTY(QString dataLocation READ dataLocation NOTIFY dataLocationChanged)
-    Q_PROPERTY(QString formFactor READ formFactor CONSTANT)
-    Q_PROPERTY(qreal screenDiagonal READ screenDiagonal NOTIFY screenDiagonalChanged)
+    Q_PROPERTY(QString formFactor READ formFactor NOTIFY formFactorChanged)
     Q_PROPERTY(int cacheSizeHint READ cacheSizeHint NOTIFY cacheSizeHintChanged)
     Q_PROPERTY(QString webviewDevtoolsDebugHost READ devtoolsHost CONSTANT)
     Q_PROPERTY(int webviewDevtoolsDebugPort READ devtoolsPort CONSTANT)
@@ -53,7 +54,6 @@
     QString cacheLocation() const;
     QString dataLocation() const;
     QString formFactor();
-    qreal screenDiagonal() const;
     int cacheSizeHint() const;
     QString devtoolsHost();
     int devtoolsPort();
@@ -62,15 +62,15 @@
 Q_SIGNALS:
     void cacheLocationChanged() const;
     void dataLocationChanged() const;
-    void screenDiagonalChanged() const;
+    void formFactorChanged() const;
     void cacheSizeHintChanged() const;
 
 private Q_SLOTS:
-    void onFocusWindowChanged(QWindow* window);
-    void updateScreen();
+    void onWindowPropertyChanged(QPlatformWindow* window, const QString &property);
 
 private:
-    qreal m_screenDiagonal; // in millimeters
+    void prepareFormFactor();
+    QString readFormFactorFromPlatform(QPlatformWindow *window);
     QString m_formFactor;
     QString m_devtoolsHost;
     int m_devtoolsPort;
@@ -80,30 +80,57 @@
 
 UbuntuWebPluginContext::UbuntuWebPluginContext(QObject* parent)
     : QObject(parent)
-    , m_screenDiagonal(0)
     , m_devtoolsPort(-2)
     , m_hostMappingRulesQueried(false)
 {
     connect(qApp, SIGNAL(applicationNameChanged()), SIGNAL(cacheLocationChanged()));
     connect(qApp, SIGNAL(applicationNameChanged()), SIGNAL(dataLocationChanged()));
     connect(qApp, SIGNAL(applicationNameChanged()), SIGNAL(cacheSizeHintChanged()));
-    updateScreen();
-    connect(qApp, SIGNAL(focusWindowChanged(QWindow*)), SLOT(onFocusWindowChanged(QWindow*)));
+    prepareFormFactor();
 }
 
-void UbuntuWebPluginContext::updateScreen()
+namespace {
+    // This implementation only considers two possible form factors: desktop,
+    // and mobile (which includes phones and tablets).
+    // XXX: do we need to consider other form factors, such as tablet?
+    const char* DESKTOP = "desktop";
+    const char* MOBILE = "mobile";
+} // anonymous namespace
+
+void UbuntuWebPluginContext::prepareFormFactor()
 {
-    QWindow* window = qApp->focusWindow();
-    if (window) {
-        QScreen* screen = window->screen();
-        if (screen) {
-            QSizeF size = screen->physicalSize();
-            qreal diagonal = qSqrt(size.width() * size.width() + size.height() * size.height());
-            if (diagonal != m_screenDiagonal) {
-                m_screenDiagonal = diagonal;
-                Q_EMIT screenDiagonalChanged();
+    // The "DESKTOP_MODE" environment variable can be used to force the form
+    // factor to desktop, when set to any valid value other than 0.
+    const char* DESKTOP_MODE_ENV_VAR = "DESKTOP_MODE";
+    if (qEnvironmentVariableIsSet(DESKTOP_MODE_ENV_VAR)) {
+        QByteArray stringValue = qgetenv(DESKTOP_MODE_ENV_VAR);
+        bool ok = false;
+        int value = stringValue.toInt(&ok);
+        if (ok) {
+            m_formFactor = (value == 0) ? MOBILE : DESKTOP;
+            return;
+        }
+    }
+
+    // If using QtUbuntu, try fetch formFactor property, falling back to assumption of mobile if that fails
+    QString platform = QGuiApplication::platformName();
+    if (platform != "ubuntumirclient") {
+        m_formFactor = DESKTOP;
+    } else { // We will get notifications of a "formFactor" property changing from the QPA for all windows
+        auto nativeInterface = qGuiApp->platformNativeInterface();
+        connect(nativeInterface,
+                SIGNAL(windowPropertyChanged(QPlatformWindow*, const QString&)),
+                SLOT(onWindowPropertyChanged(QPlatformWindow*, const QString&)));
+
+        auto window = qGuiApp->allWindows().first(); // FIXME: context property not suitable for per-window formFactor
+        if (window) {
+            auto formFactor = readFormFactorFromPlatform(window->handle());
+            if (!formFactor.isEmpty()) {
+                m_formFactor = formFactor;
+                return;
             }
         }
+        m_formFactor = MOBILE; // fallback position
     }
 }
 
@@ -131,43 +158,9 @@
 
 QString UbuntuWebPluginContext::formFactor()
 {
-    if (m_formFactor.isEmpty()) {
-        // This implementation only considers two possible form factors: desktop,
-        // and mobile (which includes phones and tablets).
-        // XXX: do we need to consider other form factors, such as tablet?
-        const char* DESKTOP = "desktop";
-        const char* MOBILE = "mobile";
-
-        // The "DESKTOP_MODE" environment variable can be used to force the form
-        // factor to desktop, when set to any valid value other than 0.
-        const char* DESKTOP_MODE_ENV_VAR = "DESKTOP_MODE";
-        if (qEnvironmentVariableIsSet(DESKTOP_MODE_ENV_VAR)) {
-            QByteArray stringValue = qgetenv(DESKTOP_MODE_ENV_VAR);
-            bool ok = false;
-            int value = stringValue.toInt(&ok);
-            if (ok) {
-                m_formFactor = (value == 0) ? MOBILE : DESKTOP;
-                return m_formFactor;
-            }
-        }
-
-        // XXX: Assume that QtUbuntu means mobile, which is currently the case,
-        // but may not remain true forever.
-        QString platform = QGuiApplication::platformName();
-        if ((platform == "ubuntu") || (platform == "ubuntumirclient")) {
-            m_formFactor = MOBILE;
-        } else {
-            m_formFactor = DESKTOP;
-        }
-    }
     return m_formFactor;
 }
 
-qreal UbuntuWebPluginContext::screenDiagonal() const
-{
-    return m_screenDiagonal;
-}
-
 int UbuntuWebPluginContext::cacheSizeHint() const
 {
     if (QCoreApplication::applicationName() == "webbrowser-app") {
@@ -242,11 +235,49 @@
     return m_devtoolsPort;
 }
 
-void UbuntuWebPluginContext::onFocusWindowChanged(QWindow* window)
-{
-    updateScreen();
-    if (window) {
-        connect(window, SIGNAL(screenChanged(QScreen*)), SLOT(updateScreen()));
+void UbuntuWebPluginContext::onWindowPropertyChanged(QPlatformWindow *window, const QString &property)
+{
+    if (property != QStringLiteral("formFactor")) {
+        return;
+    }
+
+    auto formFactor = readFormFactorFromPlatform(window);
+    if (!formFactor.isEmpty()) {
+        m_formFactor = formFactor;
+        Q_EMIT formFactorChanged();
+    }
+}
+
+// FIXME: this should be a per-window/screen property nicely wrapped by the UITK
+QString UbuntuWebPluginContext::readFormFactorFromPlatform(QPlatformWindow *window)
+{
+    auto nativeInterface = qGuiApp->platformNativeInterface();
+    QVariant formFactorVal = nativeInterface->windowProperty(window, QStringLiteral("formFactor"));
+    if (!formFactorVal.isValid()) {
+        return QString();
+    }
+
+    // taken from MirFormFactor enum in mirclient/common.h
+    typedef enum MirFormFactor
+    {
+        mir_form_factor_unknown,
+        mir_form_factor_phone,
+        mir_form_factor_tablet,
+        mir_form_factor_monitor,
+        mir_form_factor_tv,
+        mir_form_factor_projector,
+    } MirFormFactor;
+
+    bool ok;
+    auto formFactor = static_cast<MirFormFactor>(formFactorVal.toInt(&ok));
+    if (!ok) {
+        return QString();
+    }
+
+    if (formFactor == mir_form_factor_phone || formFactor == mir_form_factor_tablet) {
+        return MOBILE;
+    } else {
+        return DESKTOP;
     }
 }