WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
229069
Addition of CSSUnparsedValue. (TypedOM)
https://bugs.webkit.org/show_bug.cgi?id=229069
Summary
Addition of CSSUnparsedValue. (TypedOM)
Qiaosong Zhou
Reported
2021-08-13 02:10:52 PDT
Addition of CSSUnparsedValue. (TypedOM)
Attachments
Patch
(39.49 KB, patch)
2021-08-13 02:32 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(45.11 KB, patch)
2021-08-16 22:25 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(69.52 KB, patch)
2021-08-24 12:13 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(42.62 KB, patch)
2021-08-24 17:30 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(144.18 KB, patch)
2021-08-24 18:36 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(144.29 KB, patch)
2021-08-24 19:06 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(144.18 KB, patch)
2021-08-24 19:20 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(1.09 MB, patch)
2021-08-24 21:50 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(144.19 KB, patch)
2021-08-25 12:33 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(140.04 KB, patch)
2021-08-29 00:19 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(139.04 KB, patch)
2021-08-29 00:52 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(139.13 KB, patch)
2021-08-29 02:13 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(138.14 KB, patch)
2021-08-29 03:10 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(84.29 KB, patch)
2021-08-29 14:44 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(83.94 KB, patch)
2021-08-30 14:18 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(84.08 KB, patch)
2021-08-30 15:53 PDT
,
Qiaosong Zhou
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(84.13 KB, patch)
2021-08-30 15:58 PDT
,
Qiaosong Zhou
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(84.11 KB, patch)
2021-08-30 16:02 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Patch
(1.16 KB, patch)
2021-08-31 03:40 PDT
,
Qiaosong Zhou
no flags
Details
Formatted Diff
Diff
Show Obsolete
(17)
View All
Add attachment
proposed patch, testcase, etc.
Qiaosong Zhou
Comment 1
2021-08-13 02:32:52 PDT
Created
attachment 435474
[details]
Patch
Qiaosong Zhou
Comment 2
2021-08-16 22:25:07 PDT
Created
attachment 435664
[details]
Patch
Alex Christensen
Comment 3
2021-08-17 11:45:19 PDT
Comment on
attachment 435664
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=435664&action=review
This looks mostly reasonable. Add some test expectation changes to this patch and polish a few things first.
> Source/WebCore/bindings/js/JSCSSStyleValueCustom.cpp:41 > + if (value->getType() == CSSStyleValueType::CSSUnitValue)
using a switch statement instead would make it so getType is only called once.
> Source/WebCore/css/typedom/CSSNumericValue.h:37 > + CSSStyleValueType getType() const { return CSSStyleValueType::CSSNumericValue; }
Should this be marked as "override" or "final", or should we remove it?
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:42 > +ExceptionOr<Ref<CSSOMVariableReferenceValue>> CSSOMVariableReferenceValue::create(String variable, RefPtr<CSSUnparsedValue>&& fallback)
String&& or const String&
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:50 > +ExceptionOr<void> CSSOMVariableReferenceValue::setVariable(String variable)
ditto
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:47 > + String variable() const { return m_variable; }
const String&
> Source/WebCore/css/typedom/CSSStyleValue.cpp:46 > +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(String property, String cssText, bool parseMultiple)
More String&& or const String& in this file.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:65 > + auto parseResult = CSSParser::parseValue(styleDeclaration, propertyID, cssText, true, HTMLStandardMode);
Could you give "true" a name so we know what it means just by looking at the code?
> Source/WebCore/css/typedom/CSSStyleValue.cpp:75 > + auto cssValue = styleDeclaration->getPropertyCSSValue(propertyID); > + auto valueType = cssValue->cssValueType(); > + > + if (!cssValue) > + return Exception { SyntaxError };
It looks like some of the tests crash because of this. You need to move the null check to before you use cssValue.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:106 > + auto res = WTFMove(parseResult.releaseReturnValue().at(0));
This needs a bounds check in case an empty Vector is returned.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:62 > + if (WTF::holds_alternative<String>(segment)) > + serialization.append(WTF::get<String>(segment)); > + else > + serialization.append(WTF::get<RefPtr<CSSOMVariableReferenceValue>>(segment)->toString());
This should probably use WTF::visit instead of this in case someone adds another type to the variant. Currently we just assume it's either a String or a RefPtr. If someone added another type, it would crash trying to get a RefPtr, but if you use WTF::visit it would make a compile failure.
> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:51 > + CSSValue* valuePtr = value.get();
Don't store this value in a local variable. Just use value.get() at all the places you use this.
> Source/WebCore/dom/StyledElement.cpp:112 > + return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);
This can be WTFMove(value) instead of value.copyRef() to reduce ref churn because you don't use value after this call. Same 5 more times below this.
Chris Dumez
Comment 4
2021-08-17 12:08:18 PDT
Comment on
attachment 435664
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=435664&action=review
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:44 > + if (!variable.startsWith("--"_s))
There is a startsWith() overload that takes in a `const char*` so no need to construct a WTFString here by using _s suffix. This is actually less efficient.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:45 > + return Exception { TypeError };
Please provide an exception message as second parameter.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:52 > + if (!variable.startsWith("--"_s))
ditto.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:53 > + return Exception { TypeError };
ditto.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:61 > + return makeString("var("_s, m_variable, (m_fallback ? ", "_s : ""_s), (m_fallback ? m_fallback->toString() : ""_s), ")"_s);
Drop all the _s. makeString() works fine with const char*.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:28 > +#if ENABLE(CSS_TYPED_OM)
Do we really need a build flag for this? Wouldn't a runtime flag be sufficient?
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:42 > + static ExceptionOr<Ref<CSSOMVariableReferenceValue>> create(String, RefPtr<CSSUnparsedValue>&& fallback = { });
Also, please pass the String parameter as a `String&&` or a `const String&`.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:48 > + RefPtr<CSSUnparsedValue> fallback() const { return m_fallback.copyRef(); }
This does ref-counting churn, you could return a CSSUnparsedValue* and do `return m_fallback.get()`. Lets the call site decide if they want to ref it or not.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:51 > + explicit CSSOMVariableReferenceValue(String variable, RefPtr<CSSUnparsedValue>&& fallback)
explicit is not needed given that you have 2 non-optional parameters. Also, please pass the String parameter as a `String&&` or a `const String&`. Also, this constructor could be moved to the cpp file.
> Source/WebCore/css/typedom/CSSStyleImageValue.h:50 > + CSSStyleValueType getType() const final { return CSSStyleValueType::CSSStyleImageValue; }
Should probably be private as it doesn't make sense to call getType on a CSSStyleImageValue.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:53 > + property.convertToASCIILowercase();
This does nothing. convertToASCIILowercase() is const and returns a value that you ignore.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:67 > + return Exception { SyntaxError };
Please provide an exception message as second parameter.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:69 > +
Extra blank line here.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:70 > + // RefPtr<CSSValue>
Please drop this comment.
>> Source/WebCore/css/typedom/CSSStyleValue.cpp:75 >> + return Exception { SyntaxError }; > > It looks like some of the tests crash because of this. You need to move the null check to before you use cssValue.
Please provide an exception message as second parameter.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:79 > + if (valueType == CSSValue::CSS_VALUE_LIST) {
Why not use if (is<CSSValueList>(cssValue.get())) ?
> Source/WebCore/css/typedom/CSSStyleValue.cpp:80 > + ASSERT(is<CSSValueList>(cssValue.get()));
Then we wouldn't need this.
>> Source/WebCore/css/typedom/CSSStyleValue.cpp:106 >> + auto res = WTFMove(parseResult.releaseReturnValue().at(0)); > > This needs a bounds check in case an empty Vector is returned.
Also, unnecessary local variable here.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:136 > + // FIXME: Add Reification control flow. Returns nullptr if failed
Please end comments with a period.
> Source/WebCore/css/typedom/CSSStyleValue.h:73 > + CSSPropertyID m_cssPropertyID;
Please add a blank line before the data members.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:70 > + return Exception { RangeError };
Please provide an exception message as second parameter.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:71 > + return ExceptionOr<CSSUnparsedSegment> { CSSUnparsedSegment { m_segments[index] } };
ExceptionOr<CSSUnparsedSegment> { } should not be needed.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:77 > + return Exception { RangeError };
ditto.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:79 > + return ExceptionOr<CSSUnparsedSegment> { CSSUnparsedSegment { val } };
ditto.
> Source/WebCore/css/typedom/CSSUnparsedValue.h:41 > +class CSSUnparsedValue : public CSSStyleValue {
Should this class be marked as final?
> Source/WebCore/css/typedom/CSSUnparsedValue.h:49 > + CSSStyleValueType getType() const final { return CSSStyleValueType::CSSUnparsedValue; }
Should probably be private.
> Source/WebCore/css/typedom/CSSUnparsedValue.h:51 > + unsigned long length() const { return m_segments.size(); }
unsigned long is not the right return type here. We should return a size_t, live Vector::size().
> Source/WebCore/css/typedom/CSSUnparsedValue.h:53 > + ExceptionOr<CSSUnparsedSegment> item(unsigned long);
I would use size_t here to match.
> Source/WebCore/css/typedom/CSSUnparsedValue.h:55 > + ExceptionOr<CSSUnparsedSegment> setItem(unsigned long, CSSUnparsedSegment);
ditto.
Radar WebKit Bug Importer
Comment 5
2021-08-20 02:11:16 PDT
<
rdar://problem/82162438
>
Qiaosong Zhou
Comment 6
2021-08-24 12:13:04 PDT
Created
attachment 436317
[details]
Patch
Qiaosong Zhou
Comment 7
2021-08-24 12:14:41 PDT
Still in debugging phase. A lot of comments, commented-out code, and WTFLogs. Will be removed in the future.
Qiaosong Zhou
Comment 8
2021-08-24 17:30:41 PDT
Created
attachment 436352
[details]
Patch
Qiaosong Zhou
Comment 9
2021-08-24 18:36:41 PDT
Created
attachment 436360
[details]
Patch
Qiaosong Zhou
Comment 10
2021-08-24 19:06:09 PDT
Created
attachment 436363
[details]
Patch
Qiaosong Zhou
Comment 11
2021-08-24 19:20:44 PDT
Created
attachment 436364
[details]
Patch
Qiaosong Zhou
Comment 12
2021-08-24 21:50:24 PDT
Created
attachment 436368
[details]
Patch
Alex Christensen
Comment 13
2021-08-24 22:08:33 PDT
View in context:
https://bugs.webkit.org/attachment.cgi?id=436364&action=review
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:61 > + return makeString("var(", m_variable, (m_fallback ? ", " : emptyString()), (m_fallback ? m_fallback->toString() : emptyString()), ")");
I think it would actually be a little more efficient if the first ternary expression were (m_fallback ? ", "_s : ""_s) so that the result of the expression is an ASCIILiteral instead of a String, possibly allocated and copied from a const char*
> Source/WebCore/css/typedom/CSSStyleValue.cpp:52 > + if (isCustomPropertyName(property))
I'm guessing this might mean to be if (!isCustomPropertyName(cssProperty) Otherwise, you're always passing in a null string.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:99 > + auto parseResult = parseStyleValue(property, cssText, false);
Could you give "false" a name? constexpr auto <insert name here> = false; auto parseResult = parseStyleValue(property, cssText, <insert name here>); That makes the code easier to read.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:106 > + // Returned vector should not be empty. If parsing failed, an exception should be returned. > + ASSERT(!returnValue.isEmpty());
Can parsing succeed and return an empty vector? If not, this should probably be a RELEASE_ASSERT.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:113 > + return parseStyleValue(property, cssText, true);
Ditto with named boolean.
> Source/WebCore/css/typedom/CSSStyleValue.h:40 > +enum class CSSStyleValueType: uint8_t {
nit: space before the colon.
> Source/WebCore/css/typedom/CSSStyleValue.h:67 > + // FIXME: probably should remove this in the future
More specific is better. // FIXME: Remove this when CSSStyleValue::reifyValue is implemented.
> Source/WebCore/css/typedom/CSSStyleValue.h:71 > CSSStyleValue() = default;
This constructor should also have a FIXME to remove it.
> Source/WebCore/css/typedom/CSSStyleValue.h:74 > + CSSPropertyID m_cssPropertyID;
As the code stands now, this is uninitialized memory when the default constructor is called. Probably not too big a deal because we're about to remove the default constructor, but still not great. Would it be too disruptive to just return nullptr from CSSStyleValue::reifyValue and remove this class's default constructor instead of adding an empty CSSStyleValue::create()?
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:57 > + String serialization = emptyString();
This should use a StringBuilder to avoid an O(n^2) algorithm with copying each substring as it's assembled.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:62 > + }, [&] (const RefPtr<CSSOMVariableReferenceValue>& value) { > + serialization.append(value->toString());
Can value be null here? If so, we should add a null check. If not, can we change the Variant type to be a Ref instead of a RefPtr?
> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:46 > +RefPtr<CSSStyleValue> StylePropertyMapReadOnly::reifyValue(RefPtr<CSSValue>&& value, const String&, Document& document, Element*)
Can we remove the unused Element*?
> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:50 > +
nit: remove extra whitespace.
> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:45 > +
nit: whitespace
> Source/WebCore/dom/StyledElement.cpp:112 > + return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);
WTFMove(value) to reduce ref churn since we won't use value after this line.
> Source/WebCore/html/CustomPaintImage.cpp:69 > + return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);
WTFMove(value)
> Source/WebCore/html/CustomPaintImage.cpp:77 > + return StylePropertyMapReadOnly::reifyValue(value.copyRef(), name, element.document(), &element);
WTFMove(value)
> Source/WebCore/html/CustomPaintImage.cpp:93 > + return StylePropertyMapReadOnly::customPropertyValueOrDefault(name, element.document(), value.copyRef(), &element);
WTFMove(value)
> Source/WebCore/html/CustomPaintImage.cpp:101 > + return StylePropertyMapReadOnly::reifyValue(value.copyRef(), name, element.document(), &element);
WTFMove(value)
Qiaosong Zhou
Comment 14
2021-08-25 12:33:53 PDT
Created
attachment 436410
[details]
Patch
Qiaosong Zhou
Comment 15
2021-08-29 00:19:11 PDT
Created
attachment 436731
[details]
Patch
Qiaosong Zhou
Comment 16
2021-08-29 00:52:15 PDT
Created
attachment 436732
[details]
Patch
Qiaosong Zhou
Comment 17
2021-08-29 02:13:09 PDT
Created
attachment 436733
[details]
Patch
Qiaosong Zhou
Comment 18
2021-08-29 03:10:46 PDT
Created
attachment 436734
[details]
Patch
Qiaosong Zhou
Comment 19
2021-08-29 14:44:05 PDT
Created
attachment 436750
[details]
Patch
EWS Watchlist
Comment 20
2021-08-29 14:45:03 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see
https://trac.webkit.org/wiki/WPTExportProcess
Alex Christensen
Comment 21
2021-08-30 10:36:21 PDT
Comment on
attachment 436750
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=436750&action=review
Great! I think once these final few comments are addressed it'll be ready to land.
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:48 > + CSSUnparsedValue* fallback() const { return m_fallback.get(); }
This should probably be one of these instead: const CSSUnparsedValue* fallback() const; CSSUnparsedValue* fallback();
> Source/WebCore/css/typedom/CSSStyleValue.cpp:67 > + bool important = true;
constexpr
> Source/WebCore/css/typedom/CSSStyleValue.cpp:84 > + auto reifiedValue = CSSStyleValue::reifyValue(propertyID, currentValue.copyRef()); > + if (reifiedValue)
Nit: these can go on the same line: if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID, currentValue.copyRef()))
> Source/WebCore/css/typedom/CSSStyleValue.cpp:90 > + } else { > + auto reifiedValue = CSSStyleValue::reifyValue(propertyID, cssValue.copyRef()); > + if (reifiedValue)
else if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID, cssValue.copyRef()))
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:51 > +CSSUnparsedValue::CSSUnparsedValue(const Vector<CSSUnparsedSegment>& segments)
Can this be a Vector<CSSUnparsedSegment>&& instead? That would prevent a Vector copy if it were.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:52 > + : CSSStyleValue(nullptr)
This line is probably unnecessary since CSSStyleValue has a default constructor that does exactly this.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:65 > +void CSSUnparsedValue::serialize(StringBuilder& builder) const
I don't think this needs to be a separate function unless you're planning to use it elsewhere in the future.
> Source/WebCore/css/typedom/CSSUnparsedValue.h:32 > +#include <wtf/Variant.h>
This is probably not needed
> Source/WebCore/css/typedom/CSSUnparsedValue.h:33 > +#include <wtf/text/StringBuilder.h>
This is not needed. <wtf/Forward.h> should have enough.
> Source/WebCore/css/typedom/CSSUnparsedValue.h:52 > + ExceptionOr<CSSUnparsedSegment> setItem(size_t, CSSUnparsedSegment);
Can this be a CSSUnparsedSegment&& or a const CSSUnparsedSegment&? As it stands now, you're probably unnecessarily instantiating a whole object when calling this function.
> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:58 > // FIXME: should use raw CSSStyleValue
I think this comment can be removed since we're using CSSStyleValue now.
Chris Dumez
Comment 22
2021-08-30 10:47:01 PDT
Comment on
attachment 436750
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=436750&action=review
> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:39 > +class CSSOMVariableReferenceValue: public RefCounted<CSSOMVariableReferenceValue> {
missing space before ':'
> Source/WebCore/css/typedom/CSSStyleValue.cpp:99 > + constexpr auto parseMultiple = false;
bool is just as long as auto.
> Source/WebCore/css/typedom/CSSStyleValue.cpp:107 > + if (!returnValue.size())
returnValue.isEmpty()
> Source/WebCore/css/typedom/CSSStyleValue.cpp:115 > + constexpr auto parseMultiple = true;
bool is just as long as auto.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:83 > +ExceptionOr<CSSUnparsedSegment> CSSUnparsedValue::setItem(size_t index, CSSUnparsedSegment val)
CSSUnparsedSegment should not be passed by value. `const CSSUnparsedSegment&` or `CSSUnparsedSegment&&`. Probably the latter.
> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > + return CSSUnparsedSegment { val };
doesn't `return val;` work? or `return m_segments[index];` if you make the change I suggested above.
Qiaosong Zhou
Comment 23
2021-08-30 14:18:51 PDT
Created
attachment 436812
[details]
Patch
Qiaosong Zhou
Comment 24
2021-08-30 14:30:58 PDT
(In reply to Chris Dumez from
comment #22
)
> > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > > + return CSSUnparsedSegment { val }; > > doesn't `return val;` work? or `return m_segments[index];` if you make the > change I suggested above.
It doesn't work. The compiler throws a type error. It is not able to infer the ExceptionOr type for some reason.
Qiaosong Zhou
Comment 25
2021-08-30 15:53:10 PDT
Created
attachment 436820
[details]
Patch
Qiaosong Zhou
Comment 26
2021-08-30 15:58:58 PDT
Created
attachment 436821
[details]
Patch
Qiaosong Zhou
Comment 27
2021-08-30 16:02:45 PDT
Created
attachment 436822
[details]
Patch
EWS
Comment 28
2021-08-30 18:08:56 PDT
Committed
r281786
(
241123@main
): <
https://commits.webkit.org/241123@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 436822
[details]
.
Darin Adler
Comment 29
2021-08-30 18:52:08 PDT
Comment on
attachment 436822
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=436822&action=review
> Source/WebCore/css/typedom/CSSStyleValue.h:58 > + virtual CSSStyleValueType getType() const { return CSSStyleValueType::CSSStyleValue; }
Is this name web-exposed? If not, then it should *not* have "get" in its name to match WebKit coding style. If it is web-exposed then there may be some other style guide that overrides the WebKt one.
Qiaosong Zhou
Comment 30
2021-08-31 03:40:13 PDT
Reopening to attach new patch.
Qiaosong Zhou
Comment 31
2021-08-31 03:40:15 PDT
Created
attachment 436870
[details]
Patch
EWS
Comment 32
2021-08-31 03:42:50 PDT
qiaosong_zhou@apple.com
does not have committer permissions according to
https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json
. Rejecting
attachment 436870
[details]
from commit queue.
EWS
Comment 33
2021-08-31 10:50:02 PDT
Committed
r281809
(
241145@main
): <
https://commits.webkit.org/241145@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 436870
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug