Bug 207491

Summary: Add canShare function for Web Share API v2
Product: WebKit Reporter: Nikos Mouchtaris <nikosm>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, benjamin, cdumez, cmarcelo, commit-queue, dbates, eoconnor, esprehn+autocc, ews-watchlist, kondapallykalyan, megan_gardner, mjs, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208191
Bug Depends on:    
Bug Blocks: 198606    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Nikos Mouchtaris 2020-02-10 12:05:00 PST
Add canShare function for Web Share API v2
Comment 1 Nikos Mouchtaris 2020-02-10 12:10:51 PST
Created attachment 390281 [details]
Patch
Comment 2 Tim Horton 2020-02-10 12:53:48 PST
Comment on attachment 390281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390281&action=review

> Source/WebCore/page/Navigator.cpp:51
> +#define SUPPORT_FILE_SHARE 0
> +#define SUPPORT(FIELD) (SUPPORT_##FIELD == 1)

No idea what this is, but it doesn't look like something we do in WebKit. I'd encourage you to check out WTF's PlatformEnable*.h for how we usually do feature flags.

> Source/WebCore/page/ShareData.h:27
> +#include "File.h"

Can this be a forward declaration instead of an include (we try to limit header includes in headers to avoid the C++ build time explosion).
Comment 3 Nikos Mouchtaris 2020-02-10 14:57:08 PST
Created attachment 390300 [details]
Patch
Comment 4 Tim Horton 2020-02-12 11:38:04 PST
Comment on attachment 390300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390300&action=review

> Source/WTF/wtf/PlatformEnable.h:723
> +#define ENABLE_FILE_SHARE 0

I wonder if we should just leave this out for now and introduce it in the next patch

> Source/WebCore/page/Navigator.cpp:114
> +    if (data.title.isNull() && data.url.isNull() && data.text.isNull()) {

Is the logic here to-spec? If you have a title and a file, we'll say `true`, right? If people are using feature detection by way of canShare, we will then be lying to them that we can share files.

> Source/WebCore/page/ShareData.h:36
> +    Vector<RefPtr<File>> files;

Is this change web-exposed? I hope people are using canShare() instead of detecting it, if it is. Not your problem, though.

> LayoutTests/imported/w3c/ChangeLog:19
> +        * web-platform-tests/web-share/share-without-user-gesture.https.html:

The test is failing on the macOS bots. You'll need to fix that before landing.
Comment 5 Nikos Mouchtaris 2020-02-12 17:40:14 PST
Created attachment 390600 [details]
Patch
Comment 6 Nikos Mouchtaris 2020-02-12 19:22:36 PST
Created attachment 390610 [details]
Patch
Comment 7 Tim Horton 2020-02-12 23:44:21 PST
Comment on attachment 390610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390610&action=review

> Source/WebCore/page/Navigator.cpp:118
> +#if ENABLE(FILE_SHARE)
> +            return true;
> +#endif

This should have gone away too :)

> LayoutTests/TestExpectations:732
> +# Web platform tests are broken for web-share

"Broken" is probably not quite sufficient explanation. Why are they broken? Are they all broken for the same reason?
Comment 8 Nikos Mouchtaris 2020-02-13 09:25:16 PST
Created attachment 390658 [details]
Patch
Comment 9 Tim Horton 2020-02-19 12:56:20 PST
Comment on attachment 390658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390658&action=review

> Source/WebCore/ChangeLog:12
> +

In retrospect we probably need this to have an Experimental Feature flag. See WebPreferences.yaml, anything that's "category: experimental". You're OK without a build time flag, like I said before, but should still be a runtime-enabled experimental feature.

> Source/WebCore/page/Navigator.cpp:116
> +            // TODO: fix for when files fully implemented

Two options:

1) [Preferred] Remove this needless comment.
2) Make this comment a complete sentence, with capitalization and a period at the end (and maybe better grammar).

See https://webkit.org/code-style-guidelines/#comments

> LayoutTests/TestExpectations:736
> +# Web platform tests produce wrong failing results

This could do with a "why" (and ideally a reference to a bugzilla bug about fixing it!) instead of just "hey it's wrong"
Comment 10 Nikos Mouchtaris 2020-02-21 17:31:30 PST
Created attachment 391437 [details]
Patch
Comment 11 Nikos Mouchtaris 2020-02-24 10:24:41 PST
Created attachment 391553 [details]
Patch
Comment 12 Nikos Mouchtaris 2020-02-24 11:36:44 PST
Created attachment 391562 [details]
Patch
Comment 13 Tim Horton 2020-02-24 14:13:14 PST
Comment on attachment 391562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391562&action=review

Almost done!

> Source/WebCore/page/RuntimeEnabledFeatures.h:610
> +    bool m_WebShareFileAPIEnabled { false };

lower-case the leading W

> Source/WebKit/Shared/WebPreferences.yaml:1959
> +WebShareFileAPIEnabled:
> +    type: bool
> +    defaultValue: false
> +    humanReadableName: "Web Share File API"
> +    humanReadableDescription: "Enable sharing of files through Web Share API"
> +    webcoreBinding: RuntimeEnabledFeatures
> +    category: experimental

I think it should be called Web Share API Level 2, since it will cover both canShare and `file`

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:653
> +- (void)setWebShareFileAPIEnabled:(BOOL)flag;

No need for Legacy WebKit SPI, we'll never enable it in Legacy WebKit (we don't even have Level 1 support there).
Comment 14 Nikos Mouchtaris 2020-02-24 14:51:53 PST
Created attachment 391583 [details]
Patch
Comment 15 Nikos Mouchtaris 2020-02-24 15:37:13 PST
Created attachment 391588 [details]
Patch
Comment 16 Tim Horton 2020-02-24 16:19:14 PST
Comment on attachment 391588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391588&action=review

> Source/WebCore/ChangeLog:9
> +        Added files member to share data and canShare function to
> +        navigator.cpp.

You should add some words here (about how canShare will return false if you have any files, etc.). Also, if it makes it easier to write lots of words, you can put them down below in the per-file comments.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:616
> +// Defaults to false
> +WK_EXPORT void WKPreferencesSetWebShareFileAPIEnabled(WKPreferencesRef, bool flag);
> +WK_EXPORT bool WKPreferencesGetWebShareFileAPIEnabled(WKPreferencesRef);

Ditto what I said about legacy webkit: if there's no reason to add C SPI, we shouldn't.
Comment 17 Nikos Mouchtaris 2020-02-24 16:52:51 PST
Created attachment 391604 [details]
Patch
Comment 18 Nikos Mouchtaris 2020-02-24 16:54:45 PST
Created attachment 391605 [details]
Patch
Comment 19 WebKit Commit Bot 2020-02-24 17:22:04 PST
Comment on attachment 391605 [details]
Patch

Clearing flags on attachment: 391605

Committed r257289: <https://trac.webkit.org/changeset/257289>
Comment 20 WebKit Commit Bot 2020-02-24 17:22:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2020-02-24 17:23:15 PST
<rdar://problem/59748246>
Comment 22 Maciej Stachowiak 2020-05-06 01:00:22 PDT
The title says add the canShare API
Comment 23 Tim Horton 2020-05-06 11:21:05 PDT
(In reply to Maciej Stachowiak from comment #22)
> The title says add the canShare API

The title was correct, the file sharing part is over in https://bugs.webkit.org/show_bug.cgi?id=209265
Comment 24 Maciej Stachowiak 2020-05-07 02:14:46 PDT
(In reply to Tim Horton from comment #23)
> (In reply to Maciej Stachowiak from comment #22)
> > The title says add the canShare API
> 
> The title was correct, the file sharing part is over in
> https://bugs.webkit.org/show_bug.cgi?id=209265

The patch in this Radar adds `files` to the IDL for ShareData, was it just stubs?