Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(57)

Issue 2450093005: Support display: contents for elements, first-line and first-letter pseudos. (Closed)

Created:
4 years, 1 month ago by emilio
Modified:
3 years, 9 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support display: contents for elements, first-line and first-letter pseudos. This CL implements a basic version of display: contents that doesn't take into account pseudo-elements of display: contents elements. As part of implementing it, all the non-layout-object styles have been unified. That means that elements like <option> and <optgroup> no longer have custom style callbacks just to preserve their computed style while they don't have a LayoutObject. Instead, they use the ElementRareData, the same way as display: contents and getComputedStyle on display: none elements. TEST=imported/csswg-test/css-display-3/ BUG=657748 Review-Url: https://codereview.chromium.org/2450093005 Cr-Commit-Position: refs/heads/master@{#451966} Committed: https://chromium.googlesource.com/chromium/src/+/5fce73c8d68a52abbd31359c04373077c477e16c

Patch Set 1 #

Patch Set 2 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 12

Patch Set 3 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 6

Patch Set 4 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 5 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 6 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 7 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 2

Patch Set 8 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 9 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 4

Patch Set 10 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 11 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 12 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 13 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 14 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 15 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 16 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 6

Patch Set 17 : WIP with changes to StyleAdjuster #

Patch Set 18 : WIP: Changes to StyleAdjuster/StyleResolver. #

Patch Set 19 : WIP: Changes to StyleAdjuster/StyleResolver. #

Patch Set 20 : WIP: Changes to StyleAdjuster/StyleResolver. #

Patch Set 21 : Rebased, with more tests #

Patch Set 22 : Allow text as child of the LayoutView, since it can happen with display: contents #

Total comments: 4

Patch Set 23 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 24 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 25 : Rebased #

Patch Set 26 : Rebased #

Patch Set 27 : Rebased #

Total comments: 20

Patch Set 28 : Rebased, review comments addressed, whitespace attachment fixes postponed. #

Patch Set 29 : Support display: contents for elements, first-line and first-letter pseudos. #

Patch Set 30 : rebased #

Patch Set 31 : Support display: contents for elements, first-line and first-letter pseudos. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+553 lines, -299 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +3 lines, -31 lines 0 comments Download
D third_party/WebKit/LayoutTests/external/csswg-test/css-display-3/display-contents-computed-style-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +0 lines, -36 lines 0 comments Download
M third_party/WebKit/LayoutTests/shadow-dom/slots-fallback-in-document-tree-expected.html View 1 2 3 4 5 6 7 8 1 chunk +1 line, -4 lines 0 comments Download
M third_party/WebKit/Source/core/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/ElementResolveContext.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/ElementResolveContext.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +9 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 9 chunks +21 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 8 chunks +19 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolverState.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +17 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +15 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +14 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 5 chunks +71 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/dom/LayoutTreeBuilder.h View 1 2 3 4 2 chunks +7 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +10 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +40 lines, -38 lines 0 comments Download
M third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 21 22 11 chunks +120 lines, -33 lines 2 comments Download
A third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversalTest.cpp View 1 2 3 4 5 6 7 1 chunk +139 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/dom/NodeComputedStyle.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +5 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Text.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +22 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLOptGroupElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +0 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLOptGroupElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +0 lines, -31 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLOptionElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLOptionElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 5 chunks +11 lines, -27 lines 2 comments Download
M third_party/WebKit/Source/core/layout/LayoutObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/SVGElementRareData.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DState.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 128 (57 generated)
emilio
Hey Rune, Rego, could you take a look at this to see if this approach ...
4 years, 1 month ago (2016-10-27 16:06:40 UTC) #6
emilio
Both first-letter.html, inline-001.html and td.html work, but generate diffs of off-by-one pixels that pass if ...
4 years, 1 month ago (2016-10-27 16:14:50 UTC) #8
jfernandez
https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3139 third_party/WebKit/Source/core/dom/Element.cpp:3139: return 0; return nullptr https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3143 third_party/WebKit/Source/core/dom/Element.cpp:3143: return 0; return ...
4 years, 1 month ago (2016-10-27 17:08:02 UTC) #10
rune
On 2016/10/27 16:14:50, emilio wrote: > Both first-letter.html, inline-001.html and td.html work, but generate diffs ...
4 years, 1 month ago (2016-10-27 17:45:00 UTC) #11
emilio
On 2016/10/27 17:08:02, jfernandez wrote: > https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp > File third_party/WebKit/Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp#newcode3139 > ...
4 years, 1 month ago (2016-10-27 18:17:34 UTC) #12
emilio
On 2016/10/27 17:45:00, rune wrote: > On 2016/10/27 16:14:50, emilio wrote: > > Both first-letter.html, ...
4 years, 1 month ago (2016-10-27 18:20:24 UTC) #13
emilio
On 2016/10/27 17:08:02, jfernandez wrote: > third_party/WebKit/Source/core/dom/Element.h:449: ComputedStyle* > displayContentsStyle() const; > I guess we ...
4 years, 1 month ago (2016-10-27 18:25:04 UTC) #14
Manuel Rego
On 2016/10/27 17:45:00, rune wrote: > On 2016/10/27 16:14:50, emilio wrote: > > Both first-letter.html, ...
4 years, 1 month ago (2016-10-27 19:20:24 UTC) #15
drott
On 2016/10/27 at 19:20:24, rego wrote: > On 2016/10/27 17:45:00, rune wrote: > > On ...
4 years, 1 month ago (2016-10-27 19:26:47 UTC) #16
rune
On 2016/10/27 19:26:47, drott wrote: > On 2016/10/27 at 19:20:24, rego wrote: > > On ...
4 years, 1 month ago (2016-10-27 19:39:49 UTC) #17
jfernandez
On 2016/10/27 18:25:04, emilio wrote: > On 2016/10/27 17:08:02, jfernandez wrote: > > third_party/WebKit/Source/core/dom/Element.h:449: ComputedStyle* ...
4 years, 1 month ago (2016-10-28 08:30:12 UTC) #18
jfernandez
On 2016/10/27 18:17:34, emilio wrote: > On 2016/10/27 17:08:02, jfernandez wrote: > > > https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp ...
4 years, 1 month ago (2016-10-28 08:33:21 UTC) #19
jfernandez
On 2016/10/27 18:17:34, emilio wrote: > On 2016/10/27 17:08:02, jfernandez wrote: > > > https://codereview.chromium.org/2450093005/diff/20001/third_party/WebKit/Source/core/dom/Element.cpp ...
4 years, 1 month ago (2016-10-28 08:44:03 UTC) #20
jfernandez
4 years, 1 month ago (2016-10-28 08:44:12 UTC) #21
emilio
Sorry for the delay with this. So I addressed a bunch of review comments (note ...
4 years, 1 month ago (2016-10-31 11:34:08 UTC) #22
rune
On 2016/10/31 11:34:08, emilio wrote: > Sorry for the delay with this. > > So ...
4 years, 1 month ago (2016-10-31 14:06:13 UTC) #23
emilio
On 2016/10/31 14:06:13, rune wrote: > This is not going to work for style recalc ...
4 years, 1 month ago (2016-10-31 15:25:42 UTC) #24
rune
https://codereview.chromium.org/2450093005/diff/40001/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp File third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp (right): https://codereview.chromium.org/2450093005/diff/40001/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp#newcode117 third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp:117: m_node->storeDisplayContentsStyle(style); It's a surprise that shouldCreateLayoutObject has side-effects. https://codereview.chromium.org/2450093005/diff/40001/third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h ...
4 years, 1 month ago (2016-11-01 07:12:23 UTC) #25
rune
On 2016/10/31 15:25:42, emilio wrote: > That being said, I still haven't dived enough in ...
4 years, 1 month ago (2016-11-01 07:19:08 UTC) #26
emilio
https://codereview.chromium.org/2450093005/diff/40001/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp File third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp (right): https://codereview.chromium.org/2450093005/diff/40001/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp#newcode117 third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp:117: m_node->storeDisplayContentsStyle(style); On 2016/11/01 07:12:23, rune wrote: > It's a ...
4 years, 1 month ago (2016-11-01 15:46:31 UTC) #27
emilio
I updated the CL with less hacks, and now without killing the optimization that prevented ...
4 years, 1 month ago (2016-11-02 16:45:42 UTC) #28
emilio
On 2016/11/02 16:45:42, emilio wrote: > I updated the CL with less hacks, and now ...
4 years, 1 month ago (2016-11-02 16:46:20 UTC) #29
rune
https://codereview.chromium.org/2450093005/diff/120001/third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp (right): https://codereview.chromium.org/2450093005/diff/120001/third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp#newcode49 third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp:49: // find them in the traversal. I think you ...
4 years, 1 month ago (2016-11-08 13:09:21 UTC) #38
rune
I've looked at the traversal changes which seem to do what it's supposed to. I'm ...
4 years, 1 month ago (2016-11-08 13:26:49 UTC) #39
emilio
Yeah, I think the dust is mostly settled now. I have addressed your review comments ...
4 years, 1 month ago (2016-11-08 15:43:02 UTC) #42
emilio
So failures seem to be platform dependent (OSX only), and they're again subpixel failures, sigh... ...
4 years, 1 month ago (2016-11-08 20:53:30 UTC) #49
rune
On 2016/11/08 20:53:30, emilio wrote: > So failures seem to be platform dependent (OSX only), ...
4 years, 1 month ago (2016-11-15 13:23:15 UTC) #50
emilio
On 2016/11/15 13:23:15, rune wrote: > On 2016/11/08 20:53:30, emilio wrote: > > So failures ...
4 years, 1 month ago (2016-11-15 15:03:31 UTC) #51
emilio
On 2016/11/15 15:03:31, emilio wrote: > On 2016/11/15 13:23:15, rune wrote: > > On 2016/11/08 ...
4 years, 1 month ago (2016-11-15 15:20:21 UTC) #52
emilio
> Pfft, nevermind, I forgot to rename expectations, but I think there are a few ...
4 years, 1 month ago (2016-11-15 15:39:23 UTC) #56
emilio
So I imported the last version of the tests from CSSWG and everything seems to ...
4 years, 1 month ago (2016-11-17 13:11:58 UTC) #60
Manuel Rego
On 2016/11/17 13:11:58, emilio wrote: > So I imported the last version of the tests ...
4 years, 1 month ago (2016-11-17 13:27:42 UTC) #61
Manuel Rego
https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/resources/display-contents-acid.html File third_party/WebKit/LayoutTests/css-display-3/resources/display-contents-acid.html (right): https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/resources/display-contents-acid.html#newcode1 third_party/WebKit/LayoutTests/css-display-3/resources/display-contents-acid.html:1: <!DOCTYPE html> I wouldn't use "resources/" folder for a ...
4 years, 1 month ago (2016-11-17 13:29:24 UTC) #62
rune
https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html File third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html (right): https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html#newcode31 third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html:31: var TIMEOUT = 100; This smells flakiness. Why do ...
4 years, 1 month ago (2016-11-18 10:35:40 UTC) #63
emilio
https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html File third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html (right): https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html#newcode31 third_party/WebKit/LayoutTests/css-display-3/display-contents-reattachment.html:31: var TIMEOUT = 100; On 2016/11/18 10:35:39, rune wrote: ...
4 years, 1 month ago (2016-11-18 10:54:35 UTC) #64
emilio
https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1716 third_party/WebKit/Source/core/dom/Element.cpp:1716: parentNode()->reattachLayoutTree(); On 2016/11/18 10:35:40, rune wrote: > This is ...
4 years, 1 month ago (2016-11-18 11:04:43 UTC) #65
emilio
On 2016/11/18 11:04:43, emilio wrote: > https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/Source/core/dom/Element.cpp > File third_party/WebKit/Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1716 > ...
4 years, 1 month ago (2016-11-18 11:11:02 UTC) #66
rune
On 2016/11/18 11:11:02, emilio wrote: > On 2016/11/18 11:04:43, emilio wrote: > > > https://codereview.chromium.org/2450093005/diff/300001/third_party/WebKit/Source/core/dom/Element.cpp ...
4 years, 1 month ago (2016-11-18 11:52:40 UTC) #67
emilio
On 2016/11/18 11:52:40, rune wrote: > Could you please write up a design in a ...
4 years, 1 month ago (2016-11-18 12:01:17 UTC) #68
emilio
On 2016/11/18 11:52:40, rune wrote: > I still think it's not acceptable to reattach siblings ...
4 years, 1 month ago (2016-11-18 17:41:57 UTC) #69
emilio
Some updates: Design document (just mailed style-dev@): https://docs.google.com/document/d/1YASzPEgPk8oAnJU7RiGfbShqP8ZdySkS5Aqf0yv3cPA/edit?usp=sharing Tests imported in this CL are ready ...
4 years ago (2016-11-30 19:50:37 UTC) #84
rune
https://codereview.chromium.org/2450093005/diff/420001/third_party/WebKit/Source/core/dom/Element.cpp File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2450093005/diff/420001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1856 third_party/WebKit/Source/core/dom/Element.cpp:1856: data->clearComputedStyle(); Don't you need to avoid doing this before ...
4 years ago (2016-12-01 15:28:54 UTC) #87
emilio
Thanks for the feedback Rune. In this version I've also aligned with Gecko on the ...
4 years ago (2016-12-01 17:32:56 UTC) #88
emilio
So I rebased this on top of the test imports, then looked through the remaining ...
4 years ago (2016-12-22 10:35:00 UTC) #89
emilio
Rebased, review ping?
3 years, 11 months ago (2017-01-18 16:05:30 UTC) #90
rune
Adding bugsnash@ and nainar@ as well. https://codereview.chromium.org/2450093005/diff/520001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2450093005/diff/520001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode804 third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:804: parentStyle); Is it ...
3 years, 10 months ago (2017-01-27 13:09:23 UTC) #92
emilio
Just rebased, addressed review comments, and postponed the whitespace reattachment fixes for a followup CL. ...
3 years, 10 months ago (2017-01-27 22:13:15 UTC) #93
rune
https://codereview.chromium.org/2450093005/diff/520001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2450093005/diff/520001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode804 third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:804: parentStyle); On 2017/01/27 22:13:15, emilio wrote: > On 2017/01/27 ...
3 years, 10 months ago (2017-02-06 22:40:44 UTC) #94
emilio
I also fixed a memory regression the previous version of the patch caused (see the ...
3 years, 10 months ago (2017-02-08 10:59:24 UTC) #95
emilio
On 2017/02/08 10:59:24, emilio wrote: > > Would it be best to override layoutObjectIsNeeded() to ...
3 years, 10 months ago (2017-02-08 13:14:55 UTC) #96
emilio
On 2017/02/08 13:14:55, emilio wrote: > On 2017/02/08 10:59:24, emilio wrote: > > > Would ...
3 years, 10 months ago (2017-02-20 22:37:43 UTC) #97
rune
On 2017/02/20 22:37:43, emilio wrote: > On 2017/02/08 13:14:55, emilio wrote: > > On 2017/02/08 ...
3 years, 10 months ago (2017-02-20 23:25:04 UTC) #98
rune
On 2017/02/20 23:25:04, rune wrote: > On 2017/02/20 22:37:43, emilio wrote: > > On 2017/02/08 ...
3 years, 10 months ago (2017-02-20 23:27:01 UTC) #99
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2450093005/580001
3 years, 10 months ago (2017-02-21 08:28:51 UTC) #102
commit-bot: I haz the power
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_presubmit/builds/368888)
3 years, 10 months ago (2017-02-21 08:36:31 UTC) #104
rune
On 2017/02/21 08:36:31, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 10 months ago (2017-02-21 08:42:20 UTC) #105
emilio
On 2017/02/21 08:42:20, rune wrote: > On 2017/02/21 08:36:31, commit-bot: I haz the power wrote: ...
3 years, 10 months ago (2017-02-21 08:54:57 UTC) #106
emilio
Hi senorblanco, junov, Could you take a look at the canvas2d changes in this patch? ...
3 years, 10 months ago (2017-02-21 08:56:54 UTC) #108
emilio
rune, I had to add other check[1] to fix a bug that was caught by ...
3 years, 10 months ago (2017-02-21 11:55:04 UTC) #109
rune
On 2017/02/21 11:55:04, emilio wrote: > rune, I had to add other check[1] to fix ...
3 years, 10 months ago (2017-02-21 14:18:32 UTC) #110
rune
On 2017/02/21 14:18:32, rune wrote: > This seems like a reasonable fix, but I think ...
3 years, 10 months ago (2017-02-21 14:36:41 UTC) #111
Stephen White
canvas2d LGTM
3 years, 10 months ago (2017-02-21 18:34:12 UTC) #112
emilio
On 2017/02/21 14:36:41, rune wrote: > On 2017/02/21 14:18:32, rune wrote: > > > This ...
3 years, 10 months ago (2017-02-21 20:30:04 UTC) #113
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2450093005/600001
3 years, 10 months ago (2017-02-21 20:31:09 UTC) #116
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on ...
3 years, 10 months ago (2017-02-21 22:37:37 UTC) #118
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2450093005/600001
3 years, 10 months ago (2017-02-22 08:06:35 UTC) #120
commit-bot: I haz the power
Committed patchset #31 (id:600001) as https://chromium.googlesource.com/chromium/src/+/5fce73c8d68a52abbd31359c04373077c477e16c
3 years, 10 months ago (2017-02-22 10:08:06 UTC) #123
emilio
On 2017/02/22 10:08:06, commit-bot: I haz the power wrote: > Committed patchset #31 (id:600001) as ...
3 years, 10 months ago (2017-02-22 11:58:39 UTC) #124
rune
On 2017/02/22 11:58:39, emilio wrote: > On 2017/02/22 10:08:06, commit-bot: I haz the power wrote: ...
3 years, 10 months ago (2017-02-22 12:27:08 UTC) #125
esprehn
https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp (right): https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp#newcode36 third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp:36: inline static bool hasDisplayContentsStyle(const Node& node) { IIRC static ...
3 years, 9 months ago (2017-03-08 03:42:38 UTC) #127
emilio
3 years, 9 months ago (2017-03-08 08:32:41 UTC) #128
Message was sent while issue was closed.
https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp (right):

https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.cpp:36: inline
static bool hasDisplayContentsStyle(const Node& node) {
On 2017/03/08 03:42:38, esprehn wrote:
> IIRC static implies inline, you don't need both like this.

As far as I know, inline and static are not quite the same, since I think static
forces internal linkage, while just inline doesn't. Not that it would make a
huge difference though, I guess. If there's a preferred style perhaps it could
be enforced by the presubmit lints?

https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/2450093005/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLOptionElement.cpp:94: if
(!context.resolvedStyle && parentComputedStyle()) {
On 2017/03/08 03:42:38, esprehn wrote:
> This code doesn't look right, the call below to attachLayoutTree will compute
> the style for you. There's no reason to call originalStyleForLayoutObject()
> manually like this.
> 
> Why do we need to call updateListOnLayoutObject() when the resolvedStyle is
> null?
> 
> Note that resolvedStyle is just a cache optimization, but you're using it to
> call something with side effects on the ownerSelect which doesn't seem right.
We
> can enter this function with or without resolvedStyle, what state are you
using
> it as a proxy for?

Note that this chunk of the code is written to preserve the same behavior as
before (the updateListOnLayoutObject() call was just inside
updateNonComputedStyle()).

I think the intention of the old code may just be to avoid an unneeded style
recalc from calling updateNonComputedStyle.

I'll try to simplify this code and submit a CL for you to review.

Powered by Google App Engine
This is Rietveld 408576698