Bug 167532 - IconLoadingClient API doesn't work asynchronously
Summary: IconLoadingClient API doesn't work asynchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-27 15:55 PST by Brady Eidson
Modified: 2017-01-28 08:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.83 KB, patch)
2017-01-27 16:00 PST, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-01-27 15:55:56 PST
IconLoadingClient API doesn't work asynchronously

This is because the WTF::Function is not properly moved into the objective-C block, and in fact it can't be; Blocks must be copyable.

Making it a std::function instead works.
Comment 1 Brady Eidson 2017-01-27 15:57:48 PST
<rdar://problem/30243429>
Comment 2 Brady Eidson 2017-01-27 16:00:04 PST
Created attachment 299972 [details]
Patch
Comment 3 WebKit Commit Bot 2017-01-27 16:02:56 PST
Attachment 299972 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/IconLoadingDelegate.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/IconLoadingDelegate.mm:74:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:6731:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:874:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:874:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/APIIconLoadingClient.h:42:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIIconLoadingClient.h:42:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brady Eidson 2017-01-27 16:13:42 PST
Alex gave in-person r+ with a small change.
https://trac.webkit.org/changeset/211310
Comment 5 Sam Weinig 2017-01-27 20:42:59 PST
I don't understand what the issue is here. In my testing, WTF::Function works fine with blocks. Can you give an example where it doesn't work?
Comment 6 Alex Christensen 2017-01-27 22:24:28 PST
Comment on attachment 299972 [details]
Patch

It doesn't work with blocks that are copied or not called immediately.
Comment 7 Sam Weinig 2017-01-28 08:57:43 PST
(In reply to comment #6)
> Comment on attachment 299972 [details]
> Patch
> 
> It doesn't work with blocks that are copied or not called immediately.

Can you give me an example? I made some unit tests that all seem to work when copying a block, but perhaps I am missing something.

e.g. this works:

TEST(FunctionBlock, BlockCopyAsync)
{
    WTF::initializeMainThread();

    typedef void (^BlockType)();

    __block bool value = false;
    BlockType block = ^{ value = true; };
    
    Function<void()> function((BlockType)[block copy]);

    callOnMainThread(WTFMove(function));
    
    TestWebKitAPI::Util::run(&value);
    ASSERT_TRUE(value);
}