Handle CSSImageValue URLs in inline styles in templates correctly

Handle CSSImageValue URLs in inline styles in templates correctly

When importing or adopting nodes across Documents, this patch checks for
baseURL mismatches between the two documents and re-resolves URLs for
CSSImageValues in those cases. Without this patch, the URLs are empty
when they enter the Document (since they tried to resolve against an
empty baseURL when they were created).

This requires storing the un-resolved URL inside the CSSImageValue alongside
the resolved URL, and some unfortunately hacky code in Element.cpp to handle
re-resolving.

The resolve-on-adoption behavior matches the behavior recently specced for
<img> elements in HTML (see http://html5.org/tools/web-apps-tracker?from=8508&to=8509)

This does not yet support any other form of url() in CSS (-webkit-image-set
being the most glaring omission). I've left FIXMEs in the code to handle those,
but I fear adding any more hacks in Element.cpp; in order to support this
generally, some serious refactoring in the CSSValue class hierarchy is required.

BUG=229142
R=ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167931

[adamk, 2014-02-18 23:12:14]

Would love y'all's opinion on this approach. The code that's in
Element::didMoveToNewDocument() clearly shouldn't live in that function, but
from chatting with Ojan it might just want to live somewhere nearby in Element.

[adamk, 2014-02-20 01:19:28]

Ping?

On Tue, Feb 18, 2014 at 3:12 PM,  <adamk@chromium.org> wrote:
> Reviewers: esprehn, ojan,
>
> Message:
> Would love y'all's opinion on this approach. The code that's in
> Element::didMoveToNewDocument() clearly shouldn't live in that function, but
> from chatting with Ojan it might just want to live somewhere nearby in
> Element.
>
> Description:
> Handle urls in inline styles in templates correctly
>
> WORK IN PROGRESS
>
> BUG=229142
>
> Please review this at https://codereview.chromium.org/164803002/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+73, -30 lines):
>   A LayoutTests/fast/css/lazy-parse-inline-style.html
>   M Source/core/css/CSSImageValue.h
>   M Source/core/css/CSSImageValue.cpp
>   M Source/core/css/parser/BisonCSSParser-in.cpp
>   M Source/core/dom/Element.cpp
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ojan, 2014-02-20 20:30:43]

This all looks reasonable to me.

[esprehn, 2014-02-20 20:33:53]

Conceptually this looks fine.

[adamk, 2014-02-25 21:12:22]

Okay, here's a real stab at this functionality, PTAL

[ojan, 2014-02-26 03:00:29]

lgtm

https://codereview.chromium.org/164803002/diff/210001/Source/core/css/CSSImag...
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/164803002/diff/210001/Source/core/css/CSSImag...
Source/core/css/CSSImageValue.h:55: template<typename URLResolver> // Avoids a
dependency on Document
templates scare me. Avoiding a Document dependency seems good though.

[adamk, 2014-02-26 17:00:19]

https://codereview.chromium.org/164803002/diff/210001/Source/core/css/CSSImag...
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/164803002/diff/210001/Source/core/css/CSSImag...
Source/core/css/CSSImageValue.h:55: template<typename URLResolver> // Avoids a
dependency on Document
On 2014/02/26 03:00:30, ojan wrote:
> templates scare me. Avoiding a Document dependency seems good though.

This may be overkill, given the other dependencies on Document (since, say,
ResourceFetcher can get you access to it). But it's pretty harmless.

[adamk, 2014-02-26 17:00:24]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/164803002/210001

[esprehn, 2014-02-26 17:08:14]

If rather we didn't use templates like this, it's just asking for abuse
when someone figures out they can use a random other class. We should break
resolution off into a class and use that if you want to break the header
deps.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[adamk, 2014-02-26 17:31:34]

The CQ bit was unchecked by adamk@chromium.org

[adamk, 2014-02-26 17:44:06]

On 2014/02/26 17:08:14, esprehn wrote:
> If rather we didn't use templates like this, it's just asking for abuse
> when someone figures out they can use a random other class. We should break
> resolution off into a class and use that if you want to break the header
> deps.

Removed template usage, now I pass in the whole document (would love to have a
discussion about the tradeoffs here in general, but for this case it's no harm,
since CSSImageValue.cpp already #includes Document).

[adamk, 2014-02-26 17:44:10]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:44:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/164803002/250001

[adamk, 2014-02-26 21:19:41]

Committed patchset #6 manually as r167931 (presubmit successful).