Follow-on to "Restore in-memory parser cache for V8 compile"

Follow-on to "Restore in-memory parser cache for V8 compile"
(rrev.com/528013002).

Fixes for review comments not handled in the original CL.

BUG=399580
R=haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181554

[vogelheim, 2014-09-05 17:56:09]

Patchset #1 (id:1) has been deleted

[haraken, 2014-09-05 18:04:25]

LGTM, thanks for the very quick follow-up!

Then how about adding the helper functions in both V8ScriptRunner and the test?
I'd like to avoid hard-coding this kind of tag numbers in the code base.

[vogelheim, 2014-09-08 09:07:51]

On 2014/09/05 18:04:25, haraken wrote:
> LGTM, thanks for the very quick follow-up!
> 
> Then how about adding the helper functions in both V8ScriptRunner and the
test?
> I'd like to avoid hard-coding this kind of tag numbers in the code base.

Done.

[vogelheim, 2014-09-08 09:08:01]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-09-08 09:08:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vogelheim@chromium.org/549533002/40001

[commit-bot: I haz the power, 2014-09-08 09:08:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-08 09:08:48]

Failed to apply patch for Source/bindings/core/v8/V8ScriptRunnerTest.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/core/v8/V8ScriptRunnerTest.cpp
  Hunk #1 succeeded at 5 with fuzz 2.
  Hunk #2 succeeded at 17 (offset 1 line).
  Hunk #3 succeeded at 33 (offset 1 line).
  Hunk #4 FAILED at 83.
  Hunk #5 succeeded at 105 (offset 1 line).
  1 out of 5 hunks FAILED -- saving rejects to file
Source/bindings/core/v8/V8ScriptRunnerTest.cpp.rej

Patch:       Source/bindings/core/v8/V8ScriptRunnerTest.cpp
Index: Source/bindings/core/v8/V8ScriptRunnerTest.cpp
diff --git a/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
b/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
index
2dbb0c5e35be6902912dcfd47fc25aaeca368e45..5939bc5154f339159c6a7fdc2420cf596679b6e4
100644
--- a/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
+++ b/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
@@ -5,6 +5,7 @@
 #include "config.h"
 #include "bindings/core/v8/V8ScriptRunner.h"
 
+#include "bindings/core/v8/V8Binding.h"
 #include "core/fetch/ScriptResource.h"
 #include <gtest/gtest.h>
 #include <v8.h>
@@ -15,15 +16,9 @@ namespace {
 
 class V8ScriptRunnerTest : public ::testing::Test {
 public:
-    V8ScriptRunnerTest() { }
+    V8ScriptRunnerTest() : m_scope(v8::Isolate::GetCurrent()) { }
     virtual ~V8ScriptRunnerTest() { }
 
-    static void SetUpTestCase()
-    {
-        cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2;
-        cacheTagCode = cacheTagParser + 1;
-    }
-
     virtual void SetUp() OVERRIDE
     {
         // To trick various layers of caching, increment a counter for each
@@ -37,43 +32,39 @@ public:
         m_resource.clear();
     }
 
-    v8::Isolate* isolate()
+    v8::Isolate* isolate() const
     {
-        return v8::Isolate::GetCurrent();
+        return m_scope.isolate();
     }
-    v8::Local<v8::String> v8String(const char* value)
-    {
-        return v8::String::NewFromOneByte(
-            isolate(), reinterpret_cast<const uint8_t*>(value));
-    }
-    v8::Local<v8::String> v8String(const WTF::String& value)
-    {
-        return v8String(value.ascii().data());
-    }
-    WTF::String code()
+    WTF::String code() const
     {
         // Simple function for testing. Note:
         // - Add counter to trick V8 code cache.
         // - Pad counter to 1000 digits, to trick minimal cacheability
threshold.
         return WTF::String::format("a = function() { 1 + 1; } // %01000d\n",
counter);
     }
-    WTF::String filename()
+    WTF::String filename() const
     {
         return WTF::String::format("whatever%d.js", counter);
     }
-    WTF::String url()
+    WTF::String url() const
     {
         return WTF::String::format("http://bla.com/bla%d", counter);
     }
+    unsigned tagForParserCache() const
+    {
+        return StringHash::hash(v8::V8::GetVersion()) * 2;
+    }
+    unsigned tagForCodeCache() const
+    {
+        return tagForParserCache() + 1;
+    }
 
     bool compileScript(V8CacheOptions cacheOptions)
     {
-        v8::HandleScope handleScope(isolate());
-        v8::Local<v8::Context> context = v8::Context::New(isolate());
-        v8::Context::Scope contextScope(context);
         return !V8ScriptRunner::compileScript(
-            v8String(code()), filename(), WTF::TextPosition(),
m_resource.get(),
-            isolate(), NotSharableCrossOrigin, cacheOptions)
+            v8String(isolate(), code()), filename(), WTF::TextPosition(),
+            m_resource.get(), isolate(), NotSharableCrossOrigin, cacheOptions)
             .IsEmpty();
     }
 
@@ -92,14 +83,11 @@ public:
 protected:
     WTF::OwnPtr<ResourceRequest> m_resourceRequest;
     WTF::OwnPtr<ScriptResource> m_resource;
+    V8TestingScope m_scope;
 
-    static unsigned cacheTagParser;
-    static unsigned cacheTagCode;
     static int counter;
 };
 
