[webkit-reviews] review granted: [Bug 29102] Make user stylesheet injection work : [Attachment 39290] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 29102: Make user stylesheet injection work
https://bugs.webkit.org/show_bug.cgi?id=29102

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,43 @@
> +2009-09-09  Dave Hyatt  <hyatt at apple.com>
> +
> +	   Reviewed by Adam Roben.

Getting a little presumptuous, aren't we?

> +	   * WebCore.base.exp:
> +	   * WebCore.gypi:
> +	   * WebCore.vcproj/WebCore.vcproj:
> +	   * WebCore.xcodeproj/project.pbxproj:
> +	   * css/CSSStyleSelector.cpp:
> +	   (WebCore::CSSStyleSelector::CSSStyleSelector):
> +	   * css/CSSStyleSelector.h:
> +	   * dom/Document.cpp:
> +	   (WebCore::Document::Document):
> +	   (WebCore::Document::attach):
> +	   (WebCore::Document::pageGroupUserSheets):
> +	   (WebCore::Document::clearPageGroupUserSheets):
> +	   (WebCore::Document::recalcStyleSelector):
> +	   * dom/Document.h:
> +	   * loader/PlaceholderDocument.cpp:
> +	   (WebCore::PlaceholderDocument::attach):
> +	   * page/PageGroup.cpp:
> +	   (WebCore::PageGroup::addUserStyleSheet):
> +	   (WebCore::PageGroup::removeUserContentForWorld):
> +	   (WebCore::PageGroup::removeAllUserContent):
> +	   * page/PageGroup.h:
> +	   (WebCore::PageGroup::userStyleSheets):
> +	   * page/UserStyleSheet.h: Added.
> +	   (WebCore::UserStyleSheet::UserStyleSheet):
> +	   (WebCore::UserStyleSheet::source):
> +	   (WebCore::UserStyleSheet::url):
> +	   (WebCore::UserStyleSheet::patterns):
> +	   (WebCore::UserStyleSheet::worldID):
> +	   * page/UserStyleSheetTypes.h: Added.

It would be nice to see some file/function-level comments.

> +++ WebCore/WebCore.vcproj/WebCore.vcproj	(working copy)
> @@ -17068,14 +17068,22 @@
>				RelativePath="..\page\Settings.h"
>				>
>			</File>
> -	 <File
> +			   <File
>				RelativePath="..\page\UserScript.h"
>				>
>			</File>
> -	 <File
> +			   <File
>				RelativePath="..\page\UserScriptTypes.h"
>				>
>			</File>
> +			   <File
> +				RelativePath="..\page\UserStyleSheet.h"
> +				>
> +			</File>
> +			   <File
> +				RelativePath="..\page\UserStyleSheetTypes.h"
> +				>
> +			</File>
>			<File
>				RelativePath="..\page\WebKitPoint.h"
>				>

The indentation of the lines you modified doesn't match that of the other
lines. I think you need some more tabs (gasp!).

> +CSSStyleSelector::CSSStyleSelector(Document* doc, StyleSheetList*
styleSheets, CSSStyleSheet* mappedElementSheet,
> +				      CSSStyleSheet* pageUserSheet,
Vector<RefPtr<CSSStyleSheet> >* pageGroupUserSheets,
> +				      bool strictParsing, bool
matchAuthorAndUserStyles)

Can pageGroupUserSheets be a const Vector*?

> +	   if (pageGroupUserSheets) {
> +	       unsigned length = pageGroupUserSheets->size();
> +	       for (unsigned i = 0; i < length; i++)
> +		  
m_userStyle->addRulesFromSheet(static_cast<CSSStyleSheet*>(pageGroupUserSheets-
>at(i).get()), *m_medium, this);
> +	   }

Why is this static_cast needed?

> +Vector<RefPtr<CSSStyleSheet> >* Document::pageGroupUserSheets()

This could be a const member function if you made m_pageGroupSheetCacheValid
and m_pageGroupUserSheets mutable.

Can this return a const Vector*?

> +    if (m_pageGroupSheetCacheValid)
> +	   return m_pageGroupUserSheets.get();

It seems like either both of these variables should have "User" in their name,
or neither should.

> +    m_pageGroupSheetCacheValid = true;
> +    
> +    Page* owningPage = page();
> +    if (!owningPage)
> +	   return 0;
> +	   
> +    const PageGroup& pageGroup = owningPage->group();
> +    const UserStyleSheetMap* sheetsMap = pageGroup.userStyleSheets();
> +    if (sheetsMap) {

You could use an early return here to reduce nesting.

> +	   UserStyleSheetMap::const_iterator end = sheetsMap->end();
> +	   for (UserStyleSheetMap::const_iterator it = sheetsMap->begin(); it
!= end; ++it) {
> +	       const UserStyleSheetVector* sheets = it->second;
> +	       for (unsigned i = 0; i < sheets->size(); ++i) {
> +		   const UserStyleSheet* sheet = sheets->at(i).get();
> +		   RefPtr<CSSStyleSheet> parsedSheet =
CSSStyleSheet::create(this);
> +		   parsedSheet->parseString(sheet->source(), !inCompatMode());
> +		   if (!m_pageGroupUserSheets)
> +		       m_pageGroupUserSheets.set(new
Vector<RefPtr<CSSStyleSheet> >);
> +		   m_pageGroupUserSheets->append(parsedSheet);

You can pass parsedSheet.release() here to save a ref/deref pair.

> +void PageGroup::addUserStyleSheet(const String& source, const KURL& url,
const Vector<String>& patterns, unsigned worldID)
>  {
> -    if (!m_userScripts)
> +    if (worldID == UINT_MAX)
>	   return;

I think we prefer to use std::numeric_limits instead of UINT_MAX and friends.

> +typedef HashMap<int, UserStyleSheetVector*> UserStyleSheetMap;

I think the key type of UserStyleSheetMap should be unsigned, to match your
other uses of world IDs.

> +++ LayoutTests/userscripts/simple-stylesheet.html	(revision 0)
> @@ -0,0 +1,16 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.addUserStyleSheet("div { color: green !important;
}");
> +}
> +</script>
> +</head>
> +<body>
> +<div>This text should be green.</div>
> +<style>
> +div { color: red }
> +</style>
> +</body>
> +</html>

Could you make this a text-only test by checking the computed style of the div
using JS?

You need to add the new test to other platforms' Skipped files.

r=me