Bug 107095

Created attachment 183124 [details] factoring.

Comment on attachment 183124 [details] factoring. View in context: https://bugs.webkit.org/attachment.cgi?id=183124&action=review Seems OK, but I have a refinement I think would make this cleaner, so I am going to say review+. > Source/WebCore/css/StyleResolver.h:345 > struct MatchRequest { I think a better design would be to make the ruleSet and scope data members const the way the includeEmptyRules member is, then make firstRuleIndex and lastRuleIndex be plain old int members rather than references. Then we could pass non-const MatchRequest& objects in and handle this in a more natural way. That would also allow us to keep the default initial values for includeEmptyRules and scope, and so be cleaner at the call sites. It’s really strange for const MatchRequest& to “secretly” have some references in it and so be partly an “out” argument. There’s no reason to be twisted like that.

Created attachment 183368 [details] Introduces a RuleRange

Thank you for the review. You are a good reviewer. :) Actually, I and my colleagues were a litte supprised that we could change a value through a reference even if the object is declared 'const'. A reference is a just a pointer internally, so it should not be a surprising fact. But I am feeling that it's confusing for readers and should be avoided. I've tried your suggestions, but holding plain old int members rather than references causes another buden. The caller has to retrive the result from MatchRequest object and copy them to the collect fields of range. I am afraid that it's error-prone. I think that mixing const members (ruleSet, scope, ...) and output parameters into one object is the troublesome principally in this case. So I've decided to separete them. RuleRange is now introduced. This is not a huge win than I expected at first. But that makes code clearer. You gave me r+, but I think this patch should be reviewed again. Could you review it? (In reply to comment #2) > (From update of attachment 183124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183124&action=review > > Seems OK, but I have a refinement I think would make this cleaner, so I am going to say review+. > > > Source/WebCore/css/StyleResolver.h:345 > > struct MatchRequest { > > I think a better design would be to make the ruleSet and scope data members const the way the includeEmptyRules member is, then make firstRuleIndex and lastRuleIndex be plain old int members rather than references. Then we could pass non-const MatchRequest& objects in and handle this in a more natural way. > > That would also allow us to keep the default initial values for includeEmptyRules and scope, and so be cleaner at the call sites. > > It’s really strange for const MatchRequest& to “secretly” have some references in it and so be partly an “out” argument. There’s no reason to be twisted like that.

Let me land this patch in a few days since the pervious patch got r+ and it appears to be relatively safe to land. If there is a concern about this, please let me know.

Comment on attachment 183368 [details] Introduces a RuleRange View in context: https://bugs.webkit.org/attachment.cgi?id=183368&action=review > Source/WebCore/ChangeLog:13 > + (WebCore::StyleResolver::collectMatchingRules): It's customary to explain what's happening in the patch using these lines. I am sure you're just too excited to get this going :)

Created attachment 184374 [details] Patch for landing

Thank you for the review. Let me land this patch after I add a small explanation to the ChangeLog. (In reply to comment #6) > (From update of attachment 183368 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183368&action=review > > > Source/WebCore/ChangeLog:13 > > + (WebCore::StyleResolver::collectMatchingRules): > > It's customary to explain what's happening in the patch using these lines. I am sure you're just too excited to get this going :)

Comment on attachment 184374 [details] Patch for landing Clearing flags on attachment: 184374 Committed r140643: <http://trac.webkit.org/changeset/140643>

All reviewed patches have been landed. Closing bug.