Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2122193003:
[Typed-OM] Add CSSTokenStreamValue (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, rjwright Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Add CSSTokenStreamValue
Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue
In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>)
BUG=545318
Committed: https://crrev.com/183c36161d9f0f580975c8be4b22464eaefa3542
Cr-Commit-Position: refs/heads/master@{#406781}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changed CSSValue type, remove unused stuffs #Patch Set 3 : Added license header #
Total comments: 14
Patch Set 4 : Remove unused headers, renaming #
Total comments: 4
Patch Set 5 : Renamed functions #Patch Set 6 : Add CSSTokenStreamValue to global interface listing #Patch Set 7 : Add forEach method #Patch Set 8 : Add constructor #
Total comments: 2
Patch Set 9 : Add test for CSSTokenStreamValue #Dependent Patchsets: Messages
Total messages: 56 (35 generated)
anthonyhkf@google.com changed reviewers: + meade@chromium.org
https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:32: TokenStreamType, These should be in alphabetical order (To before Tr) :) https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:57: for (size_t i = 0; i < m_listOfReferences.size(); ++i) { We usually use something like this: for (unsigned i = 0; i < someLength; i++) https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:58: tokenStreamCSSValue->append(*CSSCustomIdentValue::create(m_listOfReferences[i])); I thought you were going to change this to be a CSSVariableReferenceValue? https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:18: class CORE_EXPORT CSSTokenStreamValueObserver : public GarbageCollectedMixin { What's this class for? It doesn't seem to be used for anything. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:26: public ValueIterable<String> { This can be on the previous line (blink doesn't have line length limits- see also the style guide: https://www.chromium.org/blink/coding-style) https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:39: String componentAtIndex(int index) { return m_listOfReferences.at(index); } Can this be renamed to referenceAtIndex or something else more descriptive? (componentAtIndex was used in CSSTransformValue because it contains CSSTransformComponents, aka components, so you shouldn't keep that name.) https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:41: Vector<String> listOfReferences() const { return m_listOfReferences; } Is this function used anywhere? https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:47: m_listOfReferences(listOfReferences) { } Constructor line breaking should be like this: MyClass::MyClass(Document* doc) : MySuperClass() , m_myMember(0) , m_doc(doc) { } https://www.chromium.org/blink/coding-style#TOC-Other-Punctuation
Description was changed from ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue BUG=545318 ========== to ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue BUG=545318 ==========
https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSStyleValue.h:32: TokenStreamType, On 2016/07/07 05:48:01, Eddy wrote: > These should be in alphabetical order (To before Tr) :) Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:57: for (size_t i = 0; i < m_listOfReferences.size(); ++i) { On 2016/07/07 05:48:01, Eddy wrote: > We usually use something like this: > for (unsigned i = 0; i < someLength; i++) Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:58: tokenStreamCSSValue->append(*CSSCustomIdentValue::create(m_listOfReferences[i])); On 2016/07/07 05:48:01, Eddy wrote: > I thought you were going to change this to be a CSSVariableReferenceValue? Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:18: class CORE_EXPORT CSSTokenStreamValueObserver : public GarbageCollectedMixin { On 2016/07/07 05:48:02, Eddy wrote: > What's this class for? It doesn't seem to be used for anything. Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:26: public ValueIterable<String> { On 2016/07/07 05:48:02, Eddy wrote: > This can be on the previous line (blink doesn't have line length limits- see > also the style guide: https://www.chromium.org/blink/coding-style) Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:39: String componentAtIndex(int index) { return m_listOfReferences.at(index); } On 2016/07/07 05:48:02, Eddy wrote: > Can this be renamed to referenceAtIndex or something else more descriptive? > (componentAtIndex was used in CSSTransformValue because it contains > CSSTransformComponents, aka components, so you shouldn't keep that name.) Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:41: Vector<String> listOfReferences() const { return m_listOfReferences; } On 2016/07/07 05:48:02, Eddy wrote: > Is this function used anywhere? Done. https://codereview.chromium.org/2122193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:47: m_listOfReferences(listOfReferences) { } On 2016/07/07 05:48:02, Eddy wrote: > Constructor line breaking should be like this: > > MyClass::MyClass(Document* doc) > : MySuperClass() > , m_myMember(0) > , m_doc(doc) > { > } > > https://www.chromium.org/blink/coding-style#TOC-Other-Punctuation Done.
meade@chromium.org changed reviewers: + timloh@chromium.org
lgtm https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:63: for (unsigned i = 0; i < m_listOfReferences.size(); ++i) { nit: i++ for consistency
meade@chromium.org changed reviewers: + shans@chromium.org
Hey Shane! Could you please have a look since Tim is away?
Anthony: Would be useful to have some notes in the description about what is still missing in this CL (e.g. it only deals with strings so far).
On 2016/07/11 at 07:03:45, meade wrote: > Anthony: Would be useful to have some notes in the description about what is still missing in this CL (e.g. it only deals with strings so far). LGTM, but I'm going to be changing at least the name of the TokenStreamValue pretty soon. Fine for this to go in as long as you're aware we might need to revisit.
Description was changed from ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue BUG=545318 ========== to ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>) BUG=545318 ==========
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:34: bool next(ScriptState* scriptState, String& value, ExceptionState& exceptionState) override You don't need to name arguments you don't reference, e.g. bool next(ScriptState*, String& result, ExceptionState&) https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:16: #include "core/css/parser/CSSPropertyParser.h" Some of these includes aren't necessary (also in the .cpp file). https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:35: String referenceAtIndex(int index) { return m_listOfReferences.at(index); } referenceAtIndex(..) const https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:37: size_t size() { return m_listOfReferences.size(); } size() const https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:40: CSSTokenStreamValue(Vector<String> listOfReferences) const Vector<>& (avoids an extra copy) https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:49: Vector<String> m_listOfReferences; listOf is generally avoided. These (going to be) a combination of string fragments and variable references, so calling these references is misleading. Maybe m_fragments is better.
https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:34: bool next(ScriptState* scriptState, String& value, ExceptionState& exceptionState) override On 2016/07/19 03:40:20, Timothy Loh wrote: > You don't need to name arguments you don't reference, e.g. > > bool next(ScriptState*, String& result, ExceptionState&) Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:63: for (unsigned i = 0; i < m_listOfReferences.size(); ++i) { On 2016/07/08 00:29:08, Eddy wrote: > nit: i++ for consistency Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:16: #include "core/css/parser/CSSPropertyParser.h" On 2016/07/19 03:40:20, Timothy Loh wrote: > Some of these includes aren't necessary (also in the .cpp file). Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:35: String referenceAtIndex(int index) { return m_listOfReferences.at(index); } On 2016/07/19 03:40:20, Timothy Loh wrote: > referenceAtIndex(..) const Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:37: size_t size() { return m_listOfReferences.size(); } On 2016/07/19 03:40:20, Timothy Loh wrote: > size() const Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:40: CSSTokenStreamValue(Vector<String> listOfReferences) On 2016/07/19 03:40:20, Timothy Loh wrote: > const Vector<>& (avoids an extra copy) Done. https://codereview.chromium.org/2122193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:49: Vector<String> m_listOfReferences; On 2016/07/19 03:40:20, Timothy Loh wrote: > listOf is generally avoided. These (going to be) a combination of string > fragments and variable references, so calling these references is misleading. > Maybe m_fragments is better. Done.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:22: bool next(ScriptState*, String& value, ExceptionState& exceptionState) override You're also not using exceptionState https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:27: String referenceAtIndex(int index) const { return m_fragments.at(index); } To be consistent, this should now be fragmentAtIndex.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Oh, also you'll need to update LayoutTests/http/tests/worklet/webexposed/global-interface-listing-paint-worklet-expected.txt and LayoutTests/webexposed/global-interface-listing-expected.txt LayoutTests/typedcssom/cssVariableReferenceValue.html is also failing (as discussed, you may need to delete this test)
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...
On 2016/07/19 05:00:06, Eddy wrote: > Oh, also you'll need to update > LayoutTests/http/tests/worklet/webexposed/global-interface-listing-paint-worklet-expected.txt > and LayoutTests/webexposed/global-interface-listing-expected.txt > > LayoutTests/typedcssom/cssVariableReferenceValue.html is also failing (as > discussed, you may need to delete this test) I think the test is not affected because in this CL I didn't change anything about the CSSVariableReferenceValue. Should I delete it now?
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: 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...
lgtm
The CQ bit was unchecked by meade@chromium.org
The CQ bit was checked by meade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shans@chromium.org Link to the patchset: https://codereview.chromium.org/2122193003/#ps140001 (title: "Add constructor")
The CQ bit was unchecked by meade@chromium.org
The CQ bit was checked by meade@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Test for the added class? https://codereview.chromium.org/2122193003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html (left): https://codereview.chromium.org/2122193003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html:1: <!DOCTYPE html> Did you mean to delete this test?
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/2122193003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:22: bool next(ScriptState*, String& value, ExceptionState& exceptionState) override On 2016/07/19 04:37:42, Eddy wrote: > You're also not using exceptionState Done. https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2122193003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:27: String referenceAtIndex(int index) const { return m_fragments.at(index); } On 2016/07/19 04:37:42, Eddy wrote: > To be consistent, this should now be fragmentAtIndex. Done. https://codereview.chromium.org/2122193003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html (left): https://codereview.chromium.org/2122193003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssVariableReferenceValue.html:1: <!DOCTYPE html> On 2016/07/20 06:18:17, Timothy Loh wrote: > Did you mean to delete this test? Actually this test is supposed to be removed later after the CSSVariableReferenceValue support the fallback (fallback will be of type CSSTokenStreamValue). After that, the test will be in C++ for the unit test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ok lgtm
The CQ bit was checked by meade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, shans@chromium.org Link to the patchset: https://codereview.chromium.org/2122193003/#ps160001 (title: "Add test for CSSTokenStreamValue")
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 CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>) BUG=545318 ========== to ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>) BUG=545318 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>) BUG=545318 ========== to ========== [Typed-OM] Add CSSTokenStreamValue Spec: https://drafts.css-houdini.org/css-typed-om/#csstokenstreamvalue In this CL, it only supports string value (instead of <string or CSSVariableReferenceValue>) BUG=545318 Committed: https://crrev.com/183c36161d9f0f580975c8be4b22464eaefa3542 Cr-Commit-Position: refs/heads/master@{#406781} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/183c36161d9f0f580975c8be4b22464eaefa3542 Cr-Commit-Position: refs/heads/master@{#406781} |
