Bug 229705 - Add support for CSSKeywordValue in CSS Typed OM
Summary: Add support for CSSKeywordValue in CSS Typed OM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Qiaosong Zhou
URL:
Keywords: InRadar
Depends on:
Blocks: 175733
  Show dependency tree / graph
 
Reported: 2021-08-31 03:09 PDT by Qiaosong Zhou
Modified: 2021-09-02 12:40 PDT (History)
18 users (show)

See Also:

Attachments
Patch (15.83 KB, patch)
2021-08-31 03:10 PDT, Qiaosong Zhou 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2021-08-31 03:33 PDT, Qiaosong Zhou 		no flags Details | Formatted Diff | Diff
Patch (254.92 KB, patch)
2021-09-01 03:54 PDT, Qiaosong Zhou 		no flags Details | Formatted Diff | Diff
Patch (254.86 KB, patch)
2021-09-01 03:57 PDT, Qiaosong Zhou 		no flags Details | Formatted Diff | Diff
Patch (254.39 KB, patch)
2021-09-01 04:10 PDT, Qiaosong Zhou 		no flags Details | Formatted Diff | Diff
Patch (254.39 KB, patch)
2021-09-01 15:55 PDT, Qiaosong Zhou 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Qiaosong Zhou 2021-08-31 03:09:32 PDT 
Add support for CSSKeywordValue in CSS Typed OM
Comment 1 Qiaosong Zhou 2021-08-31 03:10:00 PDT 
Created attachment 436866 [details]
Patch
Comment 2 Qiaosong Zhou 2021-08-31 03:33:51 PDT 
Created attachment 436869 [details]
Patch
Comment 3 Alex Christensen 2021-08-31 11:37:53 PDT 
Comment on attachment 436869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436869&action=review

This is a wonderful patch.

> Source/WebCore/css/typedom/CSSKeywordValue.cpp:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2021

> Source/WebCore/css/typedom/CSSKeywordValue.cpp:44
> +    if (value.isNull() || value.isEmpty())

value.isEmpty returns true if value is null, so only one check is needed.

> Source/WebCore/css/typedom/CSSKeywordValue.cpp:52
> +    if (value.isNull() || value.isEmpty())

ditto

> Source/WebCore/css/typedom/CSSKeywordValue.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2021

> Source/WebCore/css/typedom/CSSKeywordValue.h:30
> +#include "CSSKeywordValue.h"

This shouldn't include itself.

> Source/WebCore/css/typedom/CSSKeywordValue.h:32
> +#include <wtf/RefCounted.h>

This doesn't look needed.

> Source/WebCore/css/typedom/CSSKeywordValue.h:44
> +    String& value() { return m_value; }

const String& value() const

> Source/WebCore/css/typedom/CSSKeywordValue.idl:2
> +* Copyright (C) 2018 Apple Inc.  All rights reserved.

2021
Comment 4 Qiaosong Zhou 2021-09-01 03:54:53 PDT 
Created attachment 437010 [details]
Patch
Comment 5 Qiaosong Zhou 2021-09-01 03:57:07 PDT 
Created attachment 437011 [details]
Patch
Comment 6 Qiaosong Zhou 2021-09-01 04:10:58 PDT 
Created attachment 437012 [details]
Patch
Comment 7 Alex Christensen 2021-09-01 09:31:33 PDT 
Comment on attachment 437012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437012&action=review

And three copyright years need updating, but otherwise this looks ready to go.

> Source/WebCore/css/typedom/CSSKeywordValue.h:43
> +    const String& value() { return m_value; }

This needs a const between () and { to indicate that calling the function doesn't mutate the object.  The first const indicates that you can't mutate the returned reference.
Comment 8 Qiaosong Zhou 2021-09-01 15:55:16 PDT 
Created attachment 437075 [details]
Patch
Comment 9 EWS 2021-09-02 12:39:12 PDT 
Committed r281947 (241255@main): <https://commits.webkit.org/241255@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437075 [details].
Comment 10 Radar WebKit Bug Importer 2021-09-02 12:40:33 PDT 
<rdar://problem/82688747>