Bug 142475 - [Content Filtering] Add tests
Summary: [Content Filtering] Add tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks: 128858
  Show dependency treegraph
 
Reported: 2015-03-09 02:23 PDT by Andy Estes
Modified: 2015-03-09 16:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (51.83 KB, patch)
2015-03-09 02:56 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (51.85 KB, patch)
2015-03-09 02:59 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (51.73 KB, patch)
2015-03-09 12:19 PDT, Andy Estes
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2015-03-09 02:23:15 PDT
[Content Filtering] Add tests
Comment 1 Andy Estes 2015-03-09 02:56:39 PDT
Created attachment 248236 [details]
Patch
Comment 2 WebKit Commit Bot 2015-03-09 02:58:49 PDT
Attachment 248236 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/MockContentFilterSettings.cpp:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/testing/MockContentFilter.cpp:39:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andy Estes 2015-03-09 02:59:29 PDT
Created attachment 248237 [details]
Patch
Comment 4 WebKit Commit Bot 2015-03-09 03:01:56 PDT
Attachment 248237 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/MockContentFilterSettings.cpp:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/testing/MockContentFilter.cpp:39:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Andy Estes 2015-03-09 12:19:25 PDT
Created attachment 248268 [details]
Patch
Comment 6 WebKit Commit Bot 2015-03-09 12:21:58 PDT
Attachment 248268 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/MockContentFilter.cpp:39:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Andreas Kling 2015-03-09 15:22:05 PDT
Comment on attachment 248268 [details]
Patch

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

r=me

I didn't comment on this earlier, but classes should be marked with the WTF_MAKE_FAST_ALLOCATED macro to ensure they get allocated using WebKit's preferred allocator.

> Source/WebCore/bindings/js/JSMockContentFilterSettingsCustom.cpp:43
> +// Must be kept in sync with values in MockContentFilterSettings.idl.
> +const uint8_t decisionPointAfterResponse = 0;
> +const uint8_t decisionPointAfterAddData = 1;
> +const uint8_t decisionPointAfterFinishedAddingData = 2;
> +const uint8_t decisionAllow = 0;
> +const uint8_t decisionBlock = 1;

It would be better if the bindings generator would spit these out as public static const class members.

> Source/WebCore/testing/MockContentFilter.h:34
> +class MockContentFilter final : public ContentFilter {

WTF_MAKE_NONCOPYABLE?

> Source/WebCore/testing/MockContentFilter.h:40
> +    static std::unique_ptr<MockContentFilter> create(const ResourceResponse&);
> +
> +    explicit MockContentFilter(const ResourceResponse&);

Having both a public constructor and a public construction helper is a bit awkward.
Can we hide the constructor?

> Source/WebCore/testing/MockContentFilterSettings.h:34
> +class MockContentFilterSettings {

WTF_MAKE_NONCOPYABLE?
Comment 8 Andy Estes 2015-03-09 16:03:12 PDT
(In reply to comment #7)
> Comment on attachment 248268 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248268&action=review
> 
> r=me

Thanks!

> 
> I didn't comment on this earlier, but classes should be marked with the
> WTF_MAKE_FAST_ALLOCATED macro to ensure they get allocated using WebKit's
> preferred allocator.

Oops, I'll fix this.

> 
> > Source/WebCore/bindings/js/JSMockContentFilterSettingsCustom.cpp:43
> > +// Must be kept in sync with values in MockContentFilterSettings.idl.
> > +const uint8_t decisionPointAfterResponse = 0;
> > +const uint8_t decisionPointAfterAddData = 1;
> > +const uint8_t decisionPointAfterFinishedAddingData = 2;
> > +const uint8_t decisionAllow = 0;
> > +const uint8_t decisionBlock = 1;
> 
> It would be better if the bindings generator would spit these out as public
> static const class members.

I agree! I filed <https://bugs.webkit.org/show_bug.cgi?id=142512>.

> 
> > Source/WebCore/testing/MockContentFilter.h:34
> > +class MockContentFilter final : public ContentFilter {
> 
> WTF_MAKE_NONCOPYABLE?

Yup.

> 
> > Source/WebCore/testing/MockContentFilter.h:40
> > +    static std::unique_ptr<MockContentFilter> create(const ResourceResponse&);
> > +
> > +    explicit MockContentFilter(const ResourceResponse&);
> 
> Having both a public constructor and a public construction helper is a bit
> awkward.
> Can we hide the constructor?

Agreed. I fixed this by making the ctor private and making std::make_unique a friend.

> 
> > Source/WebCore/testing/MockContentFilterSettings.h:34
> > +class MockContentFilterSettings {
> 
> WTF_MAKE_NONCOPYABLE?

Not quite. I need the assignment operator in order to reset the settings. But I will delete the copy constructor.
Comment 9 Andy Estes 2015-03-09 16:39:32 PDT
Committed r181291: <http://trac.webkit.org/changeset/181291>