[Content Filtering] Blocked page is not always displayed when it should be
rdar://problem/20211099
*** Bug 142894 has been marked as a duplicate of this bug. ***
Created attachment 250150 [details] Patch
Attachment 250150 [details] did not pass style-queue: ERROR: Source/WebCore/loader/ContentFilter.cpp:74: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/loader/ContentFilter.cpp:158: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:35: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:36: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/cocoa/ContentFilterUnblockHandlerCocoa.mm:68: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 7 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 250150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250150&action=review r=me > Source/WebCore/loader/ContentFilter.cpp:102 > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); I would break this into two assertions so it would display the specific failure. > Source/WebCore/loader/ContentFilter.cpp:109 > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); Ditto. > Source/WebCore/loader/ContentFilter.cpp:116 > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); Ditto. > Source/WebCore/loader/ContentFilter.cpp:173 > + ASSERT(m_state != State::Allowed && m_state != State::Blocked); The same idea also applies here. > Source/WebCore/loader/ContentFilter.cpp:177 > + // Calling m_decisionFunction might delete |this|. Good to know! > Source/WebCore/loader/ContentFilter.h:52 > + ~ContentFilter() override; One of the compilers we support used to barf on "override" on destructors. I don't recall which one. EWS will tell us I suppose. > Source/WebCore/loader/ContentFilter.h:66 > + RefPtr<SharedBuffer> replacementData() const; Since this function will never return a null pointer, we should make it return Ref<SharedBuffer>. Ditto for all of its namesake functions in this patch. > Source/WebCore/loader/ContentFilter.h:95 > + PlatformContentFilter* m_blockingContentFilter; > + State m_state; I would initialize these here instead of doing it in the constructor's initializer list. > Source/WebCore/loader/DocumentLoader.cpp:1585 > + if (!unblocked && frame) Can frame really be non-null here? I honestly have no idea. > Source/WebCore/platform/spi/cocoa/NEFilterSourceSPI.h:71 > + k
(In reply to comment #5) > Comment on attachment 250150 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250150&action=review > > r=me Thanks! > > > Source/WebCore/loader/ContentFilter.cpp:102 > > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); > > I would break this into two assertions so it would display the specific > failure. Ok. > > > Source/WebCore/loader/ContentFilter.cpp:109 > > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); > > Ditto. > > > Source/WebCore/loader/ContentFilter.cpp:116 > > + ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData()); > > Ditto. > > > Source/WebCore/loader/ContentFilter.cpp:173 > > + ASSERT(m_state != State::Allowed && m_state != State::Blocked); > > The same idea also applies here. > > > Source/WebCore/loader/ContentFilter.cpp:177 > > + // Calling m_decisionFunction might delete |this|. > > Good to know! > > > Source/WebCore/loader/ContentFilter.h:52 > > + ~ContentFilter() override; > > One of the compilers we support used to barf on "override" on destructors. I > don't recall which one. EWS will tell us I suppose. Only clang should see this header, so I think we're okay. > > > Source/WebCore/loader/ContentFilter.h:66 > > + RefPtr<SharedBuffer> replacementData() const; > > Since this function will never return a null pointer, we should make it > return Ref<SharedBuffer>. > Ditto for all of its namesake functions in this patch. Ok. > > > Source/WebCore/loader/ContentFilter.h:95 > > + PlatformContentFilter* m_blockingContentFilter; > > + State m_state; > > I would initialize these here instead of doing it in the constructor's > initializer list. Done. > > > Source/WebCore/loader/DocumentLoader.cpp:1585 > > + if (!unblocked && frame) > > Can frame really be non-null here? I honestly have no idea. I think it's possible, but I can move the null check earlier in this function. > > > Source/WebCore/platform/spi/cocoa/NEFilterSourceSPI.h:71 > > + > > k
Committed r182356: <http://trac.webkit.org/changeset/182356>