Bug 84048

Summary:

ShadowRoot needs resetStyleInheritance

Product:

WebKit

Reporter:

Takashi Sakamoto <tasak>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cmarcelo, dglazkov, gustavo, macpherson, menard, ojan, philn, tasak, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ec2-cr-linux-04	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Takashi Sakamoto

Reported 2012-04-16 10:03:51 PDT

Currently ShadowRoot does not have resetStyleInheritance API. We should have this.

Attachments
Patch (11.11 KB, patch) 2012-04-16 11:44 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (172.82 KB, application/zip) 2012-04-16 12:37 PDT, WebKit Review Bot	no flags	Details
Patch (11.67 KB, patch) 2012-04-16 15:24 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (11.79 KB, patch) 2012-04-16 16:01 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (12.12 KB, patch) 2012-04-17 15:45 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (10.78 KB, patch) 2012-04-19 12:33 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (13.18 KB, patch) 2012-05-01 03:38 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (13.19 KB, patch) 2012-05-01 04:43 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (19.78 KB, patch) 2012-05-14 18:58 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (18.59 KB, patch) 2012-05-16 02:16 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (21.71 KB, patch) 2012-05-18 05:33 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (20.59 KB, patch) 2012-05-28 20:40 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (21.26 KB, patch) 2012-05-29 22:48 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (21.38 KB, patch) 2012-05-30 22:16 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (24.15 KB, patch) 2012-06-01 00:23 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (26.69 KB, patch) 2012-06-06 02:28 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (26.74 KB, patch) 2012-06-06 03:09 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (26.74 KB, patch) 2012-06-06 04:02 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Takashi Sakamoto

Comment 1 2012-04-16 10:04:25 PDT

See spec: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes

Takashi Sakamoto

Comment 2 2012-04-16 11:44:05 PDT

Created attachment 137370 [details] Patch

WebKit Review Bot

Comment 3 2012-04-16 12:37:13 PDT

Comment on attachment 137370 [details] Patch Attachment 137370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12412498 New failing tests: canvas/philip/tests/2d.text.draw.align.left.html canvas/philip/tests/2d.text.draw.fill.unaffected.html canvas/philip/tests/2d.text.draw.align.right.html canvas/philip/tests/2d.text.draw.fill.rtl.html canvas/philip/tests/2d.text.draw.align.start.rtl.html canvas/philip/tests/2d.text.draw.fill.maxWidth.zero.html canvas/philip/tests/2d.text.draw.fill.maxWidth.small.html canvas/philip/tests/2d.missingargs.html canvas/philip/tests/2d.text.draw.align.start.ltr.html accessibility/aria-disabled.html canvas/philip/tests/2d.state.saverestore.font.html canvas/philip/tests/2d.text.draw.fill.basic.html canvas/philip/tests/2d.text.draw.align.end.ltr.html canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html canvas/philip/tests/2d.text.draw.align.end.rtl.html canvas/philip/tests/2d.text-custom-font-load-crash.html canvas/philip/tests/2d.text.draw.fill.maxWidth.large.html canvas/philip/tests/2d.text.draw.align.center.html canvas/philip/tests/2d.text.draw.baseline.alphabetic.html canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html

WebKit Review Bot

Comment 4 2012-04-16 12:37:23 PDT

Created attachment 137381 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Takashi Sakamoto

Comment 5 2012-04-16 15:24:03 PDT

Created attachment 137413 [details] Patch

Takashi Sakamoto

Comment 6 2012-04-16 16:01:08 PDT

Created attachment 137421 [details] Patch

Hajime Morrita

Comment 7 2012-04-16 19:37:52 PDT

Comment on attachment 137421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137421&action=review > Source/WebCore/css/CSSStyleSelector.cpp:3081 > void CSSStyleSelector::applyProperty(CSSPropertyID id, CSSValue *value) This applyProperty() is an ultra host function and computing stuff here won't be a good idea in general. I guess we could precompute staff in CSSStyleSelector::initElement() or some per-element call path. > Source/WebCore/css/CSSStyleSelector.cpp:3083 > + TreeScope* treeScope = m_element? m_element->treeScope() : 0; Can treeScope be null? > Source/WebCore/css/CSSStyleSelector.cpp:3088 > + || ((!m_parentNode || (treeScope && treeScope->resetStyleInheritance() && treeScope != m_parentNode->treeScope())) && value->isInheritedValue()); This condition looks complicated enough. Could we simplify this somehow? > LayoutTests/fast/dom/shadow/shadow-root-resetStyleInheritance.html:1 > + <!DOCTYPE html> Could you use ref test if possible?

Takashi Sakamoto

Comment 8 2012-04-17 15:45:39 PDT

Created attachment 137623 [details] Patch

Takashi Sakamoto

Comment 9 2012-04-17 15:50:55 PDT

(In reply to comment #7) > (From update of attachment 137421 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137421&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:3081 > > void CSSStyleSelector::applyProperty(CSSPropertyID id, CSSValue *value) > > This applyProperty() is an ultra host function and computing stuff here won't be a good idea in general. > I guess we could precompute staff in CSSStyleSelector::initElement() or some per-element call path. I see. Is it ok to modify CSSStyleSelector::styleForElement()? I modified the method to create recomputed matchedResult before applyMatchedResults. > > Source/WebCore/css/CSSStyleSelector.cpp:3083 > > + TreeScope* treeScope = m_element? m_element->treeScope() : 0; > > Can treeScope be null? I removed the code. I think, treeScope cannot be null, but m_element can be null. > > Source/WebCore/css/CSSStyleSelector.cpp:3088 > > + || ((!m_parentNode || (treeScope && treeScope->resetStyleInheritance() && treeScope != m_parentNode->treeScope())) && value->isInheritedValue()); > > This condition looks complicated enough. Could we simplify this somehow? I removed the condition, but I still use the following: if (element && element->treeScope()->resetStyleInheritance() && m_parentNode && element->treeScope() != m_parentNode->treeScope()) > > LayoutTests/fast/dom/shadow/shadow-root-resetStyleInheritance.html:1 > > + <!DOCTYPE html> > > Could you use ref test if possible? Sure. Done.

Hajime Morrita

Comment 10 2012-04-18 09:09:41 PDT

Comment on attachment 137623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137623&action=review > Source/WebCore/ChangeLog:7 > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes Could you explain the change itself briefly? > Source/WebCore/css/CSSStyleSelector.cpp:1638 > +void CSSStyleSelector::applyResetStyleInheritance(MatchResult& result) This isn't the right approach. - The flag should affect not only for direct children of the shadow boundary - Allocating properties for each style resolution is too expensive. I think this "reset" thing should be handled at where we compute the inheritance in RenderStyle object because RenderStyle is responsible for style inheritance in WebKit. You can find such places by seeing the callers of RenderStyle::inheritFrom().

Takashi Sakamoto

Comment 11 2012-04-18 14:25:49 PDT

(In reply to comment #10) > (From update of attachment 137623 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137623&action=review > > > Source/WebCore/ChangeLog:7 > > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes > > Could you explain the change itself briefly? > > > Source/WebCore/css/CSSStyleSelector.cpp:1638 > > +void CSSStyleSelector::applyResetStyleInheritance(MatchResult& result) > > This isn't the right approach. > - The flag should affect not only for direct children of the shadow boundary > - Allocating properties for each style resolution is too expensive. > > I think this "reset" thing should be handled at where we compute the inheritance in RenderStyle object > because RenderStyle is responsible for style inheritance in WebKit. > You can find such places by seeing the callers of RenderStyle::inheritFrom(). I think, the best place to implement resetStyleInheritance is CSSStyleSelector::applyProperty, because RenderStyle::inheritFrom is just initializing RenderStyle (by copying the given render style to this render style). For example, CSSStyleSelector::styleForElement() has the following code: ---- m_style = RenderStyle::create(); if (m_parentStyle) m_style->inheritFrom(m_parentStyle); ... ---- If modifying CSSStyleSelector::applyProperty is not good, I have the following idea: (1) create a custom CSSStyleApplyProperty, which has only applyInitial and applyValue (applyInherit is the same as applyInitial). (2) modifying matchResult (I agree that this approach is very expensive). I think, class RenderStyle is for just holding final style values (I mean, no 'inherit' and no 'initial') for rendering, because CSSStyleApplyProperty's applyInherit handlers always look up at parent styles. e.g. ---- class ApplyPropertyDefaultBase { ... static void applyInheritValue(CSSStyleSelector* selector) { setValue(selector->style(), value(selector->parentStyle())); ... ----

Takashi Sakamoto

Comment 12 2012-04-19 12:33:26 PDT

Created attachment 137950 [details] Patch

Takashi Sakamoto

Comment 13 2012-04-19 12:37:43 PDT

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 137623 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137623&action=review > > > > > Source/WebCore/ChangeLog:7 > > > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes > > > > Could you explain the change itself briefly? I see. Done. > > > > > Source/WebCore/css/CSSStyleSelector.cpp:1638 > > > +void CSSStyleSelector::applyResetStyleInheritance(MatchResult& result) > > > > This isn't the right approach. > > - The flag should affect not only for direct children of the shadow boundary > > - Allocating properties for each style resolution is too expensive. > > > > I think this "reset" thing should be handled at where we compute the inheritance in RenderStyle object > > because RenderStyle is responsible for style inheritance in WebKit. > > You can find such places by seeing the callers of RenderStyle::inheritFrom(). I'm sorry. I misunderstood the spec. I agree that the approach (seeing the callers of RenderStyle::inheritFrom) is right. I think, CSSStyleSelector::styleForElement() is the place. I re-created a patch and also added new test cases for changing resetStyleInheritance dynamically.

Build Bot

Comment 14 2012-04-19 12:58:34 PDT

Comment on attachment 137950 [details] Patch Attachment 137950 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12297406

Ryosuke Niwa

Comment 15 2012-04-21 18:43:01 PDT

Comment on attachment 137950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137950&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1599 > + bool resetStyleInheritance = element && element->treeScope()->resetStyleInheritance(); Doesn't treeScope() have O(n) time complexity where n is the depth of shadow tree scopes? I'm concerned that this may regress performance given CSSStyleSelector::styleForElement is a pretty hot function. Also, don't we need to modify style selector for SVG as well? > Source/WebCore/dom/ShadowRoot.h:72 > + virtual bool resetStyleInheritance() const; Who overrides this function? If you're overriding it, then please add OVERRIDE keyword. > LayoutTests/fast/dom/shadow/shadow-root-resetStyleInheritance-expected.html:4 > +<head> > +</head> We don't need head element here. > LayoutTests/fast/dom/shadow/shadow-root-resetStyleInheritance-expected.html:9 > +<div><div style="color: #fee"><div></div></div></div> > +<div><div style="color: #fee"><div style="color: initial"></div></div> > +<div><div style="color: #fee"><div></div></div></div> > +<div><div style="color: #fee"><div style="color: initial"></div></div> Please add more test cases as I mentioned on the other bug. Sorry but r- due to lack of tests.

Takashi Sakamoto

Comment 16 2012-05-01 03:38:18 PDT

Created attachment 139607 [details] Patch

Takashi Sakamoto

Comment 17 2012-05-01 03:47:08 PDT

(In reply to comment #15) > (From update of attachment 137950 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137950&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:1599 > > + bool resetStyleInheritance = element && element->treeScope()->resetStyleInheritance(); > > Doesn't treeScope() have O(n) time complexity where n is the depth of shadow tree scopes? > I'm concerned that this may regress performance given CSSStyleSelector::styleForElement is a pretty hot function. The treeScope() just returns m_treeScope, which is a member variable of NodeRareData. > Also, don't we need to modify style selector for SVG as well? I found that SVG element in shadow doesn't work well now. So I think, it is not possible to test SVG styles. I will try after SVG element in shadow work (and if needed, fix bugs). > > Source/WebCore/dom/ShadowRoot.h:72 > > + virtual bool resetStyleInheritance() const; > > Who overrides this function? If you're overriding it, then please add OVERRIDE keyword. Done. > > LayoutTests/fast/dom/shadow/shadow-root-resetStyleInheritance-expected.html:9 > > +<div><div style="color: #fee"><div></div></div></div> > > +<div><div style="color: #fee"><div style="color: initial"></div></div> > > +<div><div style="color: #fee"><div></div></div></div> > > +<div><div style="color: #fee"><div style="color: initial"></div></div> > > Please add more test cases as I mentioned on the other bug. I added more style properties, especially font properties. I found that default RenderStyle doesn't have correct font description (c.f. StyleInheritedData.cpp. Its member variable: font is not initialized by using "initial")

Build Bot

Comment 18 2012-05-01 04:05:48 PDT

Comment on attachment 139607 [details] Patch Attachment 139607 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12592514

Takashi Sakamoto

Comment 19 2012-05-01 04:43:03 PDT

Created attachment 139609 [details] Patch

Hajime Morrita

Comment 20 2012-05-09 04:53:31 PDT

Comment on attachment 139609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139609&action=review > Source/WebCore/css/StyleResolver.cpp:1604 > + bool crossShadowBoundary = element && m_parentNode && element->treeScope() != m_parentNode->treeScope(); I noticed that we don't need to compare tree scopes to detect the boundary. We compute m_parentNode using Node::parentNodeForRenderingAndStyle(), which is implemented using NodeRenderingContext. And NodeRenderingContext should know what we need to know. I think we can use it directly instead of calling parentNodeForRenderingAndStyle(). Or we can add an extra out-parameter to pass necessary information. Then we don't need to re-query tree scope of each node.

Takashi Sakamoto

Comment 21 2012-05-14 18:58:50 PDT

Created attachment 141840 [details] Patch

Dimitri Glazkov (Google)

Comment 22 2012-05-14 20:06:54 PDT

Comment on attachment 141840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141840&action=review > Source/WebCore/css/StyleResolver.cpp:1642 > + bool resetStyleInheritance = element && element->treeScope()->resetStyleInheritance() && element->parentNodeForRenderingAndStyleAtShadowBoundary(); in this statement, the first two clauses are cheap, and the third one is expensive. What impact will this have on this hot function? Perhaps we could get rid of it somehow? > Source/WebCore/dom/Node.h:510 > + bool parentNodeForRenderingAndStyleAtShadowBoundary(); That is a crazy-long name. Do we really need to add this to Node?

Hajime Morrita

Comment 23 2012-05-15 22:04:05 PDT

Comment on attachment 141840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141840&action=review I think we don't need to compute m_parentNodeForRenderingAndStyleAtShadowBoundary at the constructor but we can compute it on the fly. How about to have NodeRenderingContext::resetStyleInheritance() ? >> Source/WebCore/css/StyleResolver.cpp:1642 >> + bool resetStyleInheritance = element && element->treeScope()->resetStyleInheritance() && element->parentNodeForRenderingAndStyleAtShadowBoundary(); > > in this statement, the first two clauses are cheap, and the third one is expensive. What impact will this have on this hot function? Perhaps we could get rid of it somehow? in initElement(), we call parentNodeForRendering(). That means we compute NodeRenderingContext behind that. Instead of calling parentNodeForRendering() there, we can use NodeRenderingContext directly to retrieve what we need. Both finding style reset boundary and finding the parent for rendering are (or should be) parts of same computation process. > Source/WebCore/dom/NodeRenderingContext.cpp:58 > + , m_parentNodeForRenderingAndStyleAtShadowBoundary(0) Should be false. > Source/WebCore/dom/NodeRenderingContext.cpp:86 > + m_parentNodeForRenderingAndStyleAtShadowBoundary = NodeRenderingContext(m_insertionPoint).parentNodeForRenderingAndStyleAtShadowBoundary(); Please don't create NodeRenderingContext twice. It's expensive. > Source/WebCore/dom/NodeRenderingContext.cpp:93 > + m_parentNodeForRenderingAndStyleAtShadowBoundary = false; It looks we don't need this because it's the fault value.

Takashi Sakamoto

Comment 24 2012-05-16 02:16:30 PDT

Created attachment 142201 [details] Patch

Takashi Sakamoto

Comment 25 2012-05-16 03:50:33 PDT

(In reply to comment #23) > (From update of attachment 141840 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141840&action=review > > I think we don't need to compute m_parentNodeForRenderingAndStyleAtShadowBoundary at the constructor > but we can compute it on the fly. > > How about to have NodeRenderingContext::resetStyleInheritance() ? Do you mean NodeRenderingContext also should look up ShadowRoot's resetStyleInheritance? > > >> Source/WebCore/css/StyleResolver.cpp:1642 > >> + bool resetStyleInheritance = element && element->treeScope()->resetStyleInheritance() && element->parentNodeForRenderingAndStyleAtShadowBoundary(); > > > > in this statement, the first two clauses are cheap, and the third one is expensive. What impact will this have on this hot function? Perhaps we could get rid of it somehow? > > in initElement(), we call parentNodeForRendering(). > That means we compute NodeRenderingContext behind that. > Instead of calling parentNodeForRendering() there, we can use NodeRenderingContext directly to retrieve what we need. > Both finding style reset boundary and finding the parent for rendering are (or should be) parts of same computation process. I added "NodeRenderingContext context(e);" to obtain parentNode and information about whether parentNode is in the same tree as the element:e or not. However I'm not sure which is the better way to obtain NodeRenderingContext, creating a new method in class Node, i.e. Node::parentNodeForRendering() or just adding "NodeRenderingContext context(e)". Is there any policy? I mean, Node should create NodeRenderingContext and StyleResolver shouldn't or something. > > > Source/WebCore/dom/NodeRenderingContext.cpp:58 > > + , m_parentNodeForRenderingAndStyleAtShadowBoundary(0) > > Should be false. Done. > > > Source/WebCore/dom/NodeRenderingContext.cpp:86 > > + m_parentNodeForRenderingAndStyleAtShadowBoundary = NodeRenderingContext(m_insertionPoint).parentNodeForRenderingAndStyleAtShadowBoundary(); > > Please don't create NodeRenderingContext twice. It's expensive. I see. Done. > > > Source/WebCore/dom/NodeRenderingContext.cpp:93 > > + m_parentNodeForRenderingAndStyleAtShadowBoundary = false; > > It looks we don't need this because it's the fault value. Done.

Takashi Sakamoto

Comment 26 2012-05-18 05:33:37 PDT

Created attachment 142694 [details] Patch

Takashi Sakamoto

Comment 27 2012-05-18 05:37:47 PDT

(In reply to comment #26) > Created an attachment (id=142694) [details] > Patch I updated the layout test to use getComputedStyle according to the review of bug 84251. I also modified NodeRenderingContext to calculate resetStyleInheritance directly. Best regards, Takashi Sakamoto

Hajime Morrita

Comment 28 2012-05-20 17:48:44 PDT

Comment on attachment 142694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142694&action=review Basically looks good! I added a few comments. > Source/WebCore/css/StyleResolver.cpp:1170 > + m_resetStyleInheritance = false; It looks we don't need to add m_resetStyleInheritance. Setting NULL to m_parentStyle will work fine for that purpose. > Source/WebCore/css/StyleResolver.cpp:1680 > + if (m_resetStyleInheritance) { Why do we need this? Is it different from the default?

Takashi Sakamoto

Comment 29 2012-05-25 03:32:37 PDT

(In reply to comment #28) > (From update of attachment 142694 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142694&action=review > > Basically looks good! I added a few comments. > > > Source/WebCore/css/StyleResolver.cpp:1170 > > + m_resetStyleInheritance = false; > > It looks we don't need to add m_resetStyleInheritance. > Setting NULL to m_parentStyle will work fine for that purpose. > > > Source/WebCore/css/StyleResolver.cpp:1680 > > + if (m_resetStyleInheritance) { > > Why do we need this? Is it different from the default? I need this because defaultStyle doesn't have any font information. Font information is stored in "inherited" member variable in RenderStyle. The defaultStyle's "inherited" is initialized by using StyleInheritedData::StyleInheritedData(). The constructor initializes "font" member variable by using default constructor. This causes a problem, the size of textNode in style-inheritance reseted node is 0x0. Best regards, Takashi Sakamoto

Takashi Sakamoto

Comment 30 2012-05-28 20:40:50 PDT

Created attachment 144435 [details] Patch

Hajime Morrita

Comment 31 2012-05-29 19:07:15 PDT

Comment on attachment 144435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144435&action=review The attempt looks right. Let's iterate once to factor code a bit. > Source/WebCore/css/StyleResolver.cpp:1173 > + if (resetStyleInheritance) This can be inside if (e) block above. > Source/WebCore/css/StyleResolver.cpp:1660 > + if (m_checker.document()->settings()) { I guess you can check frame() or page() here. Checking settings() is cryptic. Is it possible to extract these if/else to a function or method? Having many conditionals here in the same function looks too complicated. My feeling is that it's worth to have a method for creating an initial style instead of making it up inline here.

Takashi Sakamoto

Comment 32 2012-05-29 22:48:49 PDT

Created attachment 144706 [details] Patch

Build Bot

Comment 33 2012-05-29 23:13:45 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12839896

Early Warning System Bot

Comment 34 2012-05-29 23:23:59 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12846759

Early Warning System Bot

Comment 35 2012-05-29 23:24:22 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12814002

Build Bot

Comment 36 2012-05-29 23:27:43 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12852692

Gyuyoung Kim

Comment 37 2012-05-29 23:54:29 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12844767

WebKit Review Bot

Comment 38 2012-05-30 00:03:38 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12845787

Build Bot

Comment 39 2012-05-30 00:14:46 PDT

Comment on attachment 144706 [details] Patch Attachment 144706 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12849772

Takashi Sakamoto

Comment 40 2012-05-30 22:16:33 PDT

Created attachment 144986 [details] Patch

Takashi Sakamoto

Comment 41 2012-06-01 00:23:02 PDT

Created attachment 145225 [details] Patch

Dimitri Glazkov (Google)

Comment 42 2012-06-01 09:26:50 PDT

Comment on attachment 145225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145225&action=review > Source/WebCore/css/StyleResolver.cpp:1173 > + m_parentStyle = parentStyle; > + else > + m_parentStyle = m_parentNode ? m_parentNode->renderStyle() : 0; This can be made into inline if. > Source/WebCore/css/StyleResolver.cpp:4925 > + Settings* settings = m_checker.document()->settings(); > + if (!settings) > + return false; I would rather have a separate function that returns Settings*. This way, you avoid ambiguity of why initializeFontSylte returns a boolean: if (Settings* settings = documentSettings()) { initializeFontStyle(settings); ... Also might be worth looking at other callsites of document()->settings() and using this new accessor there. Looks like a good separate patch :)

Takashi Sakamoto

Comment 43 2012-06-06 02:28:20 PDT

Created attachment 145967 [details] Patch

Takashi Sakamoto

Comment 44 2012-06-06 03:04:36 PDT

Comment on attachment 145967 [details] Patch I canceled because the patch removed ASSERT(settings) in applyProperty.

Takashi Sakamoto

Comment 45 2012-06-06 03:09:53 PDT

Created attachment 145974 [details] Patch

Takashi Sakamoto

Comment 46 2012-06-06 04:02:31 PDT

Created attachment 145986 [details] Patch

Takashi Sakamoto

Comment 47 2012-06-06 04:04:40 PDT

(In reply to comment #45) > Created an attachment (id=145974) [details] > Patch I'm sorry. I found a mistake in my changelog entry. (WebCore::StyleResolver::checkForGenericFamilyChange) should be under the scope of css/StyleResolver.cpp, not StyleResolver.h. I created a patch again. Best regards, Takashi Sakamoto

Dimitri Glazkov (Google)

Comment 48 2012-06-06 09:03:28 PDT

Comment on attachment 145986 [details] Patch Looks good. Morrita-san, your final word?

Hajime Morrita

Comment 49 2012-06-07 21:43:32 PDT

Comment on attachment 145986 [details] Patch OK. Let's land and see.

WebKit Review Bot

Comment 50 2012-06-07 22:29:17 PDT

Comment on attachment 145986 [details] Patch Clearing flags on attachment: 145986 Committed r119799: <http://trac.webkit.org/changeset/119799>

WebKit Review Bot

Comment 51 2012-06-07 22:29:25 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.