Bug 29102 – Make user stylesheet injection work

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

This bug tracks adding apis for injecting any # of user stylesheets into page groups.

[Dave Hyatt]

Created attachment 39290 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 39290 [details]
Patch

> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,43 @@
> [phone]  Dave Hyatt  <[e-mail]>
> +
> +        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

[Dave Hyatt]

Fixed in r48222.

[Adam Roben (:aroben)]

<rdar://problem/7192001>