[webkit-reviews] review denied: [Bug 43457] Make the cascade level of "user" styles configurable : [Attachment 63481] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Aaron Boodman <aa at chromium.org>'s
request for review:
Bug 43457: Make the cascade level of "user" styles configurable
https://bugs.webkit.org/show_bug.cgi?id=43457

Attachment 63481: Patch
https://bugs.webkit.org/attachment.cgi?id=63481&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>

> +    {
> +	   OwnPtr<CSSRuleSet> tempUserStyle(new CSSRuleSet);
>	   if (pageUserSheet)
> -	       m_userStyle->addRulesFromSheet(pageUserSheet, *m_medium, this);
> +	       tempUserStyle->addRulesFromSheet(pageUserSheet, *m_medium,
this);
>	   if (pageGroupUserSheets) {
>	       unsigned length = pageGroupUserSheets->size();
> -	       for (unsigned i = 0; i < length; i++)
> -		  
m_userStyle->addRulesFromSheet(pageGroupUserSheets->at(i).get(), *m_medium,
this);
> +	       for (unsigned i = 0; i < length; i++) {
> +		   if (pageGroupUserSheets->at(i)->isUserStyleSheet())
> +		      
tempUserStyle->addRulesFromSheet(pageGroupUserSheets->at(i).get(), *m_medium,
this);
> +		   else
> +		      
m_authorStyle->addRulesFromSheet(pageGroupUserSheets->at(i).get(), *m_medium,
this);
> +	       }
>	   }
> +
> +	   if (tempUserStyle->m_ruleCount > 0)
> +	       m_userStyle = tempUserStyle.leakPtr();
>      }

Unless I'm misreading this, you have indented code inside braces but no longer
have an if statement?  Just pull the code out of the braces.


> -	       parsedSheet->setIsUserStyleSheet(true);
> +
> +	       if (sheet->level() == UserStyleSheet::UserLevel)
> +		   parsedSheet->setIsUserStyleSheet(true);
> +	       else {
> +		   ASSERT(sheet->level() == UserStyleSheet::AuthorLevel);
> +		   parsedSheet->setIsUserStyleSheet(false);
> +	       }
> +

I wouldn't worry about the assert here... just do:

parsedSheet->setIsUserStyleSheet(sheet->level() == UserStyleSheet::UserLevel)

More compact.


>      OwnPtr<UserStyleSheet> userStyleSheet(new UserStyleSheet(source, url,
whitelist, blacklist, injectedFrames));
> +    userStyleSheet->setLevel(level);

Just make level part of the constructor, so that you don't have to have a 2nd
line with a setLevel call.

Everything else looks fine.