Bug 143055 – Apply ContentExtension actions after redirects

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Apply ContentExtension actions after redirects

rdar://problem/20062613

[Brady Eidson]

Created attachment 249422 [details]
WIP for posterity

This is probably a pretty final-ish patch, but I'm working on layout tests to cover both main resources and subresources.

[Brady Eidson]

Created attachment 249425 [details]
Patch v1

[WebKit Commit Bot]

Attachment 249425 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/contentextensions/ContentExtensionsBackend.h:68:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 249425 [details]
Patch v1

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

> LayoutTests/http/tests/contentextensions/subresource-redirect-blocked.html:1
> +This page has an image.<br>

Don't you need subresource-redirect-blocked-expected.txt?

> Source/WebCore/contentextensions/ContentExtensionsBackend.h:68
> +    void processContentExtensionRulesForLoad(ResourceRequest& request, ResourceType, DocumentLoader& initiatingDocumentLoader);

request

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:48
> +    m_resourceType = ResourceType::PlugInStream;

This should be m_resourceType(ResourceType::PlugInStream)

> Tools/ChangeLog:4
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

forgot

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 249425 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249425&action=review
> 
> > LayoutTests/http/tests/contentextensions/subresource-redirect-blocked.html:1
> > +This page has an image.<br>
> 
> Don't you need subresource-redirect-blocked-expected.txt?

Yup, forgot to git-add it

> 
> > Source/WebCore/contentextensions/ContentExtensionsBackend.h:68
> > +    void processContentExtensionRulesForLoad(ResourceRequest& request, ResourceType, DocumentLoader& initiatingDocumentLoader);
> 
> request

Yup, gone.

> 
> > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:48
> > +    m_resourceType = ResourceType::PlugInStream;
> 
> This should be m_resourceType(ResourceType::PlugInStream)

Can't be - m_resourceType belongs to the base class, can't be included in subclass initialized lists

> 
> > Tools/ChangeLog:4
> > +        Need a short description (OOPS!).
> > +        Need the bug URL (OOPS!).
> 
> forgot

I sure did!

[Brady Eidson]

Created attachment 249431 [details]
Patch v2

[Alex Christensen]

Comment on attachment 249431 [details]
Patch v2

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

Looks good except these small things:

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:48
> +    m_resourceType = ResourceType::PlugInStream;

m_resourceType(ResourceType::PlugInStream)

> Source/WebCore/loader/ResourceLoadInfo.h:44
> +    PlugInStream = 0x0100,

You also need to change LoadType::FirstParty LoadType::ThirdParty so this doesn't overlap.  Maybe these should be made one enum so that is more obvious.

[Build Bot]

Comment on attachment 249431 [details]
Patch v2

Attachment 249431 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5576653375799296

New failing tests:
svg/as-image/svg-image-with-svg-data-uri.html
svg/as-image/svg-image-with-data-uri-from-canvas.html
svg/as-image/svg-image-with-data-uri-reloading.html
svg/as-image/svg-image-with-data-uri-background.html
svg/as-image/svg-image-with-data-uri-use-data-uri.svg
svg/as-image/img-zoom-svg-stylesheet.html
svg/as-image/svg-image-with-data-uri-images-disabled.html

[Build Bot]

Created attachment 249434 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alex Christensen]

(In reply to comment #8)
> Comment on attachment 249431 [details]
> Patch v2
> 
> Attachment 249431 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5576653375799296
> 
> New failing tests:
> svg/as-image/svg-image-with-svg-data-uri.html
> svg/as-image/svg-image-with-data-uri-from-canvas.html
> svg/as-image/svg-image-with-data-uri-reloading.html
> svg/as-image/svg-image-with-data-uri-background.html
> svg/as-image/svg-image-with-data-uri-use-data-uri.svg
> svg/as-image/img-zoom-svg-stylesheet.html
> svg/as-image/svg-image-with-data-uri-images-disabled.html

I knew you didn't run all tests on all platforms!  Please check these before landing.

[Build Bot]

Comment on attachment 249431 [details]
Patch v2

Attachment 249431 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5283148732563456

New failing tests:
svg/as-image/svg-image-with-svg-data-uri.html
svg/as-image/svg-image-with-data-uri-from-canvas.html
svg/as-image/svg-image-with-data-uri-reloading.html
svg/as-image/svg-image-with-data-uri-background.html
svg/as-image/svg-image-with-data-uri-use-data-uri.svg
svg/as-image/img-zoom-svg-stylesheet.html
svg/as-image/svg-image-with-data-uri-images-disabled.html

[Build Bot]

Created attachment 249436 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brady Eidson]

Will definitely look at tests before landing.

[Brady Eidson]

(In reply to comment #7)
> Comment on attachment 249431 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249431&action=review
> 
> Looks good except these small things:
> 
> > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:48
> > +    m_resourceType = ResourceType::PlugInStream;
> 
> m_resourceType(ResourceType::PlugInStream)

As mentioned above - That's initializer syntax, but this can't be an initializer, as the variable belongs to the super class.

> 
> > Source/WebCore/loader/ResourceLoadInfo.h:44
> > +    PlugInStream = 0x0100,
> 
> You also need to change LoadType::FirstParty LoadType::ThirdParty so this
> doesn't overlap.  Maybe these should be made one enum so that is more
> obvious.

That was *not* obvious. Will change.

[Brady Eidson]

http://trac.webkit.org/changeset/182016

[Brady Eidson]

The layout test failures were a missing null check on UserContentController that has now been added.