Bug 125871 - Allow partial application of PlugInAutoStart tables based on timestamp
Summary: Allow partial application of PlugInAutoStart tables based on timestamp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-17 13:36 PST by Ricky Mondello
Modified: 2013-12-20 13:35 PST (History)
4 users (show)

See Also:


Attachments
First take (10.17 KB, patch)
2013-12-17 16:18 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Take 2, with style corrections (10.16 KB, patch)
2013-12-17 16:31 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
More style corrections (10.15 KB, patch)
2013-12-17 16:37 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Addressing Jon Lee’s feedback (9.56 KB, patch)
2013-12-19 11:27 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Addressing Jon Lee’s feedback (2) (9.56 KB, patch)
2013-12-19 11:31 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Naming cleanup (9.94 KB, patch)
2013-12-20 12:11 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ricky Mondello 2013-12-17 13:36:20 PST
Allow clients of the PlugInAutoStart functionality to partially apply a table based on the timestamp of entries. More specifically, clients may find it useful to only apply policies created before a certain date to “roll back” changes to the table.
Comment 1 Ricky Mondello 2013-12-17 16:18:56 PST
Created attachment 219465 [details]
First take
Comment 2 WebKit Commit Bot 2013-12-17 16:25:58 PST
Attachment 219465 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:109:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:116:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:121:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h:59:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebContext.h:270:  The parameter name "dictionary" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ricky Mondello 2013-12-17 16:27:19 PST
<rdar://problem/15684849>
Comment 4 Ricky Mondello 2013-12-17 16:31:28 PST
Created attachment 219466 [details]
Take 2, with style corrections
Comment 5 WebKit Commit Bot 2013-12-17 16:32:56 PST
Attachment 219466 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/UIProcess/WebContext.h:270:  The parameter name "dictionary" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Ricky Mondello 2013-12-17 16:37:19 PST
Created attachment 219469 [details]
More style corrections
Comment 7 Jon Lee 2013-12-18 00:59:54 PST
Comment on attachment 219469 [details]
More style corrections

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

> Source/WebKit2/ChangeLog:18
> +            with a lambda that lets policies created after a certain time pass.

errrr-- "before time" in the function name, but let policies pass "after a certain time"?

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:394
> +void WKContextApplyPlugInAutoStartOriginsAddedBeforeTime(WKContextRef contextRef, WKDictionaryRef dictionaryRef, double time)

"Apply" seems like the wrong verb to use here. The method isn't taking the provided dictionary and amended the existing one with it, which is what I think of when I see "apply". Maybe something more along the lines of "SetPlugInAutoStartOrigins{BeforeTime,UsingTimeCutoff}"?

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:52
> +static double timeExpirationTimeWasGenerated(double expirationTime)

awkward naming. "originationTimeFromExpiration"?

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:121
> +void PlugInAutoStartProvider::setAutoStartOriginsTableWithItemsPassingTest(ImmutableDictionary const& table, std::function<bool(double timestamp)> predicate)

Wouldn't it be better for the predicate to be named properly, for readability in the code? Like "isExpirationTimeAcceptable"?
Also seems that proper style is "const ImmutableDictionary&".
Comment 8 Ricky Mondello 2013-12-19 11:27:53 PST
Created attachment 219664 [details]
Addressing Jon Lee’s feedback
Comment 9 WebKit Commit Bot 2013-12-19 11:29:15 PST
Attachment 219664 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp', u'Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.h:68:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Ricky Mondello 2013-12-19 11:31:14 PST
Created attachment 219665 [details]
Addressing Jon Lee’s feedback (2)
Comment 11 Ricky Mondello 2013-12-20 12:11:25 PST
Created attachment 219779 [details]
Naming cleanup
Comment 12 WebKit Commit Bot 2013-12-20 12:57:25 PST
Comment on attachment 219779 [details]
Naming cleanup

Clearing flags on attachment: 219779

Committed r160922: <http://trac.webkit.org/changeset/160922>
Comment 13 WebKit Commit Bot 2013-12-20 12:57:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Jon Lee 2013-12-20 13:27:17 PST
Comment on attachment 219779 [details]
Naming cleanup

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

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:109
> +void PlugInAutoStartProvider::setAutoStartOriginsFilteringOutEntriesAddedBeforeTime(ImmutableDictionary& table, double time)

Isn't it FilteringInEntriesAddedBeforeTime, or FilteringOutEntriesAddedAfterTime?
Comment 15 Ricky Mondello 2013-12-20 13:32:02 PST
Comment on attachment 219779 [details]
Naming cleanup

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

>> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:109
>> +void PlugInAutoStartProvider::setAutoStartOriginsFilteringOutEntriesAddedBeforeTime(ImmutableDictionary& table, double time)
> 
> Isn't it FilteringInEntriesAddedBeforeTime, or FilteringOutEntriesAddedAfterTime?

Yes. New bug/patch coming. Thank you for catching this.
Comment 16 Ricky Mondello 2013-12-20 13:35:00 PST
Rename PlugInAutoStartProvider’s ...EntriesAddedBeforeTime facility to ...EntriesAddedAfterTime
https://bugs.webkit.org/show_bug.cgi?id=126078