REGRESSION (r177494): -webkit-mask-image: with data URI fails on non-local files

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 247007 [details]
Testcase

r177494 broke -webkit-mask-image: with data URIs for non-local files.

[Simon Fraser (smfr)]

rdar://problem/19738198

[Dirk Schulze]

(In reply to comment #0)
> Created attachment 247007 [details]
> Testcase
> 
> r177494 broke -webkit-mask-image: with data URIs for non-local files.

What do you mean with "for non-local"? I know that we added tests to confirm that data URIs are not broken. Did you break down the issue to r177494?

Question to the test, does masking itself not work or is the image not loaded correctly? If I interpret the test correctly then the former.

[Simon Fraser (smfr)]

(In reply to comment #2)
> (In reply to comment #0)
> > Created attachment 247007 [details]
> > Testcase
> > 
> > r177494 broke -webkit-mask-image: with data URIs for non-local files.
> What do you mean with "for non-local"?

We don't do same-origin testing for file: urls, so to see the bug you have to load from an http(s) url.

> I know that we added tests to confirm
> that data URIs are not broken. Did you break down the issue to r177494?

Yes, it was blamed to that revision.

> Question to the test, does masking itself not work or is the image not
> loaded correctly? If I interpret the test correctly then the former.

The masking doesn't work (second square with little arrows is missing in a nightly).

[Simon Fraser (smfr)]

Created attachment 247128 [details]
provisional patch, needs test

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


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

[Dirk Schulze]

Comment on attachment 247128 [details]
provisional patch, needs test

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

> Source/WebCore/css/CSSParser.cpp:9584
> +        if (protocolIs(value->string, "data"))

I need to ask Radu here as well, but is special casing "data" going to be future proof? Are there other things to consider?

[Simon Fraser (smfr)]

Dirk, any update?

[Radu Stavila]

Comment on attachment 247128 [details]
provisional patch, needs test

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

I think this should be ok. Thanks for taking care of this, Simon.

> Source/WebCore/css/CSSParser.cpp:9587
> +            resourceValue = CSSPrimitiveValue::create(completeURL(value->string));

This might also need the CSSPrimitiveValue::CSS_URI second parameter.

[Simon Fraser (smfr)]

This also broke mobile twitter:
https://mobile.twitter.com/Paul2eD/status/580757901377048577

[Simon Fraser (smfr)]

Created attachment 251196 [details]
Patch

[Dirk Schulze]

Comment on attachment 251196 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 251196 [details]
Patch

Clearing flags on attachment: 251196

Committed r183053: <http://trac.webkit.org/changeset/183053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.