Limit scope getter to predefined values (issue 2376593003 by rob.buis@samsung.com)

1 view
Skip to first unread message

rob....@samsung.com

unread,
Sep 27, 2016, 6:44:47 PM9/27/16
to tk...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org
Reviewers: tkent
CL: https://codereview.chromium.org/2376593003/

Message:
PTAL.

BTW this also fixes the scope tests in:
http://w3c-test.org/html/dom/reflection-tabular.html

Description:
Limit scope getter to predefined values

The scope attribute is limited to four predefined keywords [1], make
sure the idl binding will only return one of those keywords or
empty string (for default).

Behavior matches Safari and Firefox.

[1] https://html.spec.whatwg.org/#the-th-element:attr-th-scope

Affected files (+27, -4 lines):
M third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
M third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
M third_party/WebKit/Source/core/html/HTMLTableCellElement.h
M third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
M third_party/WebKit/Source/core/html/HTMLTableCellElement.idl


Index: third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
diff --git a/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
index b7ea08dfccafdae2ab2e51eccb168d0dc071f9db..e11458e94bdcf2d5d70af14ad7b7c4010eb58eb5 100644
--- a/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
+++ b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
@@ -249,7 +249,7 @@ TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.ch
TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.chOff]
TEST SUCCEEDED: The value was the empty string. [tested HTMLTableCellElement.headers]
TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.height]
-TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.scope]
+TEST SUCCEEDED: The value was the empty string. [tested HTMLTableCellElement.scope]
TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.vAlign]
TEST SUCCEEDED: The value was the string 'null'. [tested HTMLTableCellElement.width]

Index: third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
diff --git a/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
index 35acafb6175fe22b89a24b1a7e1a77cc43d622f7..54f94ac1a0ff159be76a3f9cb6dc1a61133cdbb4 100644
--- a/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
+++ b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
@@ -577,7 +577,7 @@
{name: 'chOff', expectedNull: 'null'},
{name: 'headers', expectedNull: ''},
{name: 'height', expectedNull: 'null'},
- {name: 'scope', expectedNull: 'null'},
+ {name: 'scope', expectedNull: ''},
{name: 'vAlign', expectedNull: 'null'},
{name: 'width', expectedNull: 'null'}
]
Index: third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp b/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
index 26553e6f9bcc05a40c50c723c3a041f695542430..58d6ae5e589aa7ff66aef7c9910963dbba00cf13 100644
--- a/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
@@ -174,7 +174,29 @@ void HTMLTableCellElement::setRowSpan(unsigned n)

const AtomicString& HTMLTableCellElement::scope() const
{
- return fastGetAttribute(scopeAttr);
+ const AtomicString& scopeValue = fastGetAttribute(scopeAttr);
+ if (equalIgnoringCase(scopeValue, "row")) {
+ DEFINE_STATIC_LOCAL(const AtomicString, row, ("row"));
+ return row;
+ }
+ if (equalIgnoringCase(scopeValue, "col")) {
+ DEFINE_STATIC_LOCAL(const AtomicString, col, ("col"));
+ return col;
+ }
+ if (equalIgnoringCase(scopeValue, "rowgroup")) {
+ DEFINE_STATIC_LOCAL(const AtomicString, rowgroup, ("rowgroup"));
+ return rowgroup;
+ }
+ if (equalIgnoringCase(scopeValue, "colgroup")) {
+ DEFINE_STATIC_LOCAL(const AtomicString, colgroup, ("colgroup"));
+ return colgroup;
+ }
+ return emptyAtom;
+}
+
+void HTMLTableCellElement::setScope(const AtomicString& value)
+{
+ setAttribute(scopeAttr, value);
}

} // namespace blink
Index: third_party/WebKit/Source/core/html/HTMLTableCellElement.h
diff --git a/third_party/WebKit/Source/core/html/HTMLTableCellElement.h b/third_party/WebKit/Source/core/html/HTMLTableCellElement.h
index 58c47f0d3ab15a5cf169dfbd7d8f6b9282e42e05..1caed31bcee898d1bc29f1d76688380a0d4c8b29 100644
--- a/third_party/WebKit/Source/core/html/HTMLTableCellElement.h
+++ b/third_party/WebKit/Source/core/html/HTMLTableCellElement.h
@@ -49,6 +49,7 @@ public:
const AtomicString& headers() const;
void setRowSpan(unsigned);
const AtomicString& scope() const;
+ void setScope(const AtomicString&);

private:
HTMLTableCellElement(const QualifiedName&, Document&);
Index: third_party/WebKit/Source/core/html/HTMLTableCellElement.idl
diff --git a/third_party/WebKit/Source/core/html/HTMLTableCellElement.idl b/third_party/WebKit/Source/core/html/HTMLTableCellElement.idl
index c029aef2c5980150fde538fe795c9264aacad39f..d7adc87977422c1db87f4f1f372ce600373b913b 100644
--- a/third_party/WebKit/Source/core/html/HTMLTableCellElement.idl
+++ b/third_party/WebKit/Source/core/html/HTMLTableCellElement.idl
@@ -47,5 +47,5 @@ interface HTMLTableCellElement : HTMLElement {
// respectively. HTMLTableHeaderCellElement has the abbr and scope
// attributes, while HTMLTableDataCellElement has only abbr.
[CEReactions, Reflect] attribute DOMString abbr;
- [CEReactions, Reflect] attribute DOMString scope;
+ [CEReactions] attribute DOMString scope;
};


