Bug 28890 – Make simple user script injection work

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

This bug is covering the initial landing of basic user content support.  It has lots of issues, but the basic structure and WebKit API are defined.

[Dave Hyatt]

Created attachment 38888 [details]
Patch

[Adam Barth]

This patch is missing the HTML for the test.  :)

[Dave Hyatt]

Created attachment 38889 [details]
Include test and DRT changes

[Aaron Boodman]

Comment on attachment 38888 [details]
Patch

>  void FrameLoader::dispatchDocumentElementAvailable()
>  {
> +    m_frame->injectPageGroupUserContent(InjectContentAtDocumentStart);
>      m_client->documentElementAvailable();
>  }

FYI, I've noticed some flakiness of when this is dispatched. In some cases it
happens after more of the document is available than just the document node. I
have not had time to investigate yet. Ideally, it should be dispatched
*exactly* after the document element is available, but before anything else is
attached to it.

> @@ -66,6 +67,11 @@ namespace WebCore {
>          StorageNamespace* localStorage();
>  #endif
>  
> +        void addUserContent(const String& source, const KURL&, const String& mimeType, const Vector<String>& patterns,
> +                            const String& contentID, UserContentInjectTime);
> +        void removeUserContent() { m_userContent.clear(); }
> +        const Vector<UserContent>& userContent() const { return m_userContent; }

Maybe clearUserContent() instead of removeUserContent()? To me, "remove"
implies removing a single entry.

Also, fyi, in Chromium, extensions can be installed and uninstalled without
restarting the browser, so this API will require us clearing and the list and
repopulating it. That might be OK since uninstallation is uncommon, just wanted
you to be aware of it.

[Adam Roben (:aroben)]

Comment on attachment 38889 [details]
Include test and DRT changes

> +    if (userContent.isCSSStylesheet())
> +        injectUserStylesheet(userContent);
> +    else if (userContent.isJavascript())

That should be "isJavaScript".

> +void PageGroup::addUserContent(const String& source, const KURL& url, const String& mimeType,
> +                               const Vector<String>& patterns, const String& contentID,
> +                               UserContentInjectTime injectTime)
> +{
> +    m_userContent.append(UserContent(source, url, mimeType, patterns, contentID, injectTime));
> +}

As we discussed, "worldID" seems like a better term than "contentID".

> +    UserContent(const String& source, const KURL& url, const String& mimeType,
> +                const Vector<String>& patterns, const String& contentID,
> +                UserContentInjectTime injectTime)
> +    : m_source(source)
> +    , m_url(url)
> +    , m_mimeType(mimeType)
> +    , m_patterns(patterns)
> +    , m_contentID(contentID)
> +    , m_injectTime(injectTime)
> +    {
> +    }

The initializer list should be indented.

It's a little too bad that this will copy the patterns Vector. You could add a static constructor function that has the word "adopt" in it somewhere, but it's hard to see how to make that clear given all the parameters this constructor takes. Or you could make this constructor take a PassOwnPtr<Vector<String> >.

> +    UserContentInjectTime injectTime() const { return m_injectTime; }

I think "UserContentInjectionTime" and "injectionTime" and "m_injectionTime" would be a little better.

> +    bool isCSSStylesheet() const { return mimeType() == "text/css"; }
> +    bool isJavascript() const { return mimeType() == "text/javascript"; }

Are MIME types case-sensitive? RFC2045 seems to say that they aren't.

It seems expensive to do a string comparison every time. Can we just have a UserContentType enum instead?

> +    PageGroup* pageGroup = PageGroup::pageGroup(group);
> +    ASSERT(pageGroup);
> +    if (!pageGroup)
> +        return;

I guess this assertion might be helpful for WebKit clients that are using a Debug build of WebKit for development, but it doesn't seem like a true "assertion" in the sense of being something WebKit can guarantee. Maybe a LOG_ERROR would be better? (This same comment applies to +_removeUserContentFromGroup:).

> +    // Convert the patterns into a Vector.
> +    Vector<String> patternsVector;
> +    for (unsigned i = 0; i < [patterns count]; ++i) {
> +        if ([[patterns objectAtIndex:i] isKindOfClass:[NSString class]])
> +            patternsVector.append(String((NSString*)[patterns objectAtIndex:i]));
> +    }

It would be better to call -count only once, and to call -objectAtIndex: only once per loop iteration.

> +    pageGroup->addUserContent(source, url, mimeType, patternsVector, contentID, (UserContentInjectTime)injectTime);

It would be nice not to rely on UserContentInjectTime and WebUserContentInjectTime having the same values.

> +typedef enum {
> +    WebInjectAtDocumentStart,
> +    WebInjectAtDocumentEnd,
> +} WebUserContentInjectTime;
> +

I think WebUserContentInjectionTime would be a better name. (The parameter to +_addUserContentToGroup: should probably be renamed, too.)

You'll need to add your new test to all the non-Mac Skipped files.

Here are some ideas for more tests:
* test InjectAtDocumentStart
* test that scripts run in iframes
* test that scripts run after navigations

[Adam Barth]

Comment on attachment 38889 [details]
Include test and DRT changes

If I were writing this patch, I'd have two subclasses of UserContent, one for CSS and once for JavaScript.  Then you wouldn't have bizarre semantics like ignoring the injection time for a mimeType of text/css.  Also Frame::injectUserContent wouldn't have to do fake RTTI to do the injection.  We could just use a virtual function like Stroustrup intended.

Also, I wish we had a better representation of a pattern than a string, but we can address that in the patch that implements patterns.

[Dave Hyatt]

Yeah I definitely think subclassing would be good here.... then we could keep aroben's suggested type enum only at the API level, and just go ahead and make UserScripts or UserStyleSheets to hand off to the PageGroup.

[Dave Hyatt]

Created attachment 39013 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 39013 [details]
Patch

> +void Frame::injectUserScriptsForWorld(int worldID, const UserScriptVector& userScripts, UserScriptInjectionTime injectionTime)
> +{
> +    // Make a vector of script source codes.  Call evaluateInNewWorld on the set of scripts.
> +    // FIXME: Need to implement pattern checking.
> +    Vector<ScriptSourceCode> sourceCode;
> +    unsigned count = userScripts.size();
> +    if (count == 0)
> +        return;

We normally use isEmpty() for this.

> +    for (unsigned i = 0; i < count; ++i) {
> +        UserScript* script = userScripts[i].get();
> +        if (script->injectionTime() == injectionTime)
> +            sourceCode.append(ScriptSourceCode(script->source(), script->url()));
> +    }
> +    script()->evaluateInNewWorld(worldID, sourceCode);
> +}

I think this will break the V8 build.

> +void PageGroup::addUserScript(const String& source, const KURL& url, const Vector<String>& patterns, 
> +                              int worldID, UserScriptInjectionTime injectionTime)
> +{
> +    PassOwnPtr<UserScript> userScript(new UserScript(source, url, patterns, worldID, injectionTime));
> +    UserScriptVector* scriptsInWorld;
> +    if (!m_userScripts)
> +        m_userScripts.set(new UserScriptMap);
> +    scriptsInWorld = m_userScripts->get(worldID);
> +    if (!scriptsInWorld)
> +        scriptsInWorld = new UserScriptVector;
> +    scriptsInWorld->append(userScript);
> +    m_userScripts->set(worldID, scriptsInWorld);
> +}

I think it would be slightly better for userScript to be an OwnPtr and then to pass userScript.release() to Vector::append. You can also save a hash lookup by writing this function like this:

OwnPtr<UserScript> userScript(...);
if (!m_userScripts)
    m_userScripts.set(...);
UserScriptVector*& scriptsInWorld = m_userScripts.add(worldID, 0).first->second;
if (!scriptsInWorld)
    scriptsInWorld = new UserScriptVector;
scriptsInWorld->append(userScript.release());

> +void PageGroup::removeUserContentForWorld(int worldID)
> +{
> +    if (m_userScripts) {
> +        UserScriptVector* scriptsInWorld = m_userScripts->get(worldID);
> +        if (scriptsInWorld) {
> +            m_userScripts->remove(worldID);
> +            delete scriptsInWorld;
> +        }
> +    }
> +}

An early return would be nice. You can save a hash lookup by using HashMap::find instead of HashMap::get and passing the resulting iterator to HashMap::remove.

> +    UserScript(const String& source, const KURL& url,
> +               const Vector<String>& patterns, int worldID,
> +               UserScriptInjectionTime injectionTime)
> +    : m_source(source)
> +    , m_url(url)
> +    , m_patterns(patterns)
> +    , m_worldID(worldID)
> +    , m_injectionTime(injectionTime)
> +    {
> +    }

The initializers should be indented.

It's still too bad that we're copying the patterns Vector here.

> ++ (void)_addUserScriptToGroup:(NSString *)groupName source:(NSString *)source url:(NSURL *)url worldID:(int)worldID patterns:(NSArray *)patterns injectionTime:(WebUserScriptInjectionTime)injectionTime
> +{
> +    String group(groupName);
> +    if (group.isEmpty())
> +        return;
> +    
> +    PageGroup* pageGroup = PageGroup::pageGroup(group);
> +    if (!pageGroup)
> +        return;
> +    
> +    // Convert the patterns into a Vector.
> +    Vector<String> patternsVector;
> +    unsigned count = [patterns count];
> +    for (unsigned i = 0; i < count; ++i) {
> +        if ([[patterns objectAtIndex:i] isKindOfClass:[NSString class]])
> +            patternsVector.append(String((NSString*)[patterns objectAtIndex:i]));
> +    }

It would be good to call -count only once, and -objectAtIndex: only once per iteration.

-count returns NSUInteger, which I think is 64 bits wide in 64-bit builds. I wonder if assigning it to an unsigned will break 64-bit builds?

You should probably add some comments and assertions about 0 not being a valid worldID.

I wonder if we should use a typedef for worldID?

The distinction between "removeUserContent" (which operates on a single world) and "clearUserContent" (which operates on all worlds) isn't super-clear to me from the names. Maybe "clearUserContentForAllWorlds" would be better?

You should try to add the new header files to as many build systems as you can.

Let's see some ChangeLogs!

[Dave Hyatt]

Created attachment 39024 [details]
Patch that is ready for official review

[David Levin]

fyi, there are merge markers in WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp

[Adam Roben (:aroben)]

Comment on attachment 39024 [details]
Patch that is ready for official review

> +void ScriptController::evaluateInIsolatedWorld(int worldID, const Vector<ScriptSourceCode>& sources)
> +{
> +    // FIXME: It's not obvious what extensionGroup is.  Does world ID represent the same concept? -- hyatt
> +    m_proxy->evaluateInNewWorld(sources, worldID);
> +}

I'd like to get a V8-y person to look at his before you land.

> +void Frame::injectUserScriptsForWorld(int worldID, const UserScriptVector& userScripts, UserScriptInjectionTime injectionTime)
> +{
> +    if (userScripts.isEmpty())
> +        return;
> +
> +    // FIXME: Need to implement pattern checking.
> +    Vector<ScriptSourceCode> sourceCode;
> +    unsigned count = userScripts.size();
> +    if (count == 0)
> +        return;

No need to check both isEmpty() and count == 0.

> +void PageGroup::addUserScript(const String& source, const KURL& url, const Vector<String>& patterns, 
> +                              int worldID, UserScriptInjectionTime injectionTime)
> +{
> +    if (worldID <= 0)
> +        return;

If we don't allow negative worldIDs, maybe we should just use unsigned instead of int? Then the only disallowed value would be 0 (and I guess UINT_MAX, due to HashTable restrictions).

> +void PageGroup::removeUserContentForWorld(int worldID)
> +{
> +    if (!m_userScripts)
> +        return;
> +
> +    UserScriptMap::iterator it = m_userScripts->find(worldID);
> +    if (it->second) {
> +        m_userScripts->remove(it);
> +        delete it->second;
> +    }
> +}

It's a little more standard to check "if (it != m_userScripts->end())". That way even if we somehow screw up and end up with a null UserScriptVector* in the map, you'll still remove the entry.

I'm still a little concerned about copying the patterns Vector in the UserScript constructor. Hopefully most scripts won't have too many patterns.

You need to add the new test to all the non-Mac Skipped files. You should add the new header files to all the build systems/project files you can. r=me if you do both of those things (and get a V8 person to look at the ScriptController change).

[Dave Hyatt]

Fixed in r48057.

[Adam Roben (:aroben)]

<rdar://problem/7186923>