BackgroundImage incorrectly returns empty url() when created on-the-fly

BackgroundImage incorrectly returns empty url() when created on-the-fly

When you direcly create a <style> element and set it's backgroundImage
via styleElement.textContent, accessing the image URL via
styleElement.sheet.cssRules[0].style.backgroundImage results in empty
URL. This is inconsistent with both FF and IE.

This patch also ensures that getComputedStyle().backgroundImage returns
the absolute URL while style.backgroundImage returns the relativeURL as
is consistent with the spec stated here:
https://drafts.csswg.org/css-backgrounds-3/#the-background-image.

Please note that certain layout tests have been changed to increase
readability and to make sure that they are consistent with FF. Also FF
currently returns all URLs with quotes arounds them - that is 
consistent with spec. We currently don't do that. That will have to be
a different patch.

BUG=161644, 529640
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202625

[nainar, 2015-09-08 23:14:49]

nainar@chromium.org changed reviewers:
+ timloh@chromium.org

[nainar, 2015-09-08 23:14:49]

PTAL

Thank you!

[rune, 2015-09-08 23:57:28]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2015-09-08 23:57:28]

https://codereview.chromium.org/1306283006/diff/1/LayoutTests/fast/css/css-im...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/1/LayoutTests/fast/css/css-im...
LayoutTests/fast/css/css-imagevalue-url.html:34: test(function() {
Indentation looks inconsistent. Sometimes 8, most of the time 4.

https://codereview.chromium.org/1306283006/diff/1/Source/core/css/CSSImageVal...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/1/Source/core/css/CSSImageVal...
Source/core/css/CSSImageValue.cpp:111: if (m_absoluteURL.length())
Gecko is using the relative URL. Looking at the CSSOM spec, it doesn't say
anything about serializing the absolute url. If the relative URL is available,
we should probably use that?

[nainar, 2015-09-09 00:30:30]

rune,

Have made the changes, PTAL?

Thanks!

https://codereview.chromium.org/1306283006/diff/1/LayoutTests/fast/css/css-im...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/1/LayoutTests/fast/css/css-im...
LayoutTests/fast/css/css-imagevalue-url.html:34: test(function() {
Done.

On 2015/09/08 at 23:57:28, rune wrote:
> Indentation looks inconsistent. Sometimes 8, most of the time 4.

https://codereview.chromium.org/1306283006/diff/1/Source/core/css/CSSImageVal...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/1/Source/core/css/CSSImageVal...
Source/core/css/CSSImageValue.cpp:111: if (m_absoluteURL.length())
On 2015/09/08 at 23:57:28, rune wrote:
> Gecko is using the relative URL. Looking at the CSSOM spec, it doesn't say
anything about serializing the absolute url. If the relative URL is available,
we should probably use that?

Done.

[rune, 2015-09-09 07:24:21]

On 2015/09/09 00:30:30, nainar92 wrote:
> rune,
> 
> Have made the changes, PTAL?

We should have a crbug issue, pointed to by BUG=, for tracking the absolute ->
relative serialization as it's quite a fundamental change. I've double checked
with simonp/zcorpan who's editor of CSSOM just to confirmed that the change is
good and in line with the spec.

You have a few failures now which you need to carefully go through to see if
they're expected, and also double check with Gecko (and IE?).

Sorry if this was more work than you planned for :-/

[nainar, 2015-09-09 07:43:07]

Could you provide me with a link to the spec? For my own perusal in this regard.
Also am currently in the process of going through all the failures. Filing a bug
and editing the description as well.

[rune, 2015-09-09 07:54:42]

On 2015/09/09 07:43:07, nainar92 wrote:
> Could you provide me with a link to the spec? For my own perusal in this
regard.
> Also am currently in the process of going through all the failures. Filing a
bug
> and editing the description as well.

https://drafts.csswg.org/cssom/#serialize-a-url

[nainar, 2015-09-15 09:20:26]

The CQ bit was checked by nainar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-15 09:20:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306283006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306283006/120001

[commit-bot: I haz the power, 2015-09-15 09:20:30]

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

[commit-bot: I haz the power, 2015-09-15 09:20:31]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[nainar, 2015-09-15 09:45:22]

Hi!

I have managed to rebaseline the tests. Sorry it took so long. PTAL?

JThanks!

[Timothy Loh, 2015-09-15 12:20:48]

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/a...
File LayoutTests/fast/css/all-inherit-or-unset-color.html (left):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/a...
LayoutTests/fast/css/all-inherit-or-unset-color.html:1: <!DOCTYPE html>
Why is this file removed?

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-escaped-identifier.html (right):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-escaped-identifier.html:24:
assert_equals(rules[2].style.cssText, "background-image: url(http://w/" +
decodeURIComponent("%EA%AA%AA") + "/1b" + decodeURIComponent("%EA%AA%AA") +
".gif);", "Escaped url string");
The patch description should probably mention this behaviour change (is it
consistent with FF/IE?)

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL-expected.txt (right):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL-expected.txt:2: PASS Testing
getComputedStyle.backgroundImage returns absolute URlS and style.backgroundImage
returns relative URLs
testharness tests shouldn't need expected files (here and the png)

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:122: if (m_fromValue->isImageValue() &&
m_toValue->isImageValue()) {
The logic here seems weird. What if only one side is an image value?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:130: float percentage =
m_percentageValue->getFloatValue();
???

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:45: static
PassRefPtrWillBeRawPtr<CSSCrossfadeValue>
create(PassRefPtrWillBeRawPtr<CSSValue> fromValue,
PassRefPtrWillBeRawPtr<CSSValue> toValue,
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> percentageValue = nullptr)
Better to do this change separately and properly (i.e. without the default
argument)

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:84: , m_cachedFromImage(0)
Why the change?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
File Source/core/css/CSSImageGeneratorValue.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
Source/core/css/CSSImageGeneratorValue.h:71:
PassRefPtrWillBeRawPtr<CSSImageGeneratorValue> valueWithAbsoluteURL();
Maybe valueWithURLsMadeAbsolute / valueWithURLMadeAbsolute (for CSSImageValue)?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:34: #include "wtf/text/StringBuilder.h"
unneeded?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
File Source/core/css/CSSMarkup.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
Source/core/css/CSSMarkup.cpp:266: String serializeStringWithoutQuotations(const
String& string)
Is this a separate change?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
File Source/core/css/CSSMarkup.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
Source/core/css/CSSMarkup.h:39: String serializeStringWithoutQuotations(const
String&);
This function isn't a common serializing method from the specs like the comment
above suggests.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StyleFetchedImageSet.cpp:57: return m_imageSetValue ?
m_imageSetValue->valueWithAbsoluteURL() : nullptr;
Looks like m_imageSetValue is never null?

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StyleGeneratedImage.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StyleGeneratedImage.cpp:47: return m_imageGeneratorValue ?
m_imageGeneratorValue->valueWithAbsoluteURL() : nullptr;
m_imageGeneratorValue looks non-null

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StylePendingImage.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StylePendingImage.h:55: return (m_value &&
m_value->isImageValue()) ? toCSSImageValue(m_value)->valueWithAbsoluteURL() :
nullptr;
Does this function ever get reached?

[nainar, 2015-09-16 07:16:49]

Tim,

PTAL?

Thanks!

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/a...
File LayoutTests/fast/css/all-inherit-or-unset-color.html (left):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/a...
LayoutTests/fast/css/all-inherit-or-unset-color.html:1: <!DOCTYPE html>
Fixed.

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-escaped-identifier.html (right):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-escaped-identifier.html:24:
assert_equals(rules[2].style.cssText, "background-image: url(http://w/" +
decodeURIComponent("%EA%AA%AA") + "/1b" + decodeURIComponent("%EA%AA%AA") +
".gif);", "Escaped url string");
The resulting output is consistent with FF. Changing the patch to describe this.

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL-expected.txt (right):

https://codereview.chromium.org/1306283006/diff/160001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL-expected.txt:2: PASS Testing
getComputedStyle.backgroundImage returns absolute URlS and style.backgroundImage
returns relative URLs
Removed.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:122: if (m_fromValue->isImageValue() &&
m_toValue->isImageValue()) {
Fixed.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:130: float percentage =
m_percentageValue->getFloatValue();
Fixed.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:45: static
PassRefPtrWillBeRawPtr<CSSCrossfadeValue>
create(PassRefPtrWillBeRawPtr<CSSValue> fromValue,
PassRefPtrWillBeRawPtr<CSSValue> toValue,
PassRefPtrWillBeRawPtr<CSSPrimitiveValue> percentageValue = nullptr)
Done.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:84: , m_cachedFromImage(0)
Fixed.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
File Source/core/css/CSSImageGeneratorValue.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
Source/core/css/CSSImageGeneratorValue.h:71:
PassRefPtrWillBeRawPtr<CSSImageGeneratorValue> valueWithAbsoluteURL();
Changed name across all to -> valueWithURLMadeAbsolute

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:34: #include "wtf/text/StringBuilder.h"
Done.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
File Source/core/css/CSSMarkup.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
Source/core/css/CSSMarkup.cpp:266: String serializeStringWithoutQuotations(const
String& string)
Answered above.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
File Source/core/css/CSSMarkup.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/css/CSSMar...
Source/core/css/CSSMarkup.h:39: String serializeStringWithoutQuotations(const
String&);
Currently, we don't serialize strings the way the spec asks
(https://drafts.csswg.org/cssom/#serialize-a-string). FF does.

This function is to ensure we are on par with other Chrome layout test which
doesn't enclose relative URL in quotes.

A later patch will remove this function and instead call serializeString along
with making sure that all tests too are updated.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StyleFetchedImageSet.cpp:57: return m_imageSetValue ?
m_imageSetValue->valueWithAbsoluteURL() : nullptr;
Done.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StyleGeneratedImage.cpp (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StyleGeneratedImage.cpp:47: return m_imageGeneratorValue ?
m_imageGeneratorValue->valueWithAbsoluteURL() : nullptr;
Done.

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
File Source/core/style/StylePendingImage.h (right):

https://codereview.chromium.org/1306283006/diff/160001/Source/core/style/Styl...
Source/core/style/StylePendingImage.h:55: return (m_value &&
m_value->isImageValue()) ? toCSSImageValue(m_value)->valueWithAbsoluteURL() :
nullptr;
Removed.

[nainar, 2015-09-16 08:34:21]

Have rebased, PTAL?

[nainar, 2015-09-16 12:26:20]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2015-09-16 12:26:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306283006/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306283006/220001

[commit-bot: I haz the power, 2015-09-16 12:26:26]

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

[commit-bot: I haz the power, 2015-09-16 12:26:27]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[nainar, 2015-09-16 12:26:36]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2015-09-17 02:42:54]

nainar@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2015-09-18 02:55:09]

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:13: var rulesForCssText = function
(styleContent) {
No space after function.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:15: styleElement =
document.createElement("style");
Use separate var statements.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:18: // the style will only parsed
once it is added to a document
No need for comment.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:36:
assert_equals(cssRulesToText(rulesForCssText(cssText)), cssText);
No need for the helper functions if they're only doing one item.
No need for the <style> element on this page when you intend to simply set text
from Javascript.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:37: }, 'Testing URLs in CSS');
This is very vague, you're actually testing serialisation and round tripping of
urls in CSS.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL.html (right):

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:8: }
Left over debugging code should be removed.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:26: <div
id="target"></div>
target is never used.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:131: return this;
This can be simplified if you use temporary variables for from and to.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
File Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
Source/core/css/CSSImageSetValue.cpp:199: }
No need for braces, this could also be a ternary.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:114: // we need another patch that fixes that.
For now serializing without quotation marks
This should be a one liner.
// TODO(nainar): Serialise with quotation marks to match spec:
https://drafts.csswg.org/cssom/#serialize-a-string

https://codereview.chromium.org/1306283006/diff/220001/Source/core/style/Styl...
File Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/style/Styl...
Source/core/style/StyleFetchedImage.cpp:54: return
CSSImageValue::create(m_image->url(), const_cast<StyleFetchedImage*>(this));
Add comment stating why the const_cast is necessary.

[nainar, 2015-09-18 03:47:25]

PTAL?

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:13: var rulesForCssText = function
(styleContent) {
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:15: styleElement =
document.createElement("style");
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:18: // the style will only parsed
once it is added to a document
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:36:
assert_equals(cssRulesToText(rulesForCssText(cssText)), cssText);
Done.
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:37: }, 'Testing URLs in CSS');
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL.html (right):

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:8: }
Done.

https://codereview.chromium.org/1306283006/diff/220001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:26: <div
id="target"></div>
Done.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.cpp:131: return this;
Done.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
File Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
Source/core/css/CSSImageSetValue.cpp:199: }
Done.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:114: // we need another patch that fixes that.
For now serializing without quotation marks
Done.

https://codereview.chromium.org/1306283006/diff/220001/Source/core/style/Styl...
File Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1306283006/diff/220001/Source/core/style/Styl...
Source/core/style/StyleFetchedImage.cpp:54: return
CSSImageValue::create(m_image->url(), const_cast<StyleFetchedImage*>(this));
Done.

[alancutter (OOO until 2018), 2015-09-21 00:57:47]

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:6: addEventListener("load",
function() {
Is it necessary to wait for the load event? A comment should explain if so.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:19: });
There should only be one rule (you can assert this), no need to iterate.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL.html (right):

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:5: <div id =
"target1"></div>
s/target1/target/
Spaces around the = is inconsistent with other DOM nodes and general LayoutTest
style.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:10:
assert_not_equals(target1.style.backgroundImage,
getComputedStyle(target1).backgroundImage);
This is a fairly weak assertion. You can build the expected string using
location.href.replace('getComputedStyle-relativeURL.html', 'images1.jpg').

[nainar, 2015-09-21 01:27:36]

Alan,

PTAL?

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
File LayoutTests/fast/css/css-imagevalue-url.html (right):

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:6: addEventListener("load",
function() {
Done.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/c...
LayoutTests/fast/css/css-imagevalue-url.html:19: });
Done.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
File LayoutTests/fast/css/getComputedStyle-relativeURL.html (right):

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:5: <div id =
"target1"></div>
Done.

https://codereview.chromium.org/1306283006/diff/260001/LayoutTests/fast/css/g...
LayoutTests/fast/css/getComputedStyle-relativeURL.html:10:
assert_not_equals(target1.style.backgroundImage,
getComputedStyle(target1).backgroundImage);
Done.

[alancutter (OOO until 2018), 2015-09-21 01:39:19]

lgtm

[nainar, 2015-09-22 05:41:25]

Tim,

PTAL?

Thanks!

[Timothy Loh, 2015-09-22 05:53:27]

lgtm as long as we fix up url serialization soonish

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.h (right):

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:67:
PassRefPtrWillBeRawPtr<CSSCrossfadeValue> valueWithURLMadeAbsolute();
URL -> URLs in all of these except CSSImageValue

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:111: // TODO(nainar): Serialise with quotation
marks to match spec: https://drafts.csswg.org/cssom/#serialize-a-string
https://drafts.csswg.org/cssom/#serialize-a-url ?

[nainar, 2015-09-22 06:14:16]

Rune,

PTAL?

Thanks!

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSCro...
File Source/core/css/CSSCrossfadeValue.h (right):

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSCro...
Source/core/css/CSSCrossfadeValue.h:67:
PassRefPtrWillBeRawPtr<CSSCrossfadeValue> valueWithURLMadeAbsolute();
Done.

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSIma...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/1306283006/diff/300001/Source/core/css/CSSIma...
Source/core/css/CSSImageValue.cpp:111: // TODO(nainar): Serialise with quotation
marks to match spec: https://drafts.csswg.org/cssom/#serialize-a-string
Done.

[rune, 2015-09-22 07:43:16]

On 2015/09/22 06:14:16, nainar wrote:
> Rune,
> 
> PTAL?

You already got two l*tms. I trust those.

[nainar, 2015-09-22 07:49:14]

The CQ bit was checked by nainar@chromium.org

[nainar, 2015-09-22 07:49:15]

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

[commit-bot: I haz the power, 2015-09-22 07:49:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306283006/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306283006/320001

[commit-bot: I haz the power, 2015-09-22 08:49:28]

Committed patchset #17 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202625