Created attachment 187238 [details] Patch

Comment on attachment 187238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187238&action=review Thanks for this patch! Informally, LGTM :). I've added a few comments. > Source/WebCore/inspector/InspectorCSSAgent.cpp:691 > + RenderFlowThread* flowThread = element->renderer()->enclosingRenderFlowThread(); I would ASSERT flowThread is not-null here. If the inRenderFlowThread bit is set, there should be an enclosingRenderFlowThread available. > Source/WebCore/inspector/InspectorCSSAgent.cpp:699 > + RefPtr<CSSRuleList> matchedRules = styleResolver->styleRulesForElement(element, StyleResolver::AllCSSRules, regionList); So this basically means that we will show all the region style matching rules for the element alongside the rest of the matching rules? I wonder if this can be confusing for the users reading the matching rules list. There could be conflicting rules in the list that don't even apply because the element doesn't flow in the appropriate region of the chain. Another option would be to show only the matching rules for the regions in the region range for that element. The problem is we have this range computed only for blocks.

Comment on attachment 187238 [details] Patch Attachment 187238 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16432604 New failing tests: compositing/absolute-inside-out-of-view-fixed.html animations/3d/matrix-transform-type-animation.html accessibility/accessibility-node-memory-management.html canvas/philip/tests/2d.canvas.reference.html animations/additive-transform-animations.html

Comment on attachment 187238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187238&action=review > Source/WebCore/css/StyleResolver.cpp:2096 > +void StyleResolver::collectRulesForStyleRulesForElement(unsigned rulesToInclude, RenderRegion* regionForStyling) Should it be "collectStyleRulesForElement" instead? >> Source/WebCore/inspector/InspectorCSSAgent.cpp:699 >> + RefPtr<CSSRuleList> matchedRules = styleResolver->styleRulesForElement(element, StyleResolver::AllCSSRules, regionList); > > So this basically means that we will show all the region style matching rules for the element alongside the rest of the matching rules? I wonder if this can be confusing for the users reading the matching rules list. There could be conflicting rules in the list that don't even apply because the element doesn't flow in the appropriate region of the chain. Another option would be to show only the matching rules for the regions in the region range for that element. The problem is we have this range computed only for blocks. Agree with Andrei - this will result in the users' confusion. Any further ideas about how to visually represent the data? Ideally, I believe, there should be some API extension - either a boolean flag to this method or an entirely new method.

Created attachment 187790 [details] Patch

Created attachment 187818 [details] Patch

Comment on attachment 187238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187238&action=review Thank you for reviewing. >> Source/WebCore/css/StyleResolver.cpp:2096 >> +void StyleResolver::collectRulesForStyleRulesForElement(unsigned rulesToInclude, RenderRegion* regionForStyling) > > Should it be "collectStyleRulesForElement" instead? I see. Done. >> Source/WebCore/inspector/InspectorCSSAgent.cpp:691 >> + RenderFlowThread* flowThread = element->renderer()->enclosingRenderFlowThread(); > > I would ASSERT flowThread is not-null here. If the inRenderFlowThread bit is set, there should be an enclosingRenderFlowThread available. Done. >>> Source/WebCore/inspector/InspectorCSSAgent.cpp:699 >>> + RefPtr<CSSRuleList> matchedRules = styleResolver->styleRulesForElement(element, StyleResolver::AllCSSRules, regionList); >> >> So this basically means that we will show all the region style matching rules for the element alongside the rest of the matching rules? I wonder if this can be confusing for the users reading the matching rules list. There could be conflicting rules in the list that don't even apply because the element doesn't flow in the appropriate region of the chain. Another option would be to show only the matching rules for the regions in the region range for that element. The problem is we have this range computed only for blocks. > > Agree with Andrei - this will result in the users' confusion. Any further ideas about how to visually represent the data? Ideally, I believe, there should be some API extension - either a boolean flag to this method or an entirely new method. Yes, this new styleRulesForElement collects all the region style matching rules for the element alongside the rest of the matching rules. We will show all the region style matching rules. So I tried "a boolean flag to this method", i.e. added includeRegion to getMatchedStylesForNode.

Attachment 187818 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/inspector/styles/get-set-stylesheet-text.html', u'LayoutTests/inspector/styles/styles-new-API.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorCSSAgent.cpp', u'Source/WebCore/inspector/InspectorCSSAgent.h', u'Source/WebCore/inspector/front-end/CSSStyleModel.js']" exit_code: 1 Source/WebCore/inspector/InspectorCSSAgent.h:111: The parameter name "pseudoIdMatches" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 187818 [details] Patch Attachment 187818 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16513362 New failing tests: inspector/styles/region-style-crash.html

Comment on attachment 187818 [details] Patch Attachment 187818 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16537016 New failing tests: inspector/styles/region-style-crash.html

Comment on attachment 187818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187818&action=review > Source/WebCore/inspector/InspectorCSSAgent.cpp:698 > +#endif #else UNUSED_PARAM(includeRegion);

Created attachment 188979 [details] Patch

Attachment 188979 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/inspector/styles/get-set-stylesheet-text.html', u'LayoutTests/inspector/styles/styles-new-API.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorCSSAgent.cpp', u'Source/WebCore/inspector/InspectorCSSAgent.h', u'Source/WebCore/inspector/front-end/CSSStyleModel.js']" exit_code: 1 Source/WebCore/inspector/InspectorCSSAgent.h:113: The parameter name "pseudoIdMatches" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 187818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187818&action=review Thank you for reviewing. I would like to ask how to test this change. Currently I disabled CSS region support in Source/WebCore/inspector/front-end/CSSStyleModel.js, i.e. needRegion is always false. So no layout test can check whether inspector can obtain matched region styles or not. Should I add one more parameter to getMatchedStylesAsync and change all js and html that use getMatchedStylesAsync? >> Source/WebCore/inspector/InspectorCSSAgent.cpp:698 >> +#endif > > #else > UNUSED_PARAM(includeRegion); Done.

Comment on attachment 188979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188979&action=review Thanks for working on this patch! > Source/WebCore/ChangeLog:12 > + getMatchedStyleForNode should provide a RenderRegionList, which is > + obtained from a given node's render flow thread, for StyleResolver. > + Without this, whether StyleResolver returns CSS region styles or not > + depends on its internal state: regionForStyling. This would probably > + make region-style-crash layout test fail or flaky. I think regionForStyling is reset in the clear() method. Do you have a crash for it? Maybe we should fix just that first. > Source/WebCore/ChangeLog:14 > + No new tests. CSSStyleMode.js always disables obtaining matched region Why not add a test to check the collected rules? > Source/WebCore/css/StyleResolver.cpp:2062 > + if (regionList) { Looks like this part changed recently. There's an ElementRuleCollector allocated on the stack now. > Source/WebCore/css/StyleResolver.h:237 > + PassRefPtr<CSSRuleList> pseudoStyleRulesForElement(Element*, PseudoId, unsigned rulesToInclude = AllButEmptyCSSRules, const RenderRegionList* regionListForStyling = 0); I think pseudo styles are not supported in the region styling @rules. For example, I don't think that :hover would work with region styling at the moment. > Source/WebCore/inspector/InspectorCSSAgent.cpp:729 > +#if ENABLE(CSS_REGIONS) You should not need the ENABLE(CSS_REGIONS) flag. It's only used when parsing CSS properties related to regions. > Source/WebCore/inspector/InspectorCSSAgent.cpp:731 > + if (element->renderer() && element->renderer()->inRenderFlowThread()) { In this case renderer()->inRenderFlowThread() changed to "renderer()->flowThreadState() == RenderObject::InsideOutOfFlowThread". > Source/WebCore/inspector/InspectorCSSAgent.cpp:732 > + RenderFlowThread* flowThread = element->renderer()->enclosingRenderFlowThread(); enclosingRenderFlowThread becomes locateFlowThreadContainingBlock > Source/WebCore/inspector/InspectorCSSAgent.cpp:751 > + nit: remove empty lines.

Comment on attachment 188979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188979&action=review > Source/WebCore/inspector/Inspector.json:2412 > + { "name": "includeInherited", "type": "boolean", "optional": true, "description": "Whether to include inherited styles (default: true)." }, > + { "name": "includeRegion", "type": "boolean", "optional": true, "description": "Whether to include region styles (default: true)." } The other optional flags control different results that get passed to the callback. The includeRegion flag does not trigger another parameter in the callback, it just includes region rules in the existing matched lists (I think…) Why does this need to be an option? Why would you want to pass false?

<rdar://problem/17898290>

CSS Regions was removed from WebKit. No need for Web Inspector support.