Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2126713002:
[Typed-OM] Add CSSVariableReferenceValue (Closed)
|
Created:
4 years, 5 months ago by anthonyhkf Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Add CSSVariableReferenceValue
Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue
BUG=545318
Committed: https://crrev.com/cedaf8d293f3ab120e383f0e5adcb00b4d8710b2
Cr-Commit-Position: refs/heads/master@{#404937}
Patch Set 1 #Patch Set 2 : Removed fallback #Patch Set 3 : Add test #
Total comments: 11
Patch Set 4 : Remove unused file, fix orders #Patch Set 5 : Try to fix the layout test #
Total comments: 17
Patch Set 6 : Renaming things, remove unused things, add license header #
Total comments: 6
Patch Set 7 : Fixed spaces, remove unused header #
Messages
Total messages: 35 (16 generated)
anthonyhkf@google.com changed reviewers: + meade@chromium.org
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...
Please add a link to the spec (anchored to CSSVariableReferenceValue) to your CL description. You will probably also need to update these two files: third_party/WebKit/LayoutTests/http/tests/worklet/webexposed/global-interface-listing-paint-worklet-expected.txt third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/core.gypi:51: 'css/cssom/CSSVariableReferenceValue.idl', This isn't in alphabetical order. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.cpp (right): https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.cpp:6: } Let's just not add this file until it's needed. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:11: // TODO: change the data type of fallback as in the draft You don't need to have two TODO's, just the second one is fine. Please delete this line. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:13: class CORE_EXPORT CSSStyleVariableReferenceValue : public GarbageCollectedFinalized<CSSStyleVariableReferenceValue>, public ScriptWrappable { Please make this class final. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:14: DEFINE_WRAPPERTYPEINFO(); This class shouldn't be copyable. Please add the macro WTF_MAKE_NONCOPYABLE(CSSStyleVariableReferenceValue); https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:23: // TODO: add fallback: create(variable, fallback) Make this // TODO(anthonyhkf): Add fallback: create(variable, fallback). https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:31: void setVariable(String variable) { m_variable = variable; } StyleValues should be read-only - please remove this method and change the corresponding IDL also be readonly. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: Constructor(DOMString variable) In the other files, we've always put the constructor first. Please re-order these items so the constructors are first. https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:7: // TODO: add fallback: Constructor(DOMString variable, DOMString fallback) Should be TODO(anthonyhsk): ... https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:9: attribute DOMString variable; Let's make this readonly. readonly attribute DOMString variable;
Description was changed from ========== [Typed-OM] Add CSSVariableResourceValue BUG=619010 ========== to ========== [Typed-OM] Add CSSVariableResourceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=619010 ==========
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_...)
Description was changed from ========== [Typed-OM] Add CSSVariableResourceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=619010 ========== to ========== [Typed-OM] Add CSSVariableResourceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ==========
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...
meade@chromium.org changed reviewers: + timloh@chromium.org
lgtm with nits, +tim for OWNERS review. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html:9: var temp = new CSSVariableReferenceValue(expected); Again, let's not call this variable temp, since it's the thing under test. I think we called it variableReferenceValue or something in the polyfill? https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:33: CSSStyleVariableReferenceValue() { } We shouldn't have this empty constructor - m_variable won't be initialised in this case. You can also remove the empty create method and the empty constructor in the IDL. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:2: Constructor(), Remove this.
https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:1: #ifndef CSSStyleVariableReferenceValue_h There's a 3-line license header to add on new files, see blink.style/guide https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:30: String variable() const { return m_variable; } const String& is probably more common (maybe avoids some ref churn too) https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:36: { Use an initializer list here, e.g. CSSStyl..(..) : m_variable(variable) { } https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:44: } prevalent style is to write } // namespace blink #endif // BlaBlaBla_h https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: ImplementedAs=CSSStyleVariableReferenceValue OK for now, although this is a bit confusing. Maybe we should have a TODO to rename things.
https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: ImplementedAs=CSSStyleVariableReferenceValue On 2016/07/07 05:40:26, Timothy Loh wrote: > OK for now, although this is a bit confusing. Maybe we should have a TODO to > rename things. Do you have an idea for better naming? I'm at a bit of a loss (and CSSVariableReferenceValue was taken)
Description was changed from ========== [Typed-OM] Add CSSVariableResourceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ========== to ========== [Typed-OM] Add CSSVariableReferenceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:11: // TODO: change the data type of fallback as in the draft On 2016/07/06 05:40:11, Eddy wrote: > You don't need to have two TODO's, just the second one is fine. Please delete > this line. Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html:9: var temp = new CSSVariableReferenceValue(expected); On 2016/07/07 05:30:30, Eddy wrote: > Again, let's not call this variable temp, since it's the thing under test. I > think we called it variableReferenceValue or something in the polyfill? Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:1: #ifndef CSSStyleVariableReferenceValue_h On 2016/07/07 05:40:26, Timothy Loh wrote: > There's a 3-line license header to add on new files, see blink.style/guide Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:30: String variable() const { return m_variable; } On 2016/07/07 05:40:26, Timothy Loh wrote: > const String& is probably more common (maybe avoids some ref churn too) Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:33: CSSStyleVariableReferenceValue() { } On 2016/07/07 05:30:30, Eddy wrote: > We shouldn't have this empty constructor - m_variable won't be initialised in > this case. You can also remove the empty create method and the empty constructor > in the IDL. Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:36: { On 2016/07/07 05:40:26, Timothy Loh wrote: > Use an initializer list here, e.g. > > CSSStyl..(..) > : m_variable(variable) > { } Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:44: } On 2016/07/07 05:40:26, Timothy Loh wrote: > prevalent style is to write > > } // namespace blink > > #endif // BlaBlaBla_h Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:2: Constructor(), On 2016/07/07 05:30:30, Eddy wrote: > Remove this. Done. https://codereview.chromium.org/2126713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: ImplementedAs=CSSStyleVariableReferenceValue On 2016/07/07 05:40:26, Timothy Loh wrote: > OK for now, although this is a bit confusing. Maybe we should have a TODO to > rename things. Acknowledged.
The CQ bit was checked by meade@chromium.org 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 but the patch descrpition should probably mention the stuff you're doing which isn't in the spec (looks like the constructor isn't there, and you didn't implement the attribute as mutable) https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:9: #include "core/CSSPropertyNames.h" not used (you might need wtf/text/WTFString.h?) https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:11: #include "core/css/CSSValue.h" not used https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: // TODO(anthonyhkf): add fallback: Constructor(DOMString variable, CSSTokenStreamValue fallback) spacing is inconsistent btw, just use one space after // (here and below)
https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h (right): https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:9: #include "core/CSSPropertyNames.h" On 2016/07/08 04:36:30, Timothy Loh wrote: > not used (you might need wtf/text/WTFString.h?) Done. https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSStyleVariableReferenceValue.h:11: #include "core/css/CSSValue.h" On 2016/07/08 04:36:30, Timothy Loh wrote: > not used Done. https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl (right): https://codereview.chromium.org/2126713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSVariableReferenceValue.idl:6: // TODO(anthonyhkf): add fallback: Constructor(DOMString variable, CSSTokenStreamValue fallback) On 2016/07/08 04:36:30, Timothy Loh (ooo to jul18) wrote: > spacing is inconsistent btw, just use one space after // (here and below) Done.
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, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2126713002/#ps120001 (title: "Fixed spaces, remove unused header")
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.
Description was changed from ========== [Typed-OM] Add CSSVariableReferenceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ========== to ========== [Typed-OM] Add CSSVariableReferenceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Add CSSVariableReferenceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 ========== to ========== [Typed-OM] Add CSSVariableReferenceValue Spec: https://drafts.css-houdini.org/css-typed-om/#cssvariablereferencevalue BUG=545318 Committed: https://crrev.com/cedaf8d293f3ab120e383f0e5adcb00b4d8710b2 Cr-Commit-Position: refs/heads/master@{#404937} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/cedaf8d293f3ab120e383f0e5adcb00b4d8710b2 Cr-Commit-Position: refs/heads/master@{#404937} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
