Resolve CSS url(...) non-<image> values against the correct base

Resolve CSS url(...) non-<image> values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315
Review-Url: https://codereview.chromium.org/2625873010
Cr-Commit-Position: refs/heads/master@{#444179}
Committed: https://chromium.googlesource.com/chromium/src/+/db478183363dad5b460070122b991a8302a1f98a

[fs, 2017-01-12 16:09:11]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 16:09:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 17:57:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:57:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2017-01-12 18:00:21]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 18:00:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 19:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 19:59:20]

Dry run: This issue passed the CQ dry run.

[fs, 2017-01-12 20:18:19]

Description was changed from

==========
Resolve url(...)s against the correct base

BUG=405315
==========

to

==========
Resolve CSS url(...) values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315
==========

[fs, 2017-01-12 20:18:19]

fs@opera.com changed reviewers:
+ junov@chromium.org, timloh@chromium.org

[Timothy Loh, 2017-01-13 00:58:45]

On 2017/01/12 20:18:48, fs wrote:

Haven't reviewed this yet (will do it shortly), but the title reminded me of bug
618165 where we resolve url()s in custom property references with an empty base,
so maybe a good idea to explicitly mention filter in the title :)

[fs, 2017-01-13 08:56:08]

Description was changed from

==========
Resolve CSS url(...) values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315
==========

to

==========
Resolve CSS url(...) non-<image> values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315
==========

[fs, 2017-01-13 08:58:37]

On 2017/01/13 at 00:58:45, timloh wrote:
> On 2017/01/12 20:18:48, fs wrote:
> 
> Haven't reviewed this yet (will do it shortly), but the title reminded me of
bug 618165 where we resolve url()s in custom property references with an empty
base, so maybe a good idea to explicitly mention filter in the title :)

I added "non-<image>" to subject, since this technically speaking not limited to
'filter' (it's the only place where it makes any difference ATM though.) I take
it that bug (crbug.com/618165) is essentially the TODO in
CSSSyntaxDescriptor.cpp? (which I duplicated)

[Timothy Loh, 2017-01-15 23:30:53]

lgtm

On 2017/01/13 08:58:37, fs wrote:
> On 2017/01/13 at 00:58:45, timloh wrote:
> > On 2017/01/12 20:18:48, fs wrote:
> > 
> > Haven't reviewed this yet (will do it shortly), but the title reminded me of
> bug 618165 where we resolve url()s in custom property references with an empty
> base, so maybe a good idea to explicitly mention filter in the title :)
> 
> I added "non-<image>" to subject, since this technically speaking not limited
to
> 'filter' (it's the only place where it makes any difference ATM though.) I
take
> it that bug (crbug.com/618165) is essentially the TODO in
> CSSSyntaxDescriptor.cpp? (which I duplicated)

Not quite, the bug is about using urls in regular properties with var
references, for example something like:
--a: pink; background: var(--a) url(foo.png);
or
--a: url(foo.png); background: pink var(--a);
where CSSVariableResolver just uses strictCSSParserContext. So we could do
something like store a pointer to the current context on the CSSVariableData (on
CSSVariableReferenceValue would also work for this case but having it on
CSSVariableData also handles the below case).

The TODO is a related issue where accessing registered custom properties via
Typed OM (which is only implemented for custom paint so far) doesn't work for
relative url images. So having the context in CSSVariableData resolves the TODO
you moved up the stack in your other patch.

Anyway don't feel compelled to look at or fix these, it just seemed somewhat
related to what you were doing here :)

[fs, 2017-01-16 15:19:20]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 15:19:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2017-01-16 16:30:34]

junov, could you PTAL/RS the modules/canvas2d/ change?

On 2017/01/15 at 23:30:53, timloh wrote:
...
> On 2017/01/13 08:58:37, fs wrote:
> > On 2017/01/13 at 00:58:45, timloh wrote:
> > > On 2017/01/12 20:18:48, fs wrote:
> > > 
> > > Haven't reviewed this yet (will do it shortly), but the title reminded me
of
> > bug 618165 where we resolve url()s in custom property references with an
empty
> > base, so maybe a good idea to explicitly mention filter in the title :)
> > 
> > I added "non-<image>" to subject, since this technically speaking not
limited to
> > 'filter' (it's the only place where it makes any difference ATM though.) I
take
> > it that bug (crbug.com/618165) is essentially the TODO in
> > CSSSyntaxDescriptor.cpp? (which I duplicated)
> 
> Not quite, the bug is about using urls in regular properties with var
references, for example something like:
> --a: pink; background: var(--a) url(foo.png);
> or
> --a: url(foo.png); background: pink var(--a);
> where CSSVariableResolver just uses strictCSSParserContext. So we could do
something like store a pointer to the current context on the CSSVariableData (on
CSSVariableReferenceValue would also work for this case but having it on
CSSVariableData also handles the below case).
> 
> The TODO is a related issue where accessing registered custom properties via
Typed OM (which is only implemented for custom paint so far) doesn't work for
relative url images. So having the context in CSSVariableData resolves the TODO
you moved up the stack in your other patch.
> 
> Anyway don't feel compelled to look at or fix these, it just seemed somewhat
related to what you were doing here :)

Thanks for the explanation! (I figured some of it out already by reading through
the bug w/ context.) I'm not sure if I feel compelled, but you never know... ;-)

[commit-bot: I haz the power, 2017-01-16 17:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:05:18]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2017-01-17 16:46:24]

modules/canvas2d lgtm

[fs, 2017-01-17 16:51:03]

The CQ bit was checked by fs@opera.com

[fs, 2017-01-17 16:51:04]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2625873010/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-17 16:51:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:36:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:36:32]

Failed to apply patch for
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1737
  error: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index
b7f9ff35f4908017e3d23f8088c71c577b408ee0..3184f20455bbcb36ad8e22f54329e41af267c961
100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1298,7 +1298,7 @@ static CSSValue* consumeFilter(CSSParserTokenRange& range,
 
   CSSValueList* list = CSSValueList::createSpaceSeparated();
   do {
-    CSSValue* filterValue = consumeUrl(range);
+    CSSValue* filterValue = consumeUrl(range, context);
     if (!filterValue) {
       filterValue = consumeFilterFunction(range, context);
       if (!filterValue)
@@ -1719,16 +1719,16 @@ static CSSValue* consumePositionY(CSSParserTokenRange&
range,
 }
 
 static CSSValue* consumePaintStroke(CSSParserTokenRange& range,
-                                    CSSParserMode cssParserMode) {
+                                    const CSSParserContext* context) {
   if (range.peek().id() == CSSValueNone)
     return consumeIdent(range);
-  CSSURIValue* url = consumeUrl(range);
+  CSSURIValue* url = consumeUrl(range, context);
   if (url) {
     CSSValue* parsedValue = nullptr;
     if (range.peek().id() == CSSValueNone)
       parsedValue = consumeIdent(range);
     else
-      parsedValue = consumeColor(range, cssParserMode);
+      parsedValue = consumeColor(range, context->mode());
     if (parsedValue) {
       CSSValueList* values = CSSValueList::createSpaceSeparated();
       values->append(*url);
@@ -1737,13 +1737,14 @@ static CSSValue* consumePaintStroke(CSSParserTokenRange&
range,
     }
     return url;
   }
-  return consumeColor(range, cssParserMode);
+  return consumeColor(range, context->mode());
 }
 
-static CSSValue* consumeNoneOrURI(CSSParserTokenRange& range) {
+static CSSValue* consumeNoneOrURI(CSSParserTokenRange& range,
+                                  const CSSParserContext* context) {
   if (range.peek().id() == CSSValueNone)
     return consumeIdent(range);
-  return consumeUrl(range);
+  return consumeUrl(range, context);
 }
 
 static CSSValue* consumeStrokeDasharray(CSSParserTokenRange& range) {
@@ -2183,7 +2184,7 @@ static CSSValue* consumeClipPath(CSSParserTokenRange&
range,
                                  const CSSParserContext* context) {
   if (range.peek().id() == CSSValueNone)
     return consumeIdent(range);
-  if (CSSURIValue* url = consumeUrl(range))
+  if (CSSURIValue* url = consumeUrl(range, context))
     return url;
   return consumeBasicShape(range, context);
 }
@@ -3363,12 +3364,12 @@ const CSSValue* CSSPropertyParser::parseSingleValue(
       return consumePositionY(m_range, m_context->mode());
     case CSSPropertyFill:
     case CSSPropertyStroke:
-      return consumePaintStroke(m_range, m_context->mode());
+      return consumePaintStroke(m_range, m_context);
     case CSSPropertyMarkerStart:
     case CSSPropertyMarkerMid:
     case CSSPropertyMarkerEnd:
     case CSSPropertyMask:
-      return consumeNoneOrURI(m_range);
+      return consumeNoneOrURI(m_range, m_context);
     case CSSPropertyFlexGrow:
     case CSSPropertyFlexShrink:
       return consumeNumber(m_range, ValueRangeNonNegative);

[fs, 2017-01-17 21:32:16]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2017-01-17 22:50:08]

The CQ bit was unchecked by fs@opera.com

[fs, 2017-01-17 22:50:16]

The CQ bit was checked by fs@opera.com

[fs, 2017-01-17 22:50:16]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2625873010/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-17 22:50:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 23:16:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484693416404260,
"parent_rev": "cae06e2aab96be77610c27f4a1469f9570df1c3c", "commit_rev":
"db478183363dad5b460070122b991a8302a1f98a"}

[commit-bot: I haz the power, 2017-01-17 23:17:15]

Description was changed from

==========
Resolve CSS url(...) non-<image> values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315
==========

to

==========
Resolve CSS url(...) non-<image> values against the correct base

In an external stylesheet, the url(...) should resolve against the URL of
the stylesheet. We were always resolving against the document base URL,
meaning that 'filter' url(...)'s referencing external files could resolve
to the wrong URL.
Besides this, add a more generic mechanism to determine whether a CSSValue
contains a URL (and hence may need to be re-resolved if it's within an
inline style declaration, and it's context element is moved to a different
document.)
Also adjust the <canvas> 'filter' implementation to resolve against the
elements document when applicable.

BUG=405315

Review-Url: https://codereview.chromium.org/2625873010
Cr-Commit-Position: refs/heads/master@{#444179}
Committed:
https://chromium.googlesource.com/chromium/src/+/db478183363dad5b460070122b99...
==========

[commit-bot: I haz the power, 2017-01-17 23:17:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/db478183363dad5b460070122b99...