Bug 142475 – [Content Filtering] Add tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Content Filtering] Add tests

[Andy Estes]

Created attachment 248236 [details]
Patch

[WebKit Commit Bot]

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.

[Andy Estes]

Created attachment 248237 [details]
Patch

[WebKit Commit Bot]

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.

[Andy Estes]

Created attachment 248268 [details]
Patch

[WebKit Commit Bot]

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.

[Andreas Kling]

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?

[Andy Estes]

(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.

[Andy Estes]

Committed r181291: <http://trac.webkit.org/changeset/181291>