webbrowser-app

Merge lp:~abreu-alexandre/webbrowser-app/devtools-support into lp:webbrowser-app

devtools-support
Merge into trunk

Proposed by Alexandre Abreu on 2014-07-03

Status:	Merged
Approved by:	Olivier Tilloy on 2014-08-11
Approved revision:	604
Merged at revision:	660
Proposed branch:	lp:~abreu-alexandre/webbrowser-app/devtools-support
Merge into:	lp:webbrowser-app
Diff against target:	151 lines (+48/-19) 6 files modified src/Ubuntu/Web/UbuntuWebContext.qml (+3/-0) src/Ubuntu/Web/plugin.cpp (+20/-0) src/app/browserapplication.cpp (+22/-17) src/app/browserapplication.h (+1/-0) src/app/webbrowser/webbrowser-app.cpp (+1/-1) src/app/webcontainer/webapp-container.cpp (+1/-1)
To merge this branch:	bzr merge lp:~abreu-alexandre/webbrowser-app/devtools-support
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2014-08-11
Olivier Tilloy		2014-07-03	Approve on 2014-08-11
Review via email: mp+225561@code.launchpad.net

Commit message

Add devtools support & ubuntu webview remote debugging

Description of the change

Add devtools support & ubuntu webview remote debugging

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-03:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-28:

142 + out << " --inspector[=PORT] run a remote inspector on a specified port or" << REMOTE_INSPECTOR_PORT << "as the default port" << endl;

155 + out << " --inspector[=PORT] run a remote inspector on a specified port or" << REMOTE_INSPECTOR_PORT << "as the default port" << endl;

You’re missing whitespaces after "or" and before "as". This is how it looks when invoking the app with --help:

run a remote inspector on a specified port or9221as the default port

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-28:

Please revert the no-op change to src/Ubuntu/Web/UbuntuWebView02.qml.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-28:

The rest of the changeset looks ok but I’m wondering whether using an environment variable is the best approach. With QtWebKit we didn’t really have a choice as there was no QML API to expose devtools, the only trigger we could use was the env var.
Here, we could simply set the value of the global context property 'webviewDevtoolsDebugPort' in src/app/browserapplication.cpp, instead of setting the env var there and reading it again in the plugin (disclaimer: not tested, but I think it ought to work). What do you think?

Revision history for this message

David Barth (dbarth) wrote on 2014-07-30:

The env. variables may still just be simpler if the port option needs to be transported to child processes, like the oxide renderers. Otherwise you have to add tons of command line options to multiple progames, where a variable is just the right unix way to do it.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-30:

But the env var isn’t used by oxide at all, it’s just a way to communicate between the app and the Ubuntu.Web plugin, which I think could be achieved in a simpler way.

lp:~abreu-alexandre/webbrowser-app/devtools-support updated on 2014-07-30

602. By Alexandre Abreu on 2014-07-30: nits

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2014-07-30:

When doing this I basically had 2 use cases in mind:

- the webbrowser-app/webapp-container being launched in a debug mode,

- an application (third party) using the ubuntu webview and one wanting to "black box" debug it w/o having to touch it (either because one cannot or should not). I was thinking mostly QA needs to AP test UI componenets that use the UbuntuWebView w/o having to touch it. They would need a way to turn on the devtools w/o having access to the app itself, before being able to drive it w/ selenium etc.

In that context, some sort of an env var seemed ok to me,

The other points have been addressed

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-30:

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

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-30:

> 142 + out << " --inspector[=PORT] run a remote inspector on a
> specified port or" << REMOTE_INSPECTOR_PORT << "as the default port" << endl;
>
> 155 + out << " --inspector[=PORT] run a remote
> inspector on a specified port or" << REMOTE_INSPECTOR_PORT << "as the default
> port" << endl;
>
> You’re missing whitespaces after "or" and before "as". This is how it looks
> when invoking the app with --help:
>
> run a remote inspector on a specified port or9221as the default port

This still needs to be fixed in src/app/webbrowser/webbrowser-app.cpp.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-30:

> When doing this I basically had 2 use cases in mind:
>
> - the webbrowser-app/webapp-container being launched in a debug mode,
>
> - an application (third party) using the ubuntu webview and one wanting to
> "black box" debug it w/o having to touch it (either because one cannot or
> should not). I was thinking mostly QA needs to AP test UI componenets that use
> the UbuntuWebView w/o having to touch it. They would need a way to turn on the
> devtools w/o having access to the app itself, before being able to drive it w/
> selenium etc.
>
> In that context, some sort of an env var seemed ok to me,