tk...@chromium.org

unread,
Sep 27, 2016, 8:23:08 PM9/27/16
to rob....@samsung.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

rob....@samsung.com

unread,
Sep 28, 2016, 9:43:36 AM9/28/16
to tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

https://codereview.chromium.org/2376593003/diff/20001/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
File third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp
(right):

https://codereview.chromium.org/2376593003/diff/20001/third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp#newcode178
third_party/WebKit/Source/core/html/HTMLTableCellElement.cpp:178: if
(equalIgnoringCase(scopeValue, "row")) {
On 2016/09/28 00:23:08, tkent wrote:
> equalIgnoringCase should be equalIgnoringASCIICase.
>
>
https://html.spec.whatwg.org/multipage/infrastructure.html#enumerated-attribute

tk...@chromium.org

unread,
Sep 28, 2016, 9:52:25 AM9/28/16
to rob....@samsung.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
Do we have tests for scope IDL getter? e.g. IDL scope should return "row" for
scope="ROW".
reflection-tabular.html includes such test, but we don't import it and we need
test coverage.


https://codereview.chromium.org/2376593003/

rob....@samsung.com

unread,
Sep 28, 2016, 10:06:28 AM9/28/16
to tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
On 2016/09/28 13:52:04, tkent wrote:
> Do we have tests for scope IDL getter? e.g. IDL scope should return "row" for
> scope="ROW".
> reflection-tabular.html includes such test, but we don't import it and we need
> test coverage.

I think dom/html/level2/html/table29.js comes closest but it has only one simple
test which we pass even before my patch. I guess more subtests could be added.

reflection-tabular.hml would have all the tests we need (the other reflection
test files should be useful too). I just wonder if there is any progress with
crbug.com/490939?



https://codereview.chromium.org/2376593003/

tk...@chromium.org

unread,
Sep 28, 2016, 10:28:17 PM9/28/16
to rob....@samsung.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
On 2016/09/28 at 14:06:27, rob.buis wrote:
> reflection-tabular.hml would have all the tests we need (the other reflection
test files should be useful too). I just wonder if there is any progress with
crbug.com/490939?

No progress.
I don't remember I tried LayoutTests/SlowTests or not. If SlowTests resolves
the flakiness, we don't need to split tests.

If you'd like to land this soon, I recommend to add new tests (or copy a part of
reflection-tabular.html) to this CL, and add a comment about it to
crbug.com/490939.


https://codereview.chromium.org/2376593003/

rob....@samsung.com

unread,
Sep 29, 2016, 9:43:26 AM9/29/16
to tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
Sounds great, I added the test and can add a comment to crbug.com/490939 once it
lands.

https://codereview.chromium.org/2376593003/

tk...@chromium.org

unread,
Sep 29, 2016, 7:15:56 PM9/29/16
to rob....@samsung.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
lgtm




https://codereview.chromium.org/2376593003/diff/60001/third_party/WebKit/LayoutTests/fast/table/table-cell-scope-getter.html
File
third_party/WebKit/LayoutTests/fast/table/table-cell-scope-getter.html
(right):

https://codereview.chromium.org/2376593003/diff/60001/third_party/WebKit/LayoutTests/fast/table/table-cell-scope-getter.html#newcode8
third_party/WebKit/LayoutTests/fast/table/table-cell-scope-getter.html:8:
td = document.createElement('td');
indentation looks bad.

https://codereview.chromium.org/2376593003/

tk...@chromium.org

unread,
Sep 30, 2016, 2:51:03 AM9/30/16
to rob....@samsung.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

rob....@samsung.com

unread,
Sep 30, 2016, 8:27:20 AM9/30/16
to tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
On 2016/09/30 06:51:02, tkent wrote:
> I imported reflection-tabular.html this morning, and it was not flaky. This
CL
> may reply on it.
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=wpt%2Fhtml%2Fdom%2Freflection-tabular

Ok I'll try to add it instead of fast/table/table-cell-scope-getter.html.

https://codereview.chromium.org/2376593003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 10:31:11 AM9/30/16
to rob....@samsung.com, tk...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 11:52:08 AM9/30/16
to rob....@samsung.com, tk...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
Try jobs failed on following builders:
android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/39952)

https://codereview.chromium.org/2376593003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 11:58:24 AM9/30/16
to rob....@samsung.com, tk...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 1:35:04 PM9/30/16
to rob....@samsung.com, tk...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
Committed patchset #5 (id:80001)

https://codereview.chromium.org/2376593003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 1:38:41 PM9/30/16
to rob....@samsung.com, tk...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org
Patchset 5 (id:??) landed as
https://crrev.com/f3f53a039bcdda5bfda56f1f5ac5fd6f84b3e704
Cr-Commit-Position: refs/heads/master@{#422144}

https://codereview.chromium.org/2376593003/
Reply all
Reply to author
Forward
0 new messages