[webkit-reviews] review granted: [Bug 28890] Make simple user script injection work : [Attachment 39024] Patch that is ready for official review

[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 28890: Make simple user script injection work
https://bugs.webkit.org/show_bug.cgi?id=28890

Attachment 39024: Patch that is ready for official review
https://bugs.webkit.org/attachment.cgi?id=39024&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +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).