I guess that makes sense. One thing I just noticed is that oxide allows us to specify only a numerical port, no host. I guess it means we cannot connect to a webview running on a different device? With QtWebKit, we were able to connect to a webview running on a phone from a separate desktop machine.

lp:~abreu-alexandre/webbrowser-app/devtools-support updated on 2014-07-30

603. By Alexandre Abreu on 2014-07-30: nits

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-30:

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

review: Approve (continuous-integration)

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2014-08-07:

> > When doing this I basically had 2 use cases in mind:
> >
> > - the webbrowser-app/webapp-container being launched in a debug mode,
> >
> > - an application (third party) using the ubuntu webview and one wanting to
> > "black box" debug it w/o having to touch it (either because one cannot or
> > should not). I was thinking mostly QA needs to AP test UI componenets that
> use
> > the UbuntuWebView w/o having to touch it. They would need a way to turn on
> the
> > devtools w/o having access to the app itself, before being able to drive it
> w/
> > selenium etc.
> >
> > In that context, some sort of an env var seemed ok to me,
>
> I guess that makes sense. One thing I just noticed is that oxide allows us to
> specify only a numerical port, no host. I guess it means we cannot connect to
> a webview running on a different device? With QtWebKit, we were able to
> connect to a webview running on a phone from a separate desktop machine.

Yes, this is a shortcoming in oxide that I'll address w/ a MR there,

lp:~abreu-alexandre/webbrowser-app/devtools-support updated on 2014-08-11

604. By Alexandre Abreu on 2014-08-11: fix port parsing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-08-11:

Looks good and works well now. Thanks!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-11:

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alexandre Abreu

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === modified file 'src/Ubuntu/Web/UbuntuWebContext.qml'
 --- src/Ubuntu/Web/UbuntuWebContext.qml	2014-07-08 08:43:57 +0000
 +++ src/Ubuntu/Web/UbuntuWebContext.qml	2014-08-11 14:45:28 +0000
@@ -79,4 +79,7 @@
+     ]
      property QtObject __ua: UserAgent02 {}
++
++    devtoolsEnabled: webviewDevtoolsDebugPort !== -1
++    devtoolsPort: webviewDevtoolsDebugPort
+ }
 === modified file 'src/Ubuntu/Web/plugin.cpp'
 --- src/Ubuntu/Web/plugin.cpp	2014-07-04 11:38:27 +0000
 +++ src/Ubuntu/Web/plugin.cpp	2014-08-11 14:45:28 +0000
@@ -64,6 +64,24 @@
      return DESKTOP;
+ }
++static int getDevtoolsPort()
++{
++    const int DEVTOOLS_INVALID_PORT = -1;
++
++    int port = DEVTOOLS_INVALID_PORT;
++    const char* DEVTOOLS_PORT_ENV_VAR = "UBUNTU_WEBVIEW_DEVTOOLS_PORT";
++
++    if (qEnvironmentVariableIsSet(DEVTOOLS_PORT_ENV_VAR)) {
++        QByteArray environmentVarValue = qgetenv(DEVTOOLS_PORT_ENV_VAR);
++        bool ok = false;
++        int value = environmentVarValue.toInt(&ok);
++        if (ok) {
++            port = value;
++        }
++    }
++    return port > 0 ? port : DEVTOOLS_INVALID_PORT;
++}
++
  void UbuntuBrowserPlugin::initializeEngine(QQmlEngine* engine, const char* uri)
+ {
      Q_UNUSED(uri);
@@ -82,6 +100,8 @@
+     }
      context->setContextProperty("formFactor", getFormFactor());
++
++    context->setContextProperty("webviewDevtoolsDebugPort", getDevtoolsPort());
+ }
  void UbuntuBrowserPlugin::registerTypes(const char* uri)
 === modified file 'src/app/browserapplication.cpp'
 --- src/app/browserapplication.cpp	2014-05-22 15:27:38 +0000
 +++ src/app/browserapplication.cpp	2014-08-11 14:45:28 +0000
@@ -58,6 +58,23 @@
      delete m_engine;
