Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add canShare() method #177

Merged
merged 19 commits into from
Aug 20, 2021
Merged

Add canShare() method #177

merged 19 commits into from
Aug 20, 2021

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Sep 18, 2020

Closes #108
Closes #159

For normative changes, the following tasks have been completed:

Implementation commitment:

  • WebKit - Safari 14
  • Chromium - 66?
  • Gecko

Preview | Diff

@marcoscaceres
Copy link
Member Author

Filed Gecko bug and implemented it. Will send patch to Mozilla.

@marcoscaceres
Copy link
Member Author

Working on updating the tests also.

index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member Author

@scheib, I added some rationale text for canShare for #159 ... would you mind taking a look and let me know what you think?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
Spec:
https://w3c.github.io/web-share/#share-method

If a share() request is active when share() is called again, the new
share request fails immediately.
w3c/web-share#113

In content shell, the OS-integration for the share service is not
present. Instead of crashing in tests, we report a not implemented
error.

WPT web-share/share-sharePromise-internal-slot.https.html now passes
(It previously crashed.)

Protocols other that http and https are no longer supported by
share() or canShare().
w3c/web-share#174

canShare is being discussed in
w3c/web-share#177

Bug: 1002337, 1002514, 1131755
Change-Id: I4ec9f6eb03373fd5c6db1881df906a8df36ca4ff
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
Spec:
https://w3c.github.io/web-share/#share-method

If a share() request is active when share() is called again, the new
share request fails immediately.
w3c/web-share#113

In content shell, the OS-integration for the share service is not
present. Instead of crashing in tests, we report a not implemented
error.

WPT web-share/share-sharePromise-internal-slot.https.html now passes -
it previously crashed.
w3c/web-share#183 improves consistency between
the test and the spec.

Protocols other that http and https are no longer supported by
share() or canShare().
w3c/web-share#174

canShare is being discussed in
w3c/web-share#177

Bug: 1002337, 1002514, 1131755
Change-Id: I4ec9f6eb03373fd5c6db1881df906a8df36ca4ff
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 24, 2020
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
@scheib
Copy link

scheib commented Sep 25, 2020

Thank you - this addresses #159 well. Text explaining why to use canShare looks good to me.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 26, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 28, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Glen Robertson <glenrobchromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Cr-Commit-Position: refs/heads/master{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755

UltraBlame original commit: 527027b783378db7e3c1fcdd908e718799400989
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this pull request Oct 1, 2020
…nd https, a=testonly

Automatic update from web-platform-tests
Web Share: restrict URL scheme to http and https

We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}

--

wpt-commits: a28408e23e7cb1e4e8dc070445a51fc2f2d9a4e6
wpt-pr: 25755
@marcoscaceres
Copy link
Member Author

Pinging again 👋 I'd like get this merge so we can move the spec to CR.

@w3c w3c deleted a comment Aug 7, 2021
@w3c w3c deleted a comment Aug 7, 2021
index.html Outdated
The choice to use string literals instead of an [=enumeration=] is
deliberate. Doing so allows implementers to gracefully support future
{{ShareData}} member values, while also giving the user agent the
ability to disable sharing those members if needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this mention whatwg/webidl#893 or whatwg/webidl#491?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. If something changes in WebIDL, then it should be adopted. whatwg/webidl#491 should be closed anyway - it's no longer relevant.

@saschanaz
Copy link
Member

How can we get diff preview as other w3c repositories have?

@@ -270,6 +268,110 @@ <h4>
or bypassing the UI if there is only a single share target.
</div>
</section>
<section>
<h3>
`canShare(data)` method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be marked as historical if we think this is a broken approach?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we are not going to ship an alternative - but I don't think anyone plans to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we are not going to ship an alternative

Are going to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes... there is no plans for any engine to ship anything different. Let's just ship this and be done for now. If we need a new method, we can add it when there is demand for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Gecko would want to ship anything proven to be broken as a sole solution 🤔

cc @annevk, context: #108 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok, but if we can't shift the other implementers then we need to make a process call here (i.e., Mozilla can "formally object" and we figure it out from there).

We have two engines that already ship .canShare() already (and have now for over a year).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends on real world usage of canShare.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, that too... however, we (you and I) are kinda limited in that we can't actually get that data.

We are at an impasse here :( without input from other implementers willing to change (or even respond to issues), I don't know what we can do 😭

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, that too... however, we (you and I) are kinda limited in that we can't actually get that data.

https://chromestatus.com/metrics/feature/timeline/popularity/2737

@marcoscaceres
Copy link
Member Author

How can we get diff preview as other w3c repositories have?

We might not have had .pr_preview enabled when the PR was created a long time ago... I'll see if I can get it working.

@marcoscaceres
Copy link
Member Author

PR preview is back 🎉

@marcoscaceres
Copy link
Member Author

Ok, sent a PR to remove "tentative" from the canShare() tests. The tests all pass in WebKit, except 1 that I need to look into.

@marcoscaceres
Copy link
Member Author

I'm going ahead and merging this as this matches two shipping implementations, and this is been sitting for nearly a year. I'm happy to continue to revise canShare() and even send patches to various engines if we want to improve it in various ways - so long as things remain backwards compatible.

I'll also make another attempt to update the Gecko patch.

@marcoscaceres marcoscaceres merged commit 68494b7 into main Aug 20, 2021
@marcoscaceres marcoscaceres deleted the canShare branch August 20, 2021 01:48
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
We now follow the recent spec change limiting the permitted scheme
for shared urls to http and https - see
w3c/web-share#173
w3c/web-share#174
w3c/web-share#177

We make an exception if the page performing the share it itself loaded
from a different scheme (e.g. file) - in that case we allow the same
scheme to be used for the shared url.

Bug: 1131755
Change-Id: I6abf0f9acd40ef79ec49379314e2ef3a81d3467e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425977
Commit-Queue: Eric Willigers <[email protected]>
Reviewed-by: Glen Robertson <[email protected]>
Auto-Submit: Eric Willigers <[email protected]>
Cr-Commit-Position: refs/heads/master@{#810180}
GitOrigin-RevId: 060b7f1b2de01048a934bc4aca41973edaf4d12c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

canShare() is not future compatible Explain why canShare exists at definition.
4 participants