Bug 171100 - Implement the Web Share API
Summary: Implement the Web Share API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 189170
  Show dependency treegraph
 
Reported: 2017-04-20 22:24 PDT by Rick Byers
Modified: 2018-09-05 14:59 PDT (History)
28 users (show)

See Also:


Attachments
Patch (83.11 KB, patch)
2018-08-20 12:16 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (69.89 KB, patch)
2018-08-21 14:09 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (72.40 KB, patch)
2018-08-23 17:27 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (70.90 KB, patch)
2018-08-24 10:54 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (71.53 KB, patch)
2018-08-24 12:13 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.96 MB, application/zip)
2018-08-24 13:40 PDT, Build Bot
no flags Details
Patch (79.88 KB, patch)
2018-08-24 17:30 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.38 MB, application/zip)
2018-08-24 20:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.88 MB, application/zip)
2018-08-24 20:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.04 MB, application/zip)
2018-08-24 21:24 PDT, Build Bot
no flags Details
Patch (79.85 KB, patch)
2018-08-27 09:43 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.50 MB, application/zip)
2018-08-27 10:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.97 MB, application/zip)
2018-08-27 11:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.04 MB, application/zip)
2018-08-27 11:40 PDT, Build Bot
no flags Details
Patch (86.72 KB, patch)
2018-08-27 15:58 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (87.08 KB, patch)
2018-08-27 17:12 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (85.90 KB, patch)
2018-08-28 09:41 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (85.89 KB, patch)
2018-08-28 11:55 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (85.97 KB, patch)
2018-08-28 13:31 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Patch (85.97 KB, patch)
2018-08-28 15:28 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.27 MB, application/zip)
2018-08-28 17:36 PDT, Build Bot
no flags Details
Patch (88.38 KB, patch)
2018-08-28 17:40 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (3.07 MB, application/zip)
2018-08-28 19:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.31 MB, application/zip)
2018-08-28 19:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (17.30 MB, application/zip)
2018-08-28 19:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-08-28 21:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.27 MB, application/zip)
2018-08-28 21:47 PDT, Build Bot
no flags Details
Patch (86.58 KB, patch)
2018-08-29 15:51 PDT, Olivia Barnett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 2017-04-20 22:24:27 PDT
Web Share is a proposal to add an API to the web for sharing text, URLs and images to an arbitrary destination of the user's choice:
navigator.share({title: 'Example Page', url: 'https://example.com'});

Explainer: https://github.com/WICG/web-share/blob/master/docs/explainer.md
Draft interface: https://github.com/WICG/web-share/blob/master/docs/interface.md
Issues for questions, feedback, etc.: https://github.com/WICG/web-share/issues

This has been available behind an origin trial in Chrome since 55: https://www.chromestatus.com/feature/5668769141620736
I believe there is interest in shipping sometime soon in Chrome, but we'd love to get feedback from other potential implementors before we're locked into the API.
Comment 1 Rick Byers 2017-04-20 22:26:20 PDT
Also relevant blog entry: https://developers.google.com/web/updates/2016/10/navigator-share
Comment 2 Dean Jackson 2017-04-20 22:29:06 PDT
This is interesting. I'll add it to our status page.
Comment 3 Radar WebKit Bug Importer 2017-04-20 22:30:25 PDT
<rdar://problem/31751734>
Comment 4 Olivia Barnett 2018-08-20 12:16:22 PDT
Created attachment 347525 [details]
Patch
Comment 5 Tim Horton 2018-08-20 12:25:20 PDT
Large things:

1) Needs a test. Or probably a few.
2) Needs a RuntimeEnabledFeature switch. Or some other kind of switch?
3) Needs to be rebased on trunk, the patch doesn't apply.
4) Probably requires an email to webkit-dev noting that you're implementing this (see https://webkit.org/feature-policy/).
5) Need to update the feature status JSON file (Source/WebCore/features.json IIRC)
Comment 6 Simon Fraser (smfr) 2018-08-20 12:29:57 PDT
Comment on attachment 347525 [details]
Patch

Please include an update to Source/WebCore/features.json
Comment 7 Tim Horton 2018-08-20 14:38:42 PDT
Comment on attachment 347525 [details]
Patch

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

> Source/WebKit/UIProcess/WebShareSheetResultListenerProxy.cp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Get rid of this file. Also, it's 2018

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:52
> +- (void)presentWithParameters:(NSArray*)parameters resultListener:(WebKit::WebShareSheetResultListenerProxy*)listener completionHandler:(WTF::CompletionHandler<void(bool)>&&)completionHandler;

ObjC stars go on the right (NSArray *). The C++ one is right.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:94
> +- (void)presentWithParameters:(NSArray*)parameters resultListener:(WebKit::WebShareSheetResultListenerProxy*)listener completionHandler:(WTF::CompletionHandler<void(bool)>&&)completionHandler

