Issue 1874623002: Implement iterable<DOMString, (StyleValue or sequence<StyleValue>)> for InlineStylePropertyMap

Issue 1874623002: Implement iterable<DOMString, (StyleValue or sequence<StyleValue>)> for InlineStylePropertyMap (Closed)

Created:
4 years, 8 months ago by meade_UTC10

Modified:
4 years, 6 months ago

Reviewers:
Timothy Loh

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@computed-stylepropertymap-2

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implement iterable<DOMString, (StyleValue or sequence<StyleValue>)> for InlineStylePropertyMap Spec: https://drafts.css-houdini.org/css-typed-om/#the-stylepropertymap BUG=545318 Committed: https://crrev.com/f01caa59865aec4c79d128e14f4d9fc605e02916 Cr-Commit-Position: refs/heads/master@{#398190}

Patch Set 4 : Sync to head again #

Patch Set 5 : Fix branch parenting issue #

Patch Set 6 : Adjust formatting #

Total comments: 10

Patch Set 7 : Address review comments #

Total comments: 6

Patch Set 8 : Address review comments #

Patch Set 10 : Update LayoutTests #

Created: 4 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+230 lines, -3 lines)			Patch
M	third_party/WebKit/LayoutTests/typedcssom/computedstyle/computedStylePropertyMap.html	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+93 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration-expected.txt	View	1 2 3 4 5 6	1 chunk	+8 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+63 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/cssom/ComputedStylePropertyMap.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.h	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+19 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/cssom/StylePropertyMap.h	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/cssom/StylePropertyMap.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+39 lines, -0 lines	0 comments	Download

Depends on Patchset:

Issue 1849653003 Patch 390001

Messages

Total messages: 16 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Timothy Loh

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html (right): https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html#newcode1 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:1: <!DOCTYPE html> Can we have a test (multiple tests?) ...

4 years, 8 months ago (2016-04-15 05:27:53 UTC) #3

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html
(right):

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:1:
<!DOCTYPE html>
Can we have a test (multiple tests?) where we modify the styleMap (or old
CSSStyleDeclaration) while iterating? Probably in a separate file since this is
getting sort of big already.

for (let value of testElement.styleMap.entries())

? Seems like we should test using the iterator normally at least once :-)

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:78:
// TODO(meade): These will change to StyleValues once those values are
supported.
IMO it's better to just write the tests to fail now and check in a failing
expectation file. I think failing expectation files are a more obvious
indication of work that needs to be done than TODOs in tests.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:23: explicit
InlineStylePropertyMapIterationSource(HeapVector<std::pair<String,
StyleValueOrStyleValueSequence>> values)
let's just make a using declaration for the pair somewhere, so we don't have to
keep tying out the name.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:166:
InlineStylePropertyMap::IterationSource*
InlineStylePropertyMap::startIteration(ScriptState* scriptState, ExceptionState&
exceptionState)
can we make something that works for all the maps?

meade_UTC10

4 years, 7 months ago (2016-04-27 05:39:01 UTC) #4

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html
(right):

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:1:
<!DOCTYPE html>
On 2016/04/15 05:27:53, Timothy Loh wrote:
> Can we have a test (multiple tests?) where we modify the styleMap (or old
> CSSStyleDeclaration) while iterating? Probably in a separate file since this
is
> getting sort of big already.

Done.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:67:
while (!entry.done) {
On 2016/04/15 05:27:53, Timothy Loh wrote:
> Can this one just be
> 
> for (let value of testElement.styleMap.entries())
> 
> ? Seems like we should test using the iterator normally at least once :-)

Done.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iteration.html:78:
// TODO(meade): These will change to StyleValues once those values are
supported.
On 2016/04/15 05:27:53, Timothy Loh wrote:
> IMO it's better to just write the tests to fail now and check in a failing
> expectation file. I think failing expectation files are a more obvious
> indication of work that needs to be done than TODOs in tests.

Done.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp
(right):

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:23: explicit
InlineStylePropertyMapIterationSource(HeapVector<std::pair<String,
StyleValueOrStyleValueSequence>> values)
On 2016/04/15 05:27:53, Timothy Loh wrote:
> let's just make a using declaration for the pair somewhere, so we don't have
to
> keep tying out the name.

Done.

https://codereview.chromium.org/1874623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:166:
InlineStylePropertyMap::IterationSource*
InlineStylePropertyMap::startIteration(ScriptState* scriptState, ExceptionState&
exceptionState)
On 2016/04/15 05:27:53, Timothy Loh wrote:
> can we make something that works for all the maps?

Done.

Timothy Loh

lgtm https://codereview.chromium.org/1874623002/diff/120001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html (right): https://codereview.chromium.org/1874623002/diff/120001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html#newcode27 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html:27: }, 'Adding a property while iterating over entries() ...

4 years, 7 months ago (2016-05-13 04:57:45 UTC) #5

meade_UTC10

https://codereview.chromium.org/1874623002/diff/120001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html (right): https://codereview.chromium.org/1874623002/diff/120001/third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html#newcode27 third_party/WebKit/LayoutTests/typedcssom/inlinestyle/inlineStylePropertyMap_iterationWithModification.html:27: }, 'Adding a property while iterating over entries() doesn\'t ...

4 years, 7 months ago (2016-05-13 05:46:34 UTC) #6

commit-bot: I haz the power

This CL has an open dependency (Issue 1849653003 Patch 290001). Please resolve the dependency and ...

4 years, 7 months ago (2016-05-13 05:46:58 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874623002/180001

4 years, 6 months ago (2016-06-07 01:26:11 UTC) #13

commit-bot: I haz the power

4 years, 6 months ago (2016-06-07 01:31:54 UTC) #16

Message was sent while issue was closed.

Patchset 10 (id:??) landed as
https://crrev.com/f01caa59865aec4c79d128e14f4d9fc605e02916
Cr-Commit-Position: refs/heads/master@{#398190}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages