Add canShare function for Web Share API v2

• Component: New Bugs
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

Add canShare function for Web Share API v2

[Nikos Mouchtaris]

Created attachment 390281 [details]
Patch

[Tim Horton]

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).

[Nikos Mouchtaris]

Created attachment 390300 [details]
Patch

[Tim Horton]

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.

[Nikos Mouchtaris]

Created attachment 390600 [details]
Patch

[Nikos Mouchtaris]

Created attachment 390610 [details]
Patch

[Tim Horton]

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?

[Nikos Mouchtaris]

Created attachment 390658 [details]
Patch

[Tim Horton]

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"

[Nikos Mouchtaris]

Created attachment 391437 [details]
Patch

[Nikos Mouchtaris]

Created attachment 391553 [details]
Patch

[Nikos Mouchtaris]

Created attachment 391562 [details]
Patch

[Tim Horton]

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).

[Nikos Mouchtaris]

Created attachment 391583 [details]
Patch

[Nikos Mouchtaris]

Created attachment 391588 [details]
Patch

[Tim Horton]

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.

[Nikos Mouchtaris]

Created attachment 391604 [details]
Patch

[Nikos Mouchtaris]

Created attachment 391605 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391605 [details]
Patch

Clearing flags on attachment: 391605

Committed r257289: <https://trac.webkit.org/changeset/257289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59748246>

[Maciej Stachowiak]

The title says add the canShare API

[Tim Horton]

(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

[Maciej Stachowiak]

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