You don't need WebShareSheetResultListener. Get rid of it everywhere. You just use the completion block mechanism instead, and don't have any API.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1576
> +    RefPtr<WebShareSheetResultListener> m_activeShareSheetResultListener;

You don't need WebShareSheetResultListener

> Source/WebKit/WebProcess/WebPage/WebPage.h:1740
> +    HashMap<uint64_t, WTF::Function<void(bool granted)>> m_shareSheetResponseCallbackMap;

I don't think "granted" is the right word.
Comment 8 Tim Horton 2018-08-20 14:47:11 PDT
Comment on attachment 347525 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:3
> + *

Please clean up the '#pragma mark's and FileUpload related things in this file.
Comment 9 Wenson Hsieh 2018-08-20 15:05:42 PDT
Comment on attachment 347525 [details]
Patch

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

Overall, this looks great! A few minor comments:

> Source/WebCore/loader/EmptyClients.cpp:440
> +void EmptyChromeClient::runShareSheet(Frame&, ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&&)

Nit - the WTF:: here too.

> Source/WebCore/loader/EmptyClients.h:140
> +    void runShareSheet(Frame&, ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&&) final;

Nit - you shouldn't need an explicit WTF:: here.

> Source/WebKit/UIProcess/WebShareSheetResultListenerProxy.cp:49
> +void WebOpenPanelResultListenerProxy::chooseFiles(const Vector<WTF::String>& filenames, const String& displayString, const API::Data* iconImageData)

Nit - WTF:: here too.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4647
> +    NSMutableArray *shareDataArray = [[NSMutableArray alloc] init];

Nit - it looks like this leaks? You could consider changing this to:

auto shareDataArray = adoptNS([[NSMutableArray alloc] init]);

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4670
> +    ASSERT(_shareSheet.get() == shareSheet);

Nit - I don't think the .get() is necessary here.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:58
> +}

Nit - extra {}

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:68
> +    WKContentView *_view;

You could consider making this a WeakObjCPtr if WKShareSheet can outlive its WKContentView.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:72
> +#pragma clang diagnostic push

I don't think you need these 3 #pragma's

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:85
> +- (void)dealloc

Nit - I don't think you need to override -dealloc here.

> Source/WebKit/WebProcess/WebPage/WebPage.h:175
> +struct ShareData;

Hm...you shouldn't need to forward declare this if you're already including <WebCore/ShareData.h> above.

> Source/WebKit/WebProcess/WebPage/WebShareSheetResultListener.h:44
> +    void disconnectFromPage() { m_page = 0; }

m_page = nullptr;

If WebPage always outlives its WebShareSheetResultListener, I would suggest using a reference; otherwise, if this can potentially outlive its WebPage, you might want to consider holding a WeakPtr.

> Source/WebKitLegacy/ios/WebCoreSupport/WebChromeClientIOS.mm:160
> +void WebChromeClientIOS::runShareSheet(Frame&, ShareDataWithParsedURL&,  CompletionHandler<void(bool)>&&)

Nit - extra space before CompletionHandler
Comment 10 Tim Horton 2018-08-20 17:36:12 PDT
I think you need to add a line to messages.py’s headers_for_type()’s special_cases pointing the message generator to ShareData.h for ShareDataWithParsedURL. Not sure how you got away without that locally!
Comment 11 Tim Horton 2018-08-20 21:41:50 PDT
Comment on attachment 347525 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4644
> +    _shareSheet = adoptNS([[WKShareSheet alloc] initWithView:self]);

I think most of this logic should be inside WKShareSheet; hand ShareDataWithParsedURL all the way in there and do the conversion to ObjC types there.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4648
> +    if ([data.shareData.title length] > 0)

This is weird, because you’re going from WTFString to NSString just to check the length. WTFString has .isEmpty(), probably better to just use that.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4654
> +    if (data.url.isNull())

