Bug 196465 - Add SaveRestoreScope, which can be used to save/restore state without direct access to member variables
Summary: Add SaveRestoreScope, which can be used to save/restore state without direct ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-01 14:49 PDT by Simon Fraser (smfr)
Modified: 2019-04-03 20:34 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.70 KB, patch)
2019-04-01 14:51 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (9.20 KB, patch)
2019-04-01 15:42 PDT, Simon Fraser (smfr)
simon.fraser: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-04-01 14:49:29 PDT
Add SaveRestoreScope, which can be used to save/restore state without direct access to member variables
Comment 1 Simon Fraser (smfr) 2019-04-01 14:51:31 PDT
Created attachment 366426 [details]
Patch
Comment 2 Simon Fraser (smfr) 2019-04-01 14:52:23 PDT
I considered pointers to member functions, but it gets pretty ugly, and you'd end up with template instantiation per class/getter/setter combination.
Comment 3 EWS Watchlist 2019-04-01 14:54:09 PDT
Attachment 366426 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/SetForScope.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/SetForScope.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/SetForScope.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:70:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:83:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:86:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alex Christensen 2019-04-01 14:55:47 PDT
Comment on attachment 366426 [details]
Patch

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

Could you use this in WebKit in a place where SetForScope is insufficient?  Otherwise we have unused code in WTF, which is undesirable.

> Source/WTF/wtf/SetForScope.h:93
> +    WTF::Function<void (T)> m_setterFunction;

No space after void.
WTF:: is not needed.
Comment 5 Simon Fraser (smfr) 2019-04-01 15:42:30 PDT
Created attachment 366432 [details]
Patch
Comment 6 EWS Watchlist 2019-04-01 15:46:12 PDT
Attachment 366432 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6888:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:70:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:83:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SetForScope.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:4260:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Saam Barati 2019-04-01 23:17:45 PDT
Comment on attachment 366432 [details]
Patch

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

> Source/WTF/wtf/SetForScope.h:71
> +class SaveRestoreScope {

Can we just make SetForScope also have a constructor that handles this? I prefer that name more than this

> Source/WTF/wtf/SetForScope.h:73
> +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction)

Why not “const Function<T()>&” for the type of getter function?

> Source/WTF/wtf/SetForScope.h:79
> +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction, T newValue)

Ditto
Comment 8 Saam Barati 2019-04-01 23:19:41 PDT
Comment on attachment 366432 [details]
Patch

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

> Source/WTF/wtf/SetForScope.h:88
> +        m_setterFunction(m_originalValue);

WTFMove? Or perhaps forwarding semantics?
Comment 9 Simon Fraser (smfr) 2019-04-03 20:34:17 PDT
(In reply to Saam Barati from comment #7)
> Comment on attachment 366432 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366432&action=review
> 
> > Source/WTF/wtf/SetForScope.h:71
> > +class SaveRestoreScope {
> 
> Can we just make SetForScope also have a constructor that handles this? I
> prefer that name more than this

That would bloat SetForScope with WTF::Function, but maybe that's OK. This is also a little more loosey-goosey than SetForScope because those two functors could do anything.


> > Source/WTF/wtf/SetForScope.h:73
> > +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction)
> 
> Why not “const Function<T()>&” for the type of getter function?

I never know when to pass Functions by reference or RVF.

> > Source/WTF/wtf/SetForScope.h:79
> > +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction, T newValue)

> WTFMove? Or perhaps forwarding semantics?

Sure, I guess? Does that add any overhead when used with small POD types?