Bug 84048 - ShadowRoot needs resetStyleInheritance
Summary: ShadowRoot needs resetStyleInheritance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 10:03 PDT by Takashi Sakamoto
Modified: 2012-06-07 22:29 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2012-04-16 10:03:51 PDT
Currently ShadowRoot does not have resetStyleInheritance API.
We should have this.
Comment 2 Takashi Sakamoto 2012-04-16 11:44:05 PDT
Created attachment 137370 [details]
Patch
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 Takashi Sakamoto 2012-04-16 15:24:03 PDT
Created attachment 137413 [details]
Patch
Comment 6 Takashi Sakamoto 2012-04-16 16:01:08 PDT
Created attachment 137421 [details]
Patch
Comment 7 Hajime Morrita 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?
Comment 8 Takashi Sakamoto 2012-04-17 15:45:39 PDT
Created attachment 137623 [details]
Patch
Comment 9 Takashi Sakamoto 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.
Comment 10 Hajime Morrita 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().
Comment 11 Takashi Sakamoto 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()));
...
----
Comment 12 Takashi Sakamoto 2012-04-19 12:33:26 PDT
Created attachment 137950 [details]
Patch
Comment 13 Takashi Sakamoto 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.
Comment 14 Build Bot 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
Comment 15 Ryosuke Niwa 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.
Comment 16 Takashi Sakamoto 2012-05-01 03:38:18 PDT
Created attachment 139607 [details]
Patch
Comment 17 Takashi Sakamoto 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")
Comment 18 Build Bot 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
Comment 19 Takashi Sakamoto 2012-05-01 04:43:03 PDT
Created attachment 139609 [details]
Patch
Comment 20 Hajime Morrita 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.
Comment 21 Takashi Sakamoto 2012-05-14 18:58:50 PDT
Created attachment 141840 [details]
Patch
Comment 22 Dimitri Glazkov (Google) 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?
Comment 23 Hajime Morrita 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.
Comment 24 Takashi Sakamoto 2012-05-16 02:16:30 PDT
Created attachment 142201 [details]
Patch
Comment 25 Takashi Sakamoto 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.
Comment 26 Takashi Sakamoto 2012-05-18 05:33:37 PDT
Created attachment 142694 [details]
Patch
Comment 27 Takashi Sakamoto 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
Comment 28 Hajime Morrita 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?
Comment 29 Takashi Sakamoto 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
Comment 30 Takashi Sakamoto 2012-05-28 20:40:50 PDT
Created attachment 144435 [details]
Patch
Comment 31 Hajime Morrita 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.
Comment 32 Takashi Sakamoto 2012-05-29 22:48:49 PDT
Created attachment 144706 [details]
Patch
Comment 33 Build Bot 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
Comment 34 Early Warning System Bot 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
Comment 35 Early Warning System Bot 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
Comment 36 Build Bot 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
Comment 37 Gyuyoung Kim 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
Comment 38 WebKit Review Bot 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
Comment 39 Build Bot 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
Comment 40 Takashi Sakamoto 2012-05-30 22:16:33 PDT
Created attachment 144986 [details]
Patch
Comment 41 Takashi Sakamoto 2012-06-01 00:23:02 PDT
Created attachment 145225 [details]
Patch
Comment 42 Dimitri Glazkov (Google) 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 :)
Comment 43 Takashi Sakamoto 2012-06-06 02:28:20 PDT
Created attachment 145967 [details]
Patch
Comment 44 Takashi Sakamoto 2012-06-06 03:04:36 PDT
Comment on attachment 145967 [details]
Patch

I canceled because the patch removed ASSERT(settings) in applyProperty.
Comment 45 Takashi Sakamoto 2012-06-06 03:09:53 PDT
Created attachment 145974 [details]
Patch
Comment 46 Takashi Sakamoto 2012-06-06 04:02:31 PDT
Created attachment 145986 [details]
Patch
Comment 47 Takashi Sakamoto 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
Comment 48 Dimitri Glazkov (Google) 2012-06-06 09:03:28 PDT
Comment on attachment 145986 [details]
Patch

Looks good. Morrita-san, your final word?
Comment 49 Hajime Morrita 2012-06-07 21:43:32 PDT
Comment on attachment 145986 [details]
Patch

OK. Let's land and see.
Comment 50 WebKit Review Bot 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>
Comment 51 WebKit Review Bot 2012-06-07 22:29:25 PDT
All reviewed patches have been landed.  Closing bug.