| Summary: | Allow partial application of PlugInAutoStart tables based on timestamp | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ricky Mondello <rmondello> | ||||||||||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | commit-queue, dino, jonlee, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Ricky Mondello
2013-12-17 13:36:20 PST
Created attachment 219465 [details]
First take
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.
Created attachment 219466 [details]
Take 2, with style corrections
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.
Created attachment 219469 [details]
More style corrections
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&". Created attachment 219664 [details]
Addressing Jon Lee’s feedback
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.
Created attachment 219665 [details]
Addressing Jon Lee’s feedback (2)
Created attachment 219779 [details]
Naming cleanup
Comment on attachment 219779 [details] Naming cleanup Clearing flags on attachment: 219779 Committed r160922: <http://trac.webkit.org/changeset/160922> All reviewed patches have been landed. Closing bug. 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 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. Rename PlugInAutoStartProvider’s ...EntriesAddedBeforeTime facility to ...EntriesAddedAfterTime https://bugs.webkit.org/show_bug.cgi?id=126078 |