Implement Web Share API through Purpose
ClosedPublic

Authored by broulik on Aug 14 2019, 11:13 AM.

Details

Summary

This implements Web Share API Level 1 [1] through Purpose and also adds a generic "Share..." context menu entry.
It can be tested on [2].

[1] https://w3c.github.io/web-share/
[2] https://w3c.github.io/web-share/demos/share.html

Test Plan
  • Successfully shared a tweet via E-Mail on Twitter's mobile site (unfortunately the Desktop site doesn't make use of it)
  • Sending to KDE Connect works
  • Verified that calling it without user interaction isn't possible



I couldn't figure out how to pass both a title and contents simultaneously to it since it seems to abuse the "urls" field for all kinds of random purposes like email *body*.

The browser context menu also gets a "Share..." entry where you can share whatever page, link, image, video, etc you're pointing at. Unfortunately, when both this and KDE Connect Devices are present, you now get a "Plasma Browser Integration" sub menu with both entries in it.

Diff Detail

Repository
R856 Plasma Browser Integration
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Aug 14 2019, 11:13 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 14 2019, 11:13 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Aug 14 2019, 11:13 AM
broulik updated this revision to Diff 63716.Aug 14 2019, 12:00 PM
  • Remove leftover testing code
apol added inline comments.Aug 14 2019, 11:10 PM
host/purposeplugin.cpp
128

shouldn't this be a data: url or something like that? urls should only have urls

162

Passing application/octet-stream should work, maybe it's a bug in Purpose.

broulik added inline comments.Aug 15 2019, 7:02 AM
host/purposeplugin.cpp
128

I was wondering the same but the email plugin does something very strange:

if (url.isLocalFile()) {
    query.addQueryItem(QStringLiteral("attachment"), att.toString());
} else {
    query.addQueryItem(QStringLiteral("body"), att.toString());
}
162

I don't see any special-casing for default mimetype in Purpose.

fvogt requested changes to this revision.Aug 15 2019, 8:21 AM

AFAICT this won't work on wayland and will also break the browser's native implementation once that actually exists.

extension/content-script.js
22

generateGuid, otherwise it looks like a getter

832

Where is that documented?

host/purposeplugin.cpp
44

Just call onUnload()?

48

Can remove this, parent class does that already.

87

Now we have error, errorCode and errorMessage?

132

Merge this with the same if above and put the KIO::mimetype query into the other if. Currently it would break if both url and text are empty.

145

No response. IMO this should be handled in the plugin manager though, as discussed on that diff.

154

0 is a valid serial...

host/purposeplugin.h
58

std::unique_ptr?

59

Unused?

60

Unused?

This revision now requires changes to proceed.Aug 15 2019, 8:21 AM
broulik added inline comments.Aug 15 2019, 2:06 PM
extension/content-script.js
832

https://w3c.github.io/web-share/#security-and-privacy-considerations

  1. Security and privacy considerations

    […]

    *Due to the capabilities of the API surface, navigator.share() is available only in secure contexts (such as https:// schemes).
host/purposeplugin.cpp
87

errorCode and errorMessage are forwarded from the Purpose plugin, error is our own. I could change errorCode to return a string from us then I guess. Or I hardcode a magic 1 here which is ERR_USER_CANCELED which Purpose uses when canceled but that won't help for the busy case?

154

:)

broulik added inline comments.Aug 15 2019, 2:07 PM
host/purposeplugin.cpp
145

Yeah, I will clean that up everywhere separate of this patch as to not entangle it even further.

broulik updated this revision to Diff 63808.Aug 15 2019, 2:44 PM
  • Address comments
fvogt requested changes to this revision.Aug 15 2019, 2:49 PM
fvogt added inline comments.
host/purposeplugin.cpp
123

Doesn't use urls now?

This revision now requires changes to proceed.Aug 15 2019, 2:49 PM
broulik updated this revision to Diff 63809.Aug 15 2019, 2:49 PM
  • Reset to -1
  • Use urls again
broulik planned changes to this revision.Aug 22 2019, 4:39 PM

It should only add the navigator.share stuff if the host side is supported and loaded, in case you run old host with new extension.
The setting will be correctly hidden but since it is enabled by default it will still add the then defunct JS bits.

broulik updated this revision to Diff 65650.Sep 8 2019, 1:22 PM
  • Register navigator.share only if host supports it
  • Use custom event instead of window.postMessage
  • Drop unused KNotifications dependency
fvogt accepted this revision.Sep 16 2019, 8:24 AM
fvogt added inline comments.
host/purposeplugin.cpp
87

Just call the argument int errorCode then, one variablename less

116

This whole if chain looks a bit weird - not having any items in the urls list results in an error at the end, so why not bail out early and then refactor out all urls.isEmpty() checks?

host/purposeplugin.h
58

Whitespace

This revision is now accepted and ready to land.Sep 16 2019, 8:24 AM

I will try to find time to test it today

extension/content-script.js
24

var -> const

143

var -> let

821

var -> let

broulik updated this revision to Diff 66236.Sep 16 2019, 3:49 PM
This revision was automatically updated to reflect the committed changes.