-unsigned V8ScriptRunnerTest::cacheTagParser = 0;
-unsigned V8ScriptRunnerTest::cacheTagCode = 0;
 int V8ScriptRunnerTest::counter = 0;
 
 TEST_F(V8ScriptRunnerTest, resourcelessShouldPass)
@@ -113,43 +101,43 @@ TEST_F(V8ScriptRunnerTest, emptyResourceDoesNothing)
 {
     setEmptyResource();
     EXPECT_TRUE(compileScript(V8CacheOptionsOff));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForParserCache()));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForCodeCache()));
 
     EXPECT_TRUE(compileScript(V8CacheOptionsParse));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForParserCache()));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForCodeCache()));
 
     EXPECT_TRUE(compileScript(V8CacheOptionsCode));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForParserCache()));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForCodeCache()));
 }
 
 TEST_F(V8ScriptRunnerTest, defaultOptions)
 {
     setResource();
     EXPECT_TRUE(compileScript(V8CacheOptionsOff));
-    EXPECT_TRUE(m_resource->cachedMetadata(cacheTagParser));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+    EXPECT_TRUE(m_resource->cachedMetadata(tagForParserCache()));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForCodeCache()));
 }
 
 TEST_F(V8ScriptRunnerTest, parseOptions)
 {
     setResource();
     EXPECT_TRUE(compileScript(V8CacheOptionsParse));
-    EXPECT_TRUE(m_resource->cachedMetadata(cacheTagParser));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagCode));
+    EXPECT_TRUE(m_resource->cachedMetadata(tagForParserCache()));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForCodeCache()));
 }
 
 TEST_F(V8ScriptRunnerTest, codeOptions)
 {
     setResource();
     EXPECT_TRUE(compileScript(V8CacheOptionsCode));
-    EXPECT_FALSE(m_resource->cachedMetadata(cacheTagParser));
+    EXPECT_FALSE(m_resource->cachedMetadata(tagForParserCache()));
 
-    // TODO(vogelheim): Code caching is presently still disabled.
-    //                  Enable EXPECT when code caching lands.
-    // EXPECT_TRUE(m_resource->cachedMetadata(cacheTagCode));
+    // FIXME: Code caching is presently still disabled.
+    //        Enable EXPECT when code caching lands.
+    // EXPECT_TRUE(m_resource->cachedMetadata(tagForCodeCache()));
 }
 
 } // namespace

[vogelheim, 2014-09-08 11:17:46]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-09-08 11:18:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vogelheim@chromium.org/549533002/60001

[commit-bot: I haz the power, 2014-09-08 12:22:03]

Committed patchset #3 (id:60001) as 181554