-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
0f2ed2c
to
99cccb7
Compare
Filed Gecko bug and implemented it. Will send patch to Mozilla. |
Working on updating the tests also. |
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
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
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
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
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}
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}
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}
Thank you - this addresses #159 well. Text explaining why to use canShare looks good to me. |
…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
…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
…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
…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
…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
Pinging again 👋 I'd like get this merge so we can move the spec to CR. |
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😭
There was a problem hiding this comment.
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
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. |
PR preview is back 🎉 |
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. |
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 I'll also make another attempt to update the Gecko patch. |
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
Closes #108
Closes #159
For normative changes, the following tasks have been completed:
Implementation commitment:
Preview | Diff