+ }
++QString BrowserApplication::inspectorPort() const
++{
++    QString port;
++    Q_FOREACH(const QString& argument, m_arguments) {
++        if (argument == "--inspector") {
++            // default port
++            port = QString::number(REMOTE_INSPECTOR_PORT);
++            break;
++        }
++        if (argument.startsWith("--inspector=")) {
++            port = argument.split("--inspector=")[1];
++            break;
++        }
++    }
++    return port;
++}
++
  QString BrowserApplication::appId() const
+ {
      Q_FOREACH(const QString& argument, m_arguments) {
@@ -99,22 +116,10 @@
      QOpenGLContextPrivate::setGlobalShareContext(glcontext);
  #endif
--    bool inspector = m_arguments.contains("--inspector");
--    if (inspector) {
--        QString host;
--        Q_FOREACH(QHostAddress address, QNetworkInterface::allAddresses()) {
--            if (!address.isLoopback() && (address.protocol() == QAbstractSocket::IPv4Protocol)) {
--                host = address.toString();
--                break;
--            }
--        }
--        QString server;
--        if (host.isEmpty()) {
--            server = QString::number(REMOTE_INSPECTOR_PORT);
--        } else {
--            server = QString("%1:%2").arg(host, QString::number(REMOTE_INSPECTOR_PORT));
--        }
--        qputenv("QTWEBKIT_INSPECTOR_SERVER", server.toUtf8());
++    QString devtoolsPort = inspectorPort();
++    bool inspectorEnabled = !devtoolsPort.isEmpty();
++    if (inspectorEnabled) {
++        qputenv("UBUNTU_WEBVIEW_DEVTOOLS_PORT", devtoolsPort.toUtf8());
+     }
      m_engine = new QQmlEngine;
@@ -139,7 +144,7 @@
      m_window = qobject_cast<QQuickWindow*>(browser);
      m_webbrowserWindowProxy->setWindow(m_window);
--    browser->setProperty("developerExtrasEnabled", inspector);
++    browser->setProperty("developerExtrasEnabled", inspectorEnabled);
      return true;
+ }
 === modified file 'src/app/browserapplication.h'
 --- src/app/browserapplication.h	2014-05-22 15:27:38 +0000
 +++ src/app/browserapplication.h	2014-08-11 14:45:28 +0000
@@ -58,6 +58,7 @@
  private:
      QString appId() const;
++    QString inspectorPort() const;
      WebBrowserWindow *m_webbrowserWindowProxy;
  };
 === modified file 'src/app/webbrowser/webbrowser-app.cpp'
 --- src/app/webbrowser/webbrowser-app.cpp	2014-07-10 10:09:57 +0000
 +++ src/app/webbrowser/webbrowser-app.cpp	2014-08-11 14:45:28 +0000
@@ -113,7 +113,7 @@
      out << "  -h, --help         display this help message and exit" << endl;
      out << "  --fullscreen       display full screen" << endl;
      out << "  --maximized        opens the application maximized" << endl;
--    out << "  --inspector        run a remote inspector on port " << REMOTE_INSPECTOR_PORT << endl;
++    out << "  --inspector[=PORT] run a remote inspector on a specified port or " << REMOTE_INSPECTOR_PORT << " as the default port" << endl;
      out << "  --app-id=APP_ID    run the application with a specific APP_ID" << endl;
+ }
 === modified file 'src/app/webcontainer/webapp-container.cpp'
 --- src/app/webcontainer/webapp-container.cpp	2014-07-31 21:31:29 +0000
 +++ src/app/webcontainer/webapp-container.cpp	2014-08-11 14:45:28 +0000
@@ -188,7 +188,7 @@
      out << "  --fullscreen                        display full screen" << endl;
      out << "  --local-webapp-manifest             configure the webapp assuming that it has a local manifest.json file" << endl;
      out << "  --maximized                         opens the application maximized" << endl;
--    out << "  --inspector                         run a remote inspector on port " << REMOTE_INSPECTOR_PORT << endl;
++    out << "  --inspector[=PORT]                  run a remote inspector on a specified port or " << REMOTE_INSPECTOR_PORT << " as the default port" << endl;
      out << "  --app-id=APP_ID                     run the application with a specific APP_ID" << endl;
      out << "  --homepage=URL                      override any URL passed as an argument" << endl;
      out << "  --webapp=name                       try to match the webapp by name with an installed integration script" << endl;