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

Proposed by Alexandre Abreu
Status: Merged
Approved by: Alexandre Abreu
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
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Olivier Tilloy Approve
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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

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 :

> 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 :

> 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
379. By Alexandre Abreu

remove leftover webapp test

Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
495 }495 }
496 }496 }
497497
498 // Handle runtime requests to open urls as defined
499 // by the freedesktop application dbus interface's open
500 // method for DBUS application activation:
501 // http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus
502 // The dispatch on the org.freedesktop.Application if is done per appId at the
503 // url-dispatcher/upstart level.
504 Connections {
505 target: UriHandler
506 onOpened: {
507 if ( ! webapp) {
508 for (var i = 0; i < uris.length; ++i) {
509 newTab(uris[i], i == uris.length - 1);
510 }
511 }
512 }
513 }
514
515 function isRunningAsANamedWebapp() {498 function isRunningAsANamedWebapp() {
516 return browser.webappName && typeof(browser.webappName) === 'string' && browser.webappName.length != 0499 return browser.webappName && typeof(browser.webappName) === 'string' && browser.webappName.length != 0
517 }500 }
518501
=== 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 @@
136 browser->setProperty("webapp", m_arguments->webapp());136 browser->setProperty("webapp", m_arguments->webapp());
137 browser->setProperty("webappName", m_arguments->webappName());137 browser->setProperty("webappName", m_arguments->webappName());
138138
139 if ( ! m_arguments->webapp())
140 {
141 browser->setProperty("enableUriHandling", true);
142 }
143
139 CommandLineParser::ChromeElementFlags chromeFlags = m_arguments->chromeFlags();144 CommandLineParser::ChromeElementFlags chromeFlags = m_arguments->chromeFlags();
140 if (chromeFlags != 0)145 if (chromeFlags != 0)
141 {146 {
142147
=== 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 @@
35 property alias webappName: browser.webappName35 property alias webappName: browser.webappName
36 property alias webappModelSearchPath: browser.webappModelSearchPath36 property alias webappModelSearchPath: browser.webappModelSearchPath
3737
38 property bool enableUriHandling: false
39
38 contentOrientation: browser.screenOrientation40 contentOrientation: browser.screenOrientation
3941
40 width: 80042 width: 800
@@ -63,4 +65,20 @@
63 function newTab(url, setCurrent) {65 function newTab(url, setCurrent) {
64 return browser.newTab(url, setCurrent)66 return browser.newTab(url, setCurrent)
65 }67 }
68
69 // Handle runtime requests to open urls as defined
70 // by the freedesktop application dbus interface's open
71 // method for DBUS application activation:
72 // http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus
73 // The dispatch on the org.freedesktop.Application if is done per appId at the
74 // url-dispatcher/upstart level.
75 Connections {
76 target: enableUriHandling ? UriHandler : null
77 onOpened: {
78 for (var i = 0; i < uris.length; ++i) {
79 newTab(uris[i], i == uris.length - 1);
80 }
81 }
82 }
83
66}84}

Subscribers

People subscribed via source and target branches

to status/vote changes: