Bug 107095 – Group parameters (firstRuleIndex and lastRuleIndex) into a parameter object, RuleRange.

• Component: CSS
• Status: RESOLVED FIXED

[Hayato Ito]

This is a follow up patch for bug 106879.

Both firstRuleIndex and lastRuleIndex can also be grouped into a parameter object.

[Hayato Ito]

Created attachment 183124 [details]
factoring.

[Darin Adler]

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.

[Hayato Ito]

Created attachment 183368 [details]
Introduces a RuleRange

[Hayato Ito]

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.

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

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 :)

[Hayato Ito]

Created attachment 184374 [details]
Patch for landing

[Hayato Ito]

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 :)

[WebKit Review Bot]

Comment on attachment 184374 [details]
Patch for landing

Clearing flags on attachment: 184374

Committed r140643: <http://trac.webkit.org/changeset/140643>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.