D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R856:9c16dbae9f6c: Implement Web Share API through Purpose 
(authored by broulik).

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=66236=66363

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol, ognarb
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik updated this revision to Diff 66236.

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=65650=66236

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol, ognarb
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Carl Schwan]

ognarb added a comment.


  I will try to find time to test it today

INLINE COMMENTS

> content-script.js:24
> +return '--4xxx-yxxx-'.replace(/[xy]/g, 
> function(c) {
> +var r = Math.random()*16|0, v = c == 'x' ? r : (r&0x3|0x8);
> +return v.toString(16);

var -> const

> content-script.js:148
>  
> -// we give our transfer div a "random id" for privacy
> -// from 
> https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript
> -var mediaSessionsTransferDivId 
> ='--4xxx-yxxx-'.replace(/[xy]/g, function(c) {
> -var r = Math.random()*16|0, v = c == 'x' ? r : (r&0x3|0x8);
> -return v.toString(16);
> -});
> +var mediaSessionsTransferDivId = generateGuid();
>  

var -> let

> content-script.js:852
> +//
> +var purposeTransferDivId = generateGuid();
> +var purposeTransferClassName = "p" + purposeTransferDivId.replace(/-/g, "");

var -> let

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol, ognarb
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Fabian Vogt]

fvogt accepted this revision.
fvogt added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> broulik wrote in purposeplugin.cpp:86
> `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?

Just call the argument `int errorCode` then, one variablename less

> purposeplugin.cpp:115
> +QJsonArray urls;
> +if (!urlString.isEmpty()) {
> +urls.append(urlString);

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?

> purposeplugin.h:57
> +int m_pendingReplySerial = -1;
> +
> +};

Whitespace

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol, ognarb
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik updated this revision to Diff 65650.
broulik added a comment.


  - Register navigator.share only if host supports it
  - Use custom event instead of `window.postMessage`
  - Drop unused KNotifications dependency

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=63809=65650

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  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.

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> purposeplugin.cpp:122
> +urls.append(text);
> +showShareMenu(shareJson, QStringLiteral("text/plain"));
> +return {};

Doesn't use `urls` now?

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik updated this revision to Diff 63809.
broulik added a comment.


  - Reset to -1
  - Use urls again

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=63808=63809

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik updated this revision to Diff 63808.
broulik added a comment.


  - Address comments

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=63716=63808

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> fvogt wrote in purposeplugin.cpp:144
> No response. IMO this should be handled in the plugin manager though, as 
> discussed on that diff.

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

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> fvogt wrote in content-script.js:837
> Where is that documented?

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

> 4. Security and privacy considerations
> 
>   […]
> 
>   *Due to the capabilities of the API surface, navigator.share() is available 
> only in secure contexts (such as https:// schemes).

> fvogt wrote in purposeplugin.cpp:86
> Now we have `error`, `errorCode` and `errorMessage`?

`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?

> fvogt wrote in purposeplugin.cpp:153
> 0 is a valid serial...

:)

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> content-script.js:22
> +// from 
> https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript
> +function guid() {
> +return '--4xxx-yxxx-'.replace(/[xy]/g, 
> function(c) {

`generateGuid`, otherwise it looks like a getter

> content-script.js:837
> +
> +// navigator.share must only be defined in secure (https) context
> +if (!window.isSecureContext) {

Where is that documented?

> purposeplugin.cpp:43
> +{
> +delete m_menu;
> +m_menu = nullptr;

Just call `onUnload()`?

> purposeplugin.cpp:47
> +
> +bool PurposePlugin::onLoad()
> +{

Can remove this, parent class does that already.

> purposeplugin.cpp:86
> +sendPendingReply(false, {
> +{QStringLiteral("error"), QStringLiteral("CANCELED")}
> +});

Now we have `error`, `errorCode` and `errorMessage`?

> purposeplugin.cpp:131
> +
> +if (!text.isEmpty()) { // share text only
> +showShareMenu(shareJson, QStringLiteral("text/plain"));

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.

> purposeplugin.cpp:144
> +
> +return {};
> +}

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

> purposeplugin.cpp:153
> +sendReply(m_pendingReplySerial, reply);
> +m_pendingReplySerial = 0;
> +}

0 is a valid serial...

> purposeplugin.h:57
> +
> +Purpose::Menu *m_menu = nullptr;
> +QNetworkAccessManager *m_manager = nullptr;

`std::unique_ptr`?

> purposeplugin.h:58
> +Purpose::Menu *m_menu = nullptr;
> +QNetworkAccessManager *m_manager = nullptr;
> +QPointer m_mimeReply;

Unused?

> purposeplugin.h:59
> +QNetworkAccessManager *m_manager = nullptr;
> +QPointer m_mimeReply;
> +

Unused?

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> apol wrote in purposeplugin.cpp:127
> shouldn't this be a data: url or something like that? urls should only have 
> urls

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());
  }

> apol wrote in purposeplugin.cpp:161
> Passing application/octet-stream should work, maybe it's a bug in Purpose.

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

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> purposeplugin.cpp:127
> +if (!text.isEmpty()) {
> +urls.append(text);
> +}

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

> purposeplugin.cpp:161
> +if (!mimeType.isEmpty() && mimeType != 
> QLatin1String("application/octet-stream")) {
> +shareData.insert(QStringLiteral("mimeType"), mimeType);
> +} else {

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

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik updated this revision to Diff 63716.
broulik added a comment.


  - Remove leftover testing code

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23151?vs=63715=63716

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23151: Implement Web Share API through Purpose

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, fvogt, davidedmundson, nicolasfella, apol.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION 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
  
  F7246859: Screenshot_20190814_131126.png 

  F7246860: Screenshot_20190814_131148.png 

  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.

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D23151

AFFECTED FILES
  CMakeLists.txt
  extension/_locales/en/messages.json
  extension/constants.js
  extension/content-script.js
  extension/extension-purpose.js
  extension/manifest.json
  extension/options.html
  host/CMakeLists.txt
  host/main.cpp
  host/purposeplugin.cpp
  host/purposeplugin.h

To: broulik, #plasma, fvogt, davidedmundson, nicolasfella, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart