[Typed-OM] Add CSSStyleImageValue

[Typed-OM] Add CSSStyleImageValue

Spec: https://drafts.css-houdini.org/css-typed-om/#cssimagevalue

BUG=545318
Committed: https://crrev.com/843f49dfe710d6b230ea7cf4c1970e72561f680c
Cr-Commit-Position: refs/heads/master@{#411566}

[anthonyhkf, 2016-08-01 08:13:02]

anthonyhkf@google.com changed reviewers:
+ meade@chromium.org

[meade_UTC10, 2016-08-02 00:53:39]

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

[meade_UTC10, 2016-08-02 00:53:39]

lgtm

+tim for OWNERS review

https://codereview.chromium.org/2199703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h (right):

https://codereview.chromium.org/2199703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:59: const String&
url() const { return m_imageValue.get()->url(); }
What's this for? Did you mean to put this on CSSURLImageValue?

[anthonyhkf, 2016-08-02 01:23:14]

https://codereview.chromium.org/2199703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h (right):

https://codereview.chromium.org/2199703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:59: const String&
url() const { return m_imageValue.get()->url(); }
On 2016/08/02 00:53:39, Eddy wrote:
> What's this for? Did you mean to put this on CSSURLImageValue?

Done.
I forgot to remove it. I used it when I was trying to make other tests.

[Timothy Loh, 2016-08-02 04:31:02]

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl (right):

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl:1: [
copyright header

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl:3: ] interface
CSSImageValue : CSSResourceValue {
Does this not need the RuntimeEnabled because it inherited from a RuntimeEnabled
class?

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h (right):

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:25: if (!isNull) {
nit: generally it's nicer to do this sort of error check the other way around,
i.e.

if (isNull)
    return 0;
...

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:46: if
(intrinsicWidth(isNull) <= 0) {
Is this ever negative (aside from in the test below)? It's inconsistent to bail
if width is negative but not if height is negative.

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:47: return 0;
Why 0?

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:49: return
intrinsicHeight(isNull) / intrinsicWidth(isNull);
Is height/width correct here (vs width/height)? The spec doesn't say and I would
expect width/height here.

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValueTest.cpp
(right):

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValueTest.cpp:39:
EXPECT_EQ(isNull, true);
EXPECT_TRUE, similar below.

[anthonyhkf, 2016-08-02 06:48:32]

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl (right):

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl:1: [
On 2016/08/02 04:31:02, Timothy Loh wrote:
> copyright header

Done.

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSImageValue.idl:3: ] interface
CSSImageValue : CSSResourceValue {
On 2016/08/02 04:31:02, Timothy Loh wrote:
> Does this not need the RuntimeEnabled because it inherited from a
RuntimeEnabled
> class?

Done.

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValueTest.cpp
(right):

https://codereview.chromium.org/2199703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValueTest.cpp:39:
EXPECT_EQ(isNull, true);
On 2016/08/02 04:31:02, Timothy Loh wrote:
> EXPECT_TRUE, similar below.

Done.

[Timothy Loh, 2016-08-04 03:09:33]

lgtm

[meade_UTC10, 2016-08-04 07:00:54]

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h (right):

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:38: double
intrinsicRatio(bool& isNull)
Can the body of these methods be moved into a .cpp file?

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:60:
CSSStyleImageValue() { }
I don't think this empty constructor is needed?

[anthonyhkf, 2016-08-04 23:59:03]

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h (right):

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:38: double
intrinsicRatio(bool& isNull)
On 2016/08/04 07:00:54, Eddy wrote:
> Can the body of these methods be moved into a .cpp file?

Done.

https://codereview.chromium.org/2199703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h:60:
CSSStyleImageValue() { }
On 2016/08/04 07:00:54, Eddy wrote:
> I don't think this empty constructor is needed?

Done.

[anthonyhkf, 2016-08-09 00:18:26]

The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 00:18:59]

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, 2016-08-09 00:27:08]

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

[commit-bot: I haz the power, 2016-08-09 00:27:09]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[anthonyhkf, 2016-08-09 03:38:35]

The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 03:39:06]

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, 2016-08-09 03:55:23]

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

[commit-bot: I haz the power, 2016-08-09 03:55:24]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2016-08-12 05:36:50]

lgtm

[anthonyhkf, 2016-08-12 05:57:47]

The CQ bit was checked by anthonyhkf@google.com

[anthonyhkf, 2016-08-12 05:57:47]

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/2199703002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-12 05:58:04]

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, 2016-08-12 07:14:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-12 07:16:30]

Description was changed from

==========
[Typed-OM] Add CSSStyleImageValue

Spec: https://drafts.css-houdini.org/css-typed-om/#cssimagevalue

BUG=545318
==========

to

==========
[Typed-OM] Add CSSStyleImageValue

Spec: https://drafts.css-houdini.org/css-typed-om/#cssimagevalue

BUG=545318

Committed: https://crrev.com/843f49dfe710d6b230ea7cf4c1970e72561f680c
Cr-Commit-Position: refs/heads/master@{#411566}
==========

[commit-bot: I haz the power, 2016-08-12 07:16:31]

Patchset 8 (id:??) landed as
https://crrev.com/843f49dfe710d6b230ea7cf4c1970e72561f680c
Cr-Commit-Position: refs/heads/master@{#411566}