I think you meant to have a ! here :P
Comment 12 Tim Horton 2018-08-20 21:53:09 PDT
Comment on attachment 347525 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:101
> +    auto completionHandlerBlock = BlockPtr<void((NSString *activityType, BOOL completed, NSArray *returnedItems, NSError *activityError))>::fromCallable([completionHandler = WTFMove(completionHandler), shareSheet = self](NSString *activityType, BOOL completed, NSArray *returnedItems, NSError *activityError) mutable {

You don’t need to name all the parameters in the BlockPtr type definition (I’m pretty sure).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:108
> +    [self _presentFullscreenViewController:_shareSheetViewController.get() animated:NO];

We definitely want an animated presentation.
Comment 13 Olivia Barnett 2018-08-21 14:09:40 PDT
Created attachment 347690 [details]
Patch
Comment 14 Simon Fraser (smfr) 2018-08-21 14:35:49 PDT
Comment on attachment 347690 [details]
Patch

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

Not a full review.

> Source/WebCore/page/Navigator.cpp:102
> +    if (m_frame->page()) {

Use an early return to avoid indenting all the rest of the code.

> Source/WebCore/page/Navigator.cpp:110
> +        if (!url.isValid())
> +            promise->reject(TypeError);

You should return here, right?

> Source/WebCore/page/Navigator.cpp:123
> +            return;

No need for the return.

> Source/WebKit/Shared/WebPreferences.yaml:763
> +  humanReadableName: "Web share"

Can someone read this and figure out what the feature is about?

> Source/WebKit/Shared/WebPreferences.yaml:764
> +  humanReadableDescription: "Web share API"

Web Share API?
Comment 15 Tim Horton 2018-08-21 22:17:50 PDT
Needs to be rebased against master again
Comment 16 Tim Horton 2018-08-21 22:40:46 PDT
Comment on attachment 347690 [details]
Patch

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

> Source/WebCore/page/Chrome.h:61
> +struct ShareDataWithParsedURL;

This should be sorted to the right place (alphabetically).

> Source/WebCore/page/ChromeClient.h:106
> +struct ShareDataWithParsedURL;

Sorting.

> Source/WebCore/page/Navigator.cpp:117
> +        m_frame->page()->chrome().runShareSheet(*m_frame, shareData, [documentReference = makeWeakPtr(m_frame->document()), promise = WTFMove(promise)] (bool completed) mutable {

Why do you capture the document? Also what's the 'mutable' about? (there might be a good reason?)

> Source/WebCore/page/Navigator.cpp:122
> +            promise->reject(Exception { AbortError, "Aborted by AbortSignal."_s });

Where did the weird string "Aborted by AbortSignal" come from?

> Source/WebKit/Scripts/webkit/messages.py:385
> +        'WebCore::ShareData': ['<WebCore/ShareData.h>'],

This one is not needed, this is just for overrides where the left and right hand side don't match.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:500
> +// Defaults to false

Is this comment a lie? (it is)

> Source/WebKit/UIProcess/WebPageProxy.h:185
> +struct ShareData;

sooooooooooooooort

> Source/WebKit/UIProcess/WebPageProxy.h:1481
> +    void runShareSheet(uint64_t frameID, const WebCore::ShareDataWithParsedURL&,  uint64_t contextID);

You've got one extra space before the uint64_t

Also I think we should rename this to callbackID everywhere (it's the ... ID... of the... callback. it doesn't provide any context). Ideally it would also have a typedef instead of being a uint64_t everywhere.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:66
> +#import <WebCore/ShareData.h>

Sort (should go with other WebCore includes). Not sure how the stylebot didn't complain about this one, since this is totally something it knows about (perhaps unlike the others)

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:28
> +#import <UIKit/UIViewController.h>

It's slightly unusual to import a single UIKit header, IMO, but maybe it's OK. <UIKit/UIKit.h> is a safe bet, though.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:37
> +class TextStream;

Why?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:50
> +@property (nonatomic, assign) id <WKShareSheetDelegate> delegate;

I would put properties after all the methods. Especially it feels weird if init isn't first. Also I think I'd separate these differently:

-init

-present
-dismiss

@properties

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:76
> +        [shareDataArray addObject:(NSString*)data.shareData.title];

Lots of ObjC stars on the wrong side (should be (NSString *)) throughout this function.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:107
> +    [[UIViewController _viewControllerForFullScreenPresentationFromView:_view.get().get()] dismissViewControllerAnimated:YES completion:nil];

.getAutoreleased() returns an autoreleased raw pointer, so you can avoid this weird .get().get() (the first one returns a very-shortlived-and-unnecessary RetainPtr).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:127
> +    [self _dismissDisplayAnimated:NO];

How do we ever get here with an existing presented VC?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:129
> +    _presentationViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view.get().get()];

Ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3694
> +    

A few places you're adding unnecessary whitespace. I think maybe from when you had the listener.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6043
> +static uint64_t nextShareSheetContextId()

s/context/callback, like I said above

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6094
> +    ASSERT(callback);

I don't think there's any need for this assert, we're just about to crash if it's null, so there's no point crashing one line earlier only in debug builds :) No idea why the person above did that.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1076
> +    void runShareSheet(WebCore::Frame&, uint64_t frameID, WebCore::ShareDataWithParsedURL&, WTF::CompletionHandler<void(bool)>&& callback);

Why does this take a WebCore::Frame? it doesn't look like you need it for anything.

ACTUALLY if I follow it through, you don't need the frameID for anything either, right? I think these are vestiges of the code you copied.  This is the fun part where after growing for a while, your patch collapses to be smaller :)

> Source/WebKit/WebProcess/WebPage/WebPage.h:1735
> +    void runShareSheetResponse(bool wasCompleted, uint64_t contextId);

This is definitely not the right place for a function. WebKit style puts all member variables below all member functions.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3016
> +- (BOOL)webShareEnabled

I don't think there's any need for WebKitLegacy SPI to turn this on/off. I don't think we'll likely implement for WebKitLegacy.
Comment 17 Tim Horton 2018-08-21 23:34:23 PDT
Comment on attachment 347690 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:75
> +    if (!data.shareData.title.isEmpty())

I think it's probably wrong to put the title directly in the share items. Looking at the spec, it seems like 'text' and 'url' are the things you're sharing, and title is sort of metadata *about* the thing you're sharing. So I think we want to be a little smarter here. We have some options:

1. We could just drop the title on the floor. The spec says this is OK.
2. We could use the weird _title SPI on NSURL to smush the title into the URL, if we have one. Not sure any clients will care, but we could consider smartening some of them up.
3. If you only have a title, and no text nor URL-to-smush-into, you could use the title as the text.

I think a combination of 2 and 3 is probably the "smartest" for now, but would also be OK with 1 for the first go-around. But what we have right now is probably not workable (mostly because the share sheet generally has to pick *one* of the items to share, and it would be a shame if it shared the title instead of the more-useful URL or text).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:82
> +    UIActivityViewController *shareSheetController = [[UIActivityViewController alloc] initWithActivityItems:shareDataArray.get() applicationActivities:nil];

Please put the adoptNS here, sink it into a local RetainPtr, and then assign to the instance variable later. Otherwise this gets everyone's leak spidey-sense tingling.
Comment 18 Olivia Barnett 2018-08-23 17:27:02 PDT
Created attachment 347979 [details]
Patch
Comment 19 Build Bot 2018-08-23 17:28:54 PDT
Attachment 347979 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Tim Horton 2018-08-23 18:09:12 PDT
Comment on attachment 347979 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

Except for "No new tests", this is quite close to being ready!

> Source/WebCore/page/Navigator.cpp:108
> +        

unnecessary newline

> Source/WebCore/page/Navigator.cpp:130
> +        promise->reject(Exception { AbortError, "Share sheet cancelled, rejecting promise with abort."_s });

I don't think we should use the word "share sheet" in shared code, that's kind of an iOSism. Maybe just remove the word "sheet"? I'm not sure. Also not totally sure you need to describe that you're rejecting the promise in the promise rejection, but follow whatever you see other people doing.

> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:575
> +bool PageClientImpl::handleRunShareSheet(WebPageProxy*, const ShareDataWithParsedURL& shareData, WTF::CompletionHandler<void(bool)>&& completionHandler)

Let's just call it showShareSheet here too!

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:37
> +namespace WTF {
> +}

??

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:46
> +namespace API {
> +}
> +
> +namespace WebKit {
> +}

?????

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:84
> +            url._title = data.shareData.title;

You'll need to import UIKitSPI.h (hopefully it already knows about _title).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:93
> +    NSURL *urlWithTitle = (NSURL *)data.url;
> +    urlWithTitle._title = data.shareData.title;

I think this is not still real. urlWithTitle goes nowhere, and the real code for this is above in the !data.url.isNull() block

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:176
> +#define WebKitWebShareEnabledPreferenceKey @"WebKitWebShareEnabled"

You don't need this.
Comment 21 Olivia Barnett 2018-08-24 10:54:54 PDT
Created attachment 348022 [details]
Patch
Comment 22 Build Bot 2018-08-24 10:58:09 PDT
Attachment 348022 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Tim Horton 2018-08-24 11:20:20 PDT
Comment on attachment 348022 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:45
> +@property (nonatomic, assign) id <WKShareSheetDelegate> delegate;

This should be weak.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:35
> +#import "UIKitSPI.h"

LOL, _title is only in  the WebCore UIKitSPI.h, not the one you included here. So sorry :( I think you'll need to add _title to the WebKit one. (you can grab those five lines from the WebCore one).

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:74
> +    bool arrayContainsElements = false;

Why this instead of just testing [shareDataArray count]? (Either way is fine I guess)

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:137
> +    // May need to clear/dismiss VC?

Get rid of this comment.
Comment 24 Olivia Barnett 2018-08-24 12:13:38 PDT
Created attachment 348033 [details]
Patch
Comment 25 Build Bot 2018-08-24 12:15:29 PDT
Attachment 348033 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Build Bot 2018-08-24 13:40:54 PDT
Comment on attachment 348033 [details]
Patch

Attachment 348033 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8974635

New failing tests:
fast/dom/navigator-detached-no-crash.html
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
imported/w3c/web-platform-tests/web-share/share-without-user-gesture.https.html
imported/w3c/web-platform-tests/web-share/idlharness.https.html
imported/w3c/web-platform-tests/web-share/share-empty.https.html
Comment 27 Build Bot 2018-08-24 13:40:56 PDT
Created attachment 348037 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 28 Tim Horton 2018-08-24 14:10:08 PDT
You'll want to rebaseline the WPTs that became passes, and probably fix things to make the ones that became fails not fails.
Comment 29 Olivia Barnett 2018-08-24 17:30:19 PDT
Created attachment 348062 [details]
Patch
Comment 30 Build Bot 2018-08-24 17:32:02 PDT
Attachment 348062 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Tim Horton 2018-08-24 17:39:02 PDT
Comment on attachment 348062 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

You can totally change this to "No new tests; adjusted expectations for existing tests." and get rid of the OOPS and be happy.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:433
> +@property (nonatomic, retain, setter=_setContainerViews:)
> +NSArray *_containerViews;

What happened here?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:34
> +#import "APIArray.h"
> +#import "APIData.h"
> +#import "APIOpenPanelParameters.h"
> +#import "APIString.h"

I doubt you need these.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:40
> +#import "WKData.h"
> +#import "WKStringCF.h"
> +#import "WKURLCF.h"
> +#import "WebIconUtilities.h"

I doubt you need these.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:42
> +#import <MobileCoreServices/MobileCoreServices.h>

I doubt you need this.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:43
> +#import <WebCore/LocalizedStrings.h>

I doubt you need this.

> LayoutTests/imported/w3c/web-platform-tests/web-share/idlharness.https-expected.txt:3
> -FAIL Navigator interface: operation share(ShareData) assert_own_property: interface prototype object missing non-static operation expected property "share" missing
> +FAIL Navigator interface: operation share(ShareData) assert_equals: property has wrong .length expected 0 but got 1

Need to ask Sam about this.
Comment 32 Build Bot 2018-08-24 20:41:52 PDT
Comment on attachment 348062 [details]
Patch

Attachment 348062 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8978254

New failing tests:
imported/w3c/web-platform-tests/web-share/share-without-user-gesture.https.html
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/idlharness.https.html
imported/w3c/web-platform-tests/web-share/share-empty.https.html
Comment 33 Build Bot 2018-08-24 20:41:54 PDT
Created attachment 348069 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 34 Build Bot 2018-08-24 20:54:37 PDT
Comment on attachment 348062 [details]
Patch

Attachment 348062 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8978290

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
fast/dom/navigator-detached-no-crash.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 35 Build Bot 2018-08-24 20:54:39 PDT
Created attachment 348071 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 36 Build Bot 2018-08-24 21:24:31 PDT
Comment on attachment 348062 [details]
Patch

Attachment 348062 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8978319

New failing tests:
imported/w3c/web-platform-tests/web-share/share-without-user-gesture.https.html
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/idlharness.https.html
imported/w3c/web-platform-tests/web-share/share-empty.https.html
Comment 37 Build Bot 2018-08-24 21:24:33 PDT
Created attachment 348074 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 38 Olivia Barnett 2018-08-27 09:43:03 PDT
Created attachment 348153 [details]
Patch
Comment 39 Build Bot 2018-08-27 09:47:46 PDT
Attachment 348153 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Build Bot 2018-08-27 10:48:56 PDT
Comment on attachment 348153 [details]
Patch

Attachment 348153 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8996788

New failing tests:
imported/w3c/web-platform-tests/web-share/share-without-user-gesture.https.html
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/idlharness.https.html
imported/w3c/web-platform-tests/web-share/share-empty.https.html
Comment 41 Build Bot 2018-08-27 10:48:58 PDT
Created attachment 348163 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 42 Build Bot 2018-08-27 11:18:00 PDT
Comment on attachment 348153 [details]
Patch

Attachment 348153 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8997025

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
fast/dom/navigator-detached-no-crash.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 43 Build Bot 2018-08-27 11:18:03 PDT
Created attachment 348171 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 44 Build Bot 2018-08-27 11:40:23 PDT
Comment on attachment 348153 [details]
Patch

Attachment 348153 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8996925

New failing tests:
imported/w3c/web-platform-tests/web-share/share-without-user-gesture.https.html
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/idlharness.https.html
imported/w3c/web-platform-tests/web-share/share-empty.https.html
Comment 45 Build Bot 2018-08-27 11:40:26 PDT
Created attachment 348178 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 46 Olivia Barnett 2018-08-27 15:58:47 PDT
Created attachment 348225 [details]
Patch
Comment 47 Build Bot 2018-08-27 16:01:52 PDT
Attachment 348225 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Wenson Hsieh 2018-08-27 16:32:07 PDT
Comment on attachment 348225 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests; adjusted expectations for existing tests.

You should mention your new test in fast/events/ios!

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:384
> +- (void)invokeShareSheetWithResolution:(BOOL)resolved WK_API_AVAILABLE(ios(11.3));

This should be WK_API_AVAILABLE(ios(WK_IOS_TBA));

> Source/WebKit/UIProcess/PageClient.h:205
> +    virtual bool showShareSheet(WebPageProxy*, const WebCore::ShareDataWithParsedURL&,  WTF::CompletionHandler<void (bool)>&&) { return false; }

Nit - extra space before WTF::

> Source/WebKit/UIProcess/WebPageProxy.cpp:5254
> +    

Nit - it doesn't look like this extra whitespace change is intended

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:46
> +@property (nonatomic, weak, assign) id <WKShareSheetDelegate> delegate;

Nit - I don't think you need the assign here.

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:40
> +@interface _WKShareSheetItem : NSObject

Hm...what's _WKShareSheetItem used for?

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:80
> +    RetainPtr<UIActivityViewController> shareSheetController = adoptNS([[UIActivityViewController alloc] initWithActivityItems:shareDataArray.get() applicationActivities:nil]);

Nit - s/RetainPtr<UIActivityViewController>/auto/

> Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:88
> +    _shareSheetViewController = shareSheetController;

This can be _shareSheetViewController = WTFMove(shareSheetController);

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6085
> +void WebPage::runShareSheetResponse(bool wasGranted, uint64_t callbackID)

s/uint64_t/ShareSheetCallbackID/ here?

> Source/WebKit/WebProcess/WebPage/WebPage.h:1734
> +    HashMap<uint64_t, WTF::Function<void(bool completed)>> m_shareSheetResponseCallbackMap;

ShareSheetCallbackID here too?

> LayoutTests/fast/events/ios/share.html:20
> +                    navigator.share({ title: "Example Page", url: "url", text: "text"}).then((result) => {

Nit - Indentation is a bit off here too.

Also, a space between "text" and }

> LayoutTests/resources/ui-helper.js:248
> +	const resolveShareSheet = `(() => uiController.invokeShareSheetWithResolution(${resolved}))()`;

Nit - Indentation is a bit off here.
Comment 49 Olivia Barnett 2018-08-27 16:38:50 PDT
(In reply to Wenson Hsieh from comment #48)
> Comment on attachment 348225 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348225&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests; adjusted expectations for existing tests.
> 
> You should mention your new test in fast/events/ios!
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:384
> > +- (void)invokeShareSheetWithResolution:(BOOL)resolved WK_API_AVAILABLE(ios(11.3));
> 
> This should be WK_API_AVAILABLE(ios(WK_IOS_TBA));
> 
> > Source/WebKit/UIProcess/PageClient.h:205
> > +    virtual bool showShareSheet(WebPageProxy*, const WebCore::ShareDataWithParsedURL&,  WTF::CompletionHandler<void (bool)>&&) { return false; }
> 
> Nit - extra space before WTF::
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:5254
> > +    
> 
> Nit - it doesn't look like this extra whitespace change is intended
> 
> > Source/WebKit/UIProcess/ios/forms/WKShareSheet.h:46
> > +@property (nonatomic, weak, assign) id <WKShareSheetDelegate> delegate;
> 
> Nit - I don't think you need the assign here.
> 
> > Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:40
> > +@interface _WKShareSheetItem : NSObject
> 
> Hm...what's _WKShareSheetItem used for?

Ah, nothing.
> 
> > Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:80
> > +    RetainPtr<UIActivityViewController> shareSheetController = adoptNS([[UIActivityViewController alloc] initWithActivityItems:shareDataArray.get() applicationActivities:nil]);
> 
> Nit - s/RetainPtr<UIActivityViewController>/auto/
> 
> > Source/WebKit/UIProcess/ios/forms/WKShareSheet.mm:88
> > +    _shareSheetViewController = shareSheetController;
> 
> This can be _shareSheetViewController = WTFMove(shareSheetController);
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6085
> > +void WebPage::runShareSheetResponse(bool wasGranted, uint64_t callbackID)
> 
> s/uint64_t/ShareSheetCallbackID/ here?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:1734
> > +    HashMap<uint64_t, WTF::Function<void(bool completed)>> m_shareSheetResponseCallbackMap;
> 
> ShareSheetCallbackID here too?
> 
> > LayoutTests/fast/events/ios/share.html:20
> > +                    navigator.share({ title: "Example Page", url: "url", text: "text"}).then((result) => {
> 
> Nit - Indentation is a bit off here too.
> 
> Also, a space between "text" and }
> 
> > LayoutTests/resources/ui-helper.js:248
> > +	const resolveShareSheet = `(() => uiController.invokeShareSheetWithResolution(${resolved}))()`;
> 
> Nit - Indentation is a bit off here.
Comment 50 Tim Horton 2018-08-27 16:57:46 PDT
Comment on attachment 348225 [details]
Patch

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

> Source/WebCore/page/NavigatorShare.idl:15
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

This this the wrong license. Should be 2-clause BSD.
Comment 51 Olivia Barnett 2018-08-27 17:12:25 PDT
Created attachment 348241 [details]
Patch
Comment 52 Build Bot 2018-08-27 17:16:47 PDT
Attachment 348241 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 Wenson Hsieh 2018-08-27 20:04:04 PDT
Comment on attachment 348241 [details]
Patch

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

I think this is looking pretty good. Just some minor comments below:

> Source/WebCore/page/Navigator.cpp:135
> +    return;

Nit - you don't need the return down here.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:500
> +// Defaults to true

It seems like this only defaults to true on iOS?

> Source/WebKit/UIProcess/PageClient.h:205
> +    virtual bool showShareSheet(WebPageProxy*, const WebCore::ShareDataWithParsedURL&, WTF::CompletionHandler<void (bool)>&&) { return false; }

It doesn't seem like the first argument here (WebPageProxy*) is needed.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4593
> +    CompletionHandler<void(bool)> completionHandler = [this, protectedThis = makeRef(*this), callbackID] (bool access) {

I don't think you need to capture this as well as protectedThis; you can just capture protectedThis and then use protectedThis->m_process and protectedThis->m_pageID.

> Source/WebKit/UIProcess/WebPageProxy.h:1479
> +    void runShareSheet(const WebCore::ShareDataWithParsedURL&, uint64_t callbackID);

ShareSheetCallbackID!

> Source/WebKit/WebProcess/WebPage/WebPage.h:1078
> +    void runShareSheetResponse(bool wasCompleted, uint64_t contextId);

ShareSheetCallbackID!

> LayoutTests/resources/ui-helper.js:248
> +	const resolveShareSheet = `(() => uiController.invokeShareSheetWithResolution(${resolved}))()`;

Nit - still missing one level of indentation here.
Comment 54 Wenson Hsieh 2018-08-27 20:12:49 PDT
Comment on attachment 348241 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4593
>> +    CompletionHandler<void(bool)> completionHandler = [this, protectedThis = makeRef(*this), callbackID] (bool access) {
> 
> I don't think you need to capture this as well as protectedThis; you can just capture protectedThis and then use protectedThis->m_process and protectedThis->m_pageID.

Actually, never mind! A quick grep through WebKit shows that this is a pretty common idiom.
Comment 55 Olivia Barnett 2018-08-28 09:02:40 PDT
> > LayoutTests/resources/ui-helper.js:248
> > +	const resolveShareSheet = `(() => uiController.invokeShareSheetWithResolution(${resolved}))()`;
> 
> Nit - still missing one level of indentation here.

Wait I'm confused -- all the examples above (like setTimePickerValue) have the same indentation?
Comment 56 Olivia Barnett 2018-08-28 09:41:08 PDT
Created attachment 348299 [details]
Patch
Comment 57 Build Bot 2018-08-28 09:43:20 PDT
Attachment 348299 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 Tim Horton 2018-08-28 10:09:43 PDT
Comment on attachment 348299 [details]
Patch

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

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:423
> +@property (nonatomic, copy, nullable, setter=_setTitle:) NSString *_title;

Get that `nullable` out of there, it's viral and makes the compiler want more nullable in more places.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:385
> +- (void)invokeShareSheetWithResolution:(BOOL)resolved WK_API_AVAILABLE(ios(WK_IOS_TBA));

I see other things around here don't have leading underscores, but I don't understand it. Wenson, what's up with that?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:67
> +struct ShareDataWithParsedURL;

Sort this up one line.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6074
> +void WebPage::runShareSheet(ShareDataWithParsedURL& shareData, WTF::CompletionHandler<void(bool)>&& callback)

"Run" is weird. How about s/runShareSheet/showShareSheet/.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6085
> +void WebPage::runShareSheetResponse(bool wasGranted, ShareSheetCallbackID callbackID)

"Run" is weird. How about s/runShareSheetResponse/didCompleteShareSheet/ or didDismissShareSheet or something

> LayoutTests/resources/ui-helper.js:268
> +	const resolveShareSheet = `(() => uiController.invokeShareSheetWithResolution(${resolved}))()`;
> +	return new Promise(resolve => testRunner.runUIScript(resolveShareSheet, resolve));

The confusion here between you and Wenson is that you have TABS here. No tabs please (and this is why!!)
Comment 59 Wenson Hsieh 2018-08-28 11:07:22 PDT
Comment on attachment 348299 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:385
>> +- (void)invokeShareSheetWithResolution:(BOOL)resolved WK_API_AVAILABLE(ios(WK_IOS_TBA));
> 
> I see other things around here don't have leading underscores, but I don't understand it. Wenson, what's up with that?

TBH, it seems like there should be leading underscores per WebKit SPI convention...one of us should clean this all up at some point :P
Comment 60 Olivia Barnett 2018-08-28 11:55:17 PDT
Created attachment 348315 [details]
Patch
Comment 61 Build Bot 2018-08-28 11:57:57 PDT
Attachment 348315 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 Tim Horton 2018-08-28 12:44:43 PDT
Comment on attachment 348315 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:500
> +// Defaults to false, true on iOS wk2

This is WebKit2 code, so you can just drop the wk2 (it's also not called wk2 in the API so this is a weird thing to have in an SPI header).

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:385
> +- (void)invokeShareSheetWithResolution:(BOOL)resolved WK_API_AVAILABLE(ios(WK_IOS_TBA));

Still missing the underscore.
Comment 63 Olivia Barnett 2018-08-28 13:31:06 PDT
Created attachment 348332 [details]
Patch
Comment 64 Build Bot 2018-08-28 13:36:34 PDT
Attachment 348332 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 65 Olivia Barnett 2018-08-28 15:28:09 PDT
Created attachment 348351 [details]
Patch
Comment 66 Build Bot 2018-08-28 15:31:56 PDT
Attachment 348351 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Build Bot 2018-08-28 17:36:24 PDT
Comment on attachment 348351 [details]
Patch

Attachment 348351 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9016523

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 68 Build Bot 2018-08-28 17:36:26 PDT
Created attachment 348366 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 69 Olivia Barnett 2018-08-28 17:40:17 PDT
Created attachment 348367 [details]
Patch
Comment 70 Build Bot 2018-08-28 17:44:52 PDT
Attachment 348367 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 71 Build Bot 2018-08-28 19:32:20 PDT
Comment on attachment 348367 [details]
Patch

Attachment 348367 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9018140

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 72 Build Bot 2018-08-28 19:32:23 PDT
Created attachment 348377 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 73 Build Bot 2018-08-28 19:37:25 PDT
Comment on attachment 348367 [details]
Patch

Attachment 348367 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9018379

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 74 Build Bot 2018-08-28 19:37:28 PDT
Created attachment 348378 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 75 Build Bot 2018-08-28 19:41:45 PDT
Comment on attachment 348367 [details]
Patch

Attachment 348367 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9018120

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
Comment 76 Build Bot 2018-08-28 19:41:48 PDT
Created attachment 348379 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 77 Build Bot 2018-08-28 21:03:18 PDT
Comment on attachment 348367 [details]
Patch

Attachment 348367 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9018878

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
imported/w3c/web-platform-tests/web-share/share-securecontext.http.html
Comment 78 Build Bot 2018-08-28 21:03:21 PDT
Created attachment 348386 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 79 Build Bot 2018-08-28 21:47:05 PDT
Comment on attachment 348367 [details]
Patch

Attachment 348367 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9018997

New failing tests:
imported/w3c/web-platform-tests/web-share/share-url-invalid.https.html
Comment 80 Build Bot 2018-08-28 21:47:08 PDT
Created attachment 348391 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 81 Olivia Barnett 2018-08-29 15:51:02 PDT
Created attachment 348442 [details]
Patch
Comment 82 Build Bot 2018-08-29 15:54:52 PDT
Attachment 348442 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:286:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 83 WebKit Commit Bot 2018-08-29 17:20:34 PDT
Comment on attachment 348442 [details]
Patch

Clearing flags on attachment: 348442

Committed r235489: <https://trac.webkit.org/changeset/235489>
Comment 84 WebKit Commit Bot 2018-08-29 17:20:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 86 Tim Horton 2018-08-30 11:59:16 PDT
Super weird that we didn't get that here.
Comment 87 David Kilzer (:ddkilzer) 2018-09-05 14:59:13 PDT
(In reply to WebKit Commit Bot from comment #83)
> Comment on attachment 348442 [details]
> Patch
> 
> Clearing flags on attachment: 348442
> 
> Committed r235489: <https://trac.webkit.org/changeset/235489>

Follow-up fix due to bad WebKit Xcode project merge:

Committed r235710: <https://trac.webkit.org/changeset/235710>