Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208283002:
[Typed-OM] Add CSSURLImageValue (Closed)
|
Created:
4 years, 4 months ago by anthonyhkf Modified:
4 years, 4 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@CSSProperties_Image Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Add CSSURLImageValue
Spec: https://drafts.css-houdini.org/css-typed-om/#cssurlimagevalue
BUG=545318
Committed: https://crrev.com/15c53e904f0eda633664ac054a421f9bf8b6146f
Cr-Commit-Position: refs/heads/master@{#413386}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Reparent branch #
Total comments: 4
Patch Set 3 : Add simple unittest #Patch Set 4 : Add to global interface listing and add layouttest #
Total comments: 2
Patch Set 5 : Change String to AtomicString #Patch Set 6 : Rebase #Patch Set 7 : Fixed typo #Patch Set 8 : Add unit test with CSSImageValue #
Total comments: 8
Patch Set 9 : Changed url #Patch Set 10 : Remove unknown changes to chromium.linux.json #
Total comments: 4
Patch Set 11 : Make the class final and explicit constructor #Patch Set 12 : Rebase #Messages
Total messages: 55 (41 generated)
anthonyhkf@google.com changed reviewers: + ikilpatrick@chromium.org, meade@chromium.org
https://codereview.chromium.org/2208283002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:33: CSSURLImageValue(const CSSImageValue* imageValue) This can be CSSURLImageValue(const CSSImageValue* imageValue) : CSSStyleImageValue(imageValue) {}
lgtm with comment. Let's come back to adding the OWNER reviewer when the chain of CLs has been resolved though - it's got very long! o.O Let's focus on getting the ones at the start landed :)
sorry for the slow review. https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:29: : CSSStyleImageValue(CSSImageValue::create(AtomicString(url))) Do we need this constructor? In create is it better just to go: static CSSURLImageValue* create(const String& url) { return new CSSURLImageValue(CSSImageValue::create(url)); } ? https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:35: m_imageValue = imageValue; instead of assigning pass this into the parent constructor? I.e. CSSURLImageValue(...) : CSSStyleImageValue(imageValue) { }
https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:29: : CSSStyleImageValue(CSSImageValue::create(AtomicString(url))) On 2016/08/06 00:18:52, ikilpatrick wrote: > Do we need this constructor? In create is it better just to go: > > static CSSURLImageValue* create(const String& url) > { > return new CSSURLImageValue(CSSImageValue::create(url)); > } > > ? Done. https://codereview.chromium.org/2208283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:35: m_imageValue = imageValue; On 2016/08/06 00:18:52, ikilpatrick wrote: > instead of assigning pass this into the parent constructor? > > I.e. > > CSSURLImageValue(...) > : CSSStyleImageValue(imageValue) > { > } Done.
lgtm https://codereview.chromium.org/2208283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:16: static CSSURLImageValue* create(const String& url) Is it possible for this create function to accept an AtomicString... and the bindings just work? I forget: static CSSURLImageValue* create(const AtomicString& url) ... etc.
https://codereview.chromium.org/2208283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:16: static CSSURLImageValue* create(const String& url) On 2016/08/07 13:35:13, ikilpatrick wrote: > Is it possible for this create function to accept an AtomicString... and the > bindings just work? I forget: > > static CSSURLImageValue* create(const AtomicString& url) > ... etc. It seems that it can :)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
meade@chromium.org changed reviewers: + esprehn@chromium.org
+elliott for OWNERS! (you were my random dice roll this time :) https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:8: var bg = new CSSURLImageValue("https://s-media-cache-ak0.pinimg.com/736x/b7/49/fa/b749fa790b531d4fb8f2007bf74859f0.jpg"); Don't use this URL. Anything else will do since it doesn't get loaded. We want to avoid hitting this URL by accident when running tests later if we forget to change it :) https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp (right): https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:5: Remove extra newline https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:26: checkNullURLImageValue(CSSURLImageValue::create("https://s-media-cache-ak0.pinimg.com/736x/b7/49/fa/b749fa790b531d4fb8f2007bf74859f0.jpg")); Likewise, don't use the URL here either :) Maybe use http://localhost? https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:31: checkNullURLImageValue(CSSURLImageValue::create(CSSImageValue::create("http://example.com"))); Same.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:8: var bg = new CSSURLImageValue("https://s-media-cache-ak0.pinimg.com/736x/b7/49/fa/b749fa790b531d4fb8f2007bf74859f0.jpg"); On 2016/08/12 05:35:32, Eddy wrote: > Don't use this URL. Anything else will do since it doesn't get loaded. > We want to avoid hitting this URL by accident when running tests later if we > forget to change it :) Done. https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp (right): https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:5: On 2016/08/12 05:35:32, Eddy wrote: > Remove extra newline Done. https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:26: checkNullURLImageValue(CSSURLImageValue::create("https://s-media-cache-ak0.pinimg.com/736x/b7/49/fa/b749fa790b531d4fb8f2007bf74859f0.jpg")); On 2016/08/12 05:35:32, Eddy wrote: > Likewise, don't use the URL here either :) > Maybe use http://localhost Done. https://codereview.chromium.org/2208283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValueTest.cpp:31: checkNullURLImageValue(CSSURLImageValue::create(CSSImageValue::create("http://example.com"))); On 2016/08/12 05:35:32, Eddy wrote: > Same. Done.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits fixed. https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:12: class CORE_EXPORT CSSURLImageValue : public CSSStyleImageValue { final ? https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:32: CSSURLImageValue(const CSSImageValue* imageValue) explicit
https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h (right): https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:12: class CORE_EXPORT CSSURLImageValue : public CSSStyleImageValue { On 2016/08/15 16:04:10, esprehn wrote: > final ? Done. https://codereview.chromium.org/2208283002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSURLImageValue.h:32: CSSURLImageValue(const CSSImageValue* imageValue) On 2016/08/15 16:04:10, esprehn wrote: > explicit Done.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyhkf@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, ikilpatrick@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2208283002/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Add CSSURLImageValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssurlimagevalue BUG=545318 ========== to ========== [Typed-OM] Add CSSURLImageValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssurlimagevalue BUG=545318 Committed: https://crrev.com/15c53e904f0eda633664ac054a421f9bf8b6146f Cr-Commit-Position: refs/heads/master@{#413386} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/15c53e904f0eda633664ac054a421f9bf8b6146f Cr-Commit-Position: refs/heads/master@{#413386} |
