webbrowser-app

Merge lp:~abreu-alexandre/webbrowser-app/fix1237548 into lp:webbrowser-app

fix1237548
Merge into trunk

Proposed by Alexandre Abreu on 2013-10-09

Status:

Merged

Approved by:

Alexandre Abreu on 2013-10-10

Approved revision:

379

Merged at revision:

383

Proposed branch:

lp:~abreu-alexandre/webbrowser-app/fix1237548

Merge into:

lp:webbrowser-app

Diff against target:

77 lines (+23/-17)

3 files modified

src/app/Browser.qml (+0/-17)
src/app/webbrowser-app.cpp (+5/-0)
src/app/webbrowser-app.qml (+18/-0)

To merge this branch:

bzr merge lp:~abreu-alexandre/webbrowser-app/fix1237548

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2013-10-10
Olivier Tilloy		2013-10-09	Approve on 2013-10-10
Review via email: mp+190221@code.launchpad.net

Commit message

Fix 1237548: webapps shouldn't expose a org.freedesktop.Application interface on DBUS

Description of the change

Fix 1237548: webapps shouldn't expose a org.freedesktop.Application interface on DBUS

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-09:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/webbrowser-app-ci/480/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2013-10-10:

Why does this code need to move to src/app/webbrowser-app.qml ? (I’m fine with the move, but I’d like to understand if it’s really necessary).

Why do we need a new 'enableUriHandling' property? Can’t we enable the connection to the UriHandler based on the value of the 'webapp' property?

review: Needs Information

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2013-10-10:

> Why does this code need to move to src/app/webbrowser-app.qml ? (I’m fine with
> the move, but I’d like to understand if it’s really necessary).
>
> Why do we need a new 'enableUriHandling' property? Can’t we enable the
> connection to the UriHandler based on the value of the 'webapp' property?

Yeah I guess that w/o context it might not make a lot of sense,

If you look at the UriHandler code, as soon as the element is instanciated, the
DBUS object is created and available. The predicate used to know if we are to create
one or not is if in webapp mode but the flag is propagated only *after* the Browser
element has been created (so not synchronously w/ the component's creation).

Hence the specific flag (false by default), which has been moved to webbrowser-app.qml
since it does not make sense for it to be in browser.qml imo (and would require an
extra unecessary alias just for the sake of reaching the property) which triggers
the creation of the singleton UriHandler when set to true.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2013-10-10:

> If you look at the UriHandler code, as soon as the element is instanciated,
> the
> DBUS object is created and available. The predicate used to know if we are to
> create
> one or not is if in webapp mode but the flag is propagated only *after* the
> Browser
> element has been created (so not synchronously w/ the component's creation).

Yes, but the default value of the webapp property is false, so the UriHandler shouldn’t be instantiated when the Browser component is created.

> Hence the specific flag (false by default), which has been moved to
> webbrowser-app.qml
> since it does not make sense for it to be in browser.qml imo (and would
> require an
> extra unecessary alias just for the sake of reaching the property) which
> triggers
> the creation of the singleton UriHandler when set to true.

I think that a specific flag is not absolutely needed, still I think your proposal makes the code easier to read, so let’s go for it!

Note that the check "if ( ! webapp)" in the onOpened handler is not needed any longer, please remove it.

review: Needs Fixing

lp:~abreu-alexandre/webbrowser-app/fix1237548 updated on 2013-10-10

379. By Alexandre Abreu on 2013-10-10: remove leftover webapp test

Revision history for this message

Olivier Tilloy (osomon) wrote on 2013-10-10:

Looks good now, thanks!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-10:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/webbrowser-app-ci/488/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-10:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/webbrowser-app-ci/487/rebuild

review: Needs Fixing (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/app/Browser.qml'
 --- src/app/Browser.qml	2013-10-07 19:01:57 +0000
 +++ src/app/Browser.qml	2013-10-10 13:54:54 +0000
@@ -495,23 +495,6 @@
+         }
+     }
--    // Handle runtime requests to open urls as defined
--    // by the freedesktop application dbus interface's open
--    // method for DBUS application activation:
--    // http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus
--    // The dispatch on the org.freedesktop.Application if is done per appId at the
--    // url-dispatcher/upstart level.
--    Connections {
--        target: UriHandler
--        onOpened: {
--            if ( ! webapp) {
--                for (var i = 0; i < uris.length; ++i) {
--                    newTab(uris[i], i == uris.length - 1);
--                }
--            }
--        }
--    }
--
      function isRunningAsANamedWebapp() {
          return browser.webappName && typeof(browser.webappName) === 'string' && browser.webappName.length != 0
+     }
 === modified file 'src/app/webbrowser-app.cpp'
 --- src/app/webbrowser-app.cpp	2013-09-25 15:33:02 +0000
 +++ src/app/webbrowser-app.cpp	2013-10-10 13:54:54 +0000
@@ -136,6 +136,11 @@
      browser->setProperty("webapp", m_arguments->webapp());
      browser->setProperty("webappName", m_arguments->webappName());
++    if ( ! m_arguments->webapp())
++    {
++        browser->setProperty("enableUriHandling", true);
++    }
++
      CommandLineParser::ChromeElementFlags chromeFlags = m_arguments->chromeFlags();
      if (chromeFlags != 0)
+     {
 === modified file 'src/app/webbrowser-app.qml'
 --- src/app/webbrowser-app.qml	2013-09-24 21:46:50 +0000
 +++ src/app/webbrowser-app.qml	2013-10-10 13:54:54 +0000
@@ -35,6 +35,8 @@
      property alias webappName: browser.webappName
      property alias webappModelSearchPath: browser.webappModelSearchPath
++    property bool enableUriHandling: false
++
      contentOrientation: browser.screenOrientation
      width: 800
@@ -63,4 +65,20 @@
      function newTab(url, setCurrent) {
          return browser.newTab(url, setCurrent)
+     }
++
++    // Handle runtime requests to open urls as defined
++    // by the freedesktop application dbus interface's open
++    // method for DBUS application activation:
++    // http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus
++    // The dispatch on the org.freedesktop.Application if is done per appId at the
++    // url-dispatcher/upstart level.
++    Connections {
++        target: enableUriHandling ? UriHandler : null
++        onOpened: {
++            for (var i = 0; i < uris.length; ++i) {
++                newTab(uris[i], i == uris.length - 1);
++            }
++        }
++    }
++
+ }