Bug 173660 - Allow constructing a WTF:Function from a function pointer
Summary: Allow constructing a WTF:Function from a function pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-21 10:02 PDT by Chris Dumez
Modified: 2017-06-21 18:32 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.04 KB, patch)
2017-06-21 10:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2017-06-21 10:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2017-06-21 10:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2017-06-21 12:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2017-06-21 14:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-06-21 10:02:42 PDT
Allow constructing a WTF:Function from a function pointer.
Comment 1 Chris Dumez 2017-06-21 10:05:26 PDT
Created attachment 313526 [details]
Patch
Comment 2 Chris Dumez 2017-06-21 10:46:08 PDT
Created attachment 313530 [details]
Patch
Comment 3 Chris Dumez 2017-06-21 10:58:17 PDT
Created attachment 313531 [details]
Patch
Comment 4 Darin Adler 2017-06-21 11:26:33 PDT
Comment on attachment 313531 [details]
Patch

Looks great. I remember one example from one of the std::function patches that was taking a bool that I don’t see you changing back here.
Comment 5 Darin Adler 2017-06-21 11:26:59 PDT
Could add a unit test for this too since we have Function.cpp now.
Comment 6 Chris Dumez 2017-06-21 11:27:48 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 313531 [details]
> Patch
> 
> Looks great. I remember one example from one of the std::function patches
> that was taking a bool that I don’t see you changing back here.

networkStateChanged. Found it. Will fix as well, thanks.
Comment 7 Chris Dumez 2017-06-21 11:28:03 PDT
(In reply to Darin Adler from comment #5)
> Could add a unit test for this too since we have Function.cpp now.

Good idea. Will do.
Comment 8 Chris Dumez 2017-06-21 12:06:34 PDT
Created attachment 313538 [details]
Patch
Comment 9 Darin Adler 2017-06-21 13:44:29 PDT
Comment on attachment 313538 [details]
Patch

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

> Source/WebCore/page/Page.cpp:284
> -        networkStateNotifier().addNetworkStateChangeListener([] (bool isOnLine) { networkStateChanged(isOnLine); });
> +        networkStateNotifier().addNetworkStateChangeListener(networkStateChanged);

My fault you added this, but it’s not compiling on Windows, maybe due to the different flavors of function pointer that exist on Windows.

> Source/WebCore/workers/Worker.cpp:62
> -        networkStateNotifier().addNetworkStateChangeListener([] (bool isOnLine) { networkStateChanged(isOnLine); });
> +        networkStateNotifier().addNetworkStateChangeListener(networkStateChanged);

Same here.
Comment 10 Chris Dumez 2017-06-21 14:37:28 PDT
Created attachment 313552 [details]
Patch
Comment 11 Build Bot 2017-06-21 14:39:14 PDT
Attachment 313552 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/Worker.cpp:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/Page.cpp:284:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Chris Dumez 2017-06-21 15:25:09 PDT
Comment on attachment 313552 [details]
Patch

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

> Source/WebCore/page/Page.cpp:284
> +        networkStateNotifier().addNetworkStateChangeListener(WTF::Function<void (bool)> { networkStateChanged });

Somehow Windows requires the explicit WTF::Function<void (bool)> { } here.
Comment 13 WebKit Commit Bot 2017-06-21 15:53:17 PDT
Comment on attachment 313552 [details]
Patch

Clearing flags on attachment: 313552

Committed r218660: <http://trac.webkit.org/changeset/218660>
Comment 14 WebKit Commit Bot 2017-06-21 15:53:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryan Haddad 2017-06-21 17:41:18 PDT
(In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 313552 [details]
> Patch
> 
> Clearing flags on attachment: 313552
> 
> Committed r218660: <http://trac.webkit.org/changeset/218660>

This change broke the Windows Debug build:
https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/2230
Comment 16 Chris Dumez 2017-06-21 18:22:08 PDT
(In reply to Ryan Haddad from comment #15)
> (In reply to WebKit Commit Bot from comment #13)
> > Comment on attachment 313552 [details]
> > Patch
> > 
> > Clearing flags on attachment: 313552
> > 
> > Committed r218660: <http://trac.webkit.org/changeset/218660>
> 
> This change broke the Windows Debug build:
> https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> 2230

Will fix now, thanks.
Comment 17 Chris Dumez 2017-06-21 18:26:18 PDT
(In reply to Chris Dumez from comment #16)
> (In reply to Ryan Haddad from comment #15)
> > (In reply to WebKit Commit Bot from comment #13)
> > > Comment on attachment 313552 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 313552
> > > 
> > > Committed r218660: <http://trac.webkit.org/changeset/218660>
> > 
> > This change broke the Windows Debug build:
> > https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> > 2230
> 
> Will fix now, thanks.

<http://trac.webkit.org/changeset/218671>
Comment 18 Chris Dumez 2017-06-21 18:32:22 PDT
Comment on attachment 313552 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:284
>> +        networkStateNotifier().addNetworkStateChangeListener(WTF::Function<void (bool)> { networkStateChanged });
> 
> Somehow Windows requires the explicit WTF::Function<void (bool)> { } here.

I really need to find a way to make Window build without this. This is ugly :/