[Webkit-unassigned] [Bug 36665] Page Cache does not work on Android with V8

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36665





--- Comment #4 from Adam Barth <abarth at webkit.org>  2010-03-26 10:08:01 PST ---
(From update of attachment 51755)
>+	Note that isolated worlds are not taken into account in any way,

Tab?

>+ * Copyright 2010, The Android Open Source Project

I'm not sure we can accept this sort of copyright line.  We've been told not to
accept "the chromium authors".  Not sure how that applies here.

>+#if PLATFORM(ANDROID)

Why is this android specific?  Seems like anyone who wants to use page cache
and v8 will want this.

>+ScriptCachedFrameData::ScriptCachedFrameData(Frame* frame) : m_domWindow(frame->domWindow())

Please move initializer to the next line.

>+    m_context->ReattachGlobal(m_global);

I'm worried that m_global might be the global object from an isolated world. 
Do we understand how this code interacts with isolated worlds?  We might want
something either here or when we grab the global to make sure that we're
getting the main world's object.

>- * Copyright (C) 2008, 2009 Google Inc. All rights reserved.
>+ * Copyright (C) 2008, 2009, 2010 Google Inc. All rights reserved.

You just need the latest year.

>+#if PLATFORM(CHROMIUM)
> // We don't use WebKit's page caching, so this implementation is just a stub.

Gain, this had nothing to do with CHROMIUM / ANDROID.  There's already a bit
somewhere that says whether PageCache is enabled.  We should key off of that.

>+// FIXME: the right guard should be ENABLE(PAGE_CACHE). Replace with the right guard, once
>+// https://bugs.webkit.org/show_bug.cgi?id=35061 is fixed.
>+//
>+// On Android we do use WebKit's page cache. For now we don't support isolated worlds
>+// so our implementation does not take them into account.

Ah, I see.  Can you add some nasty ASSERTS about this so we don't turn this on
by accident and not realize the mess we've gotten ourselves into?

>+        v8::Persistent<v8::Object> m_global;
>+        v8::Persistent<v8::Context> m_context;

Please use OwnHandle so you don't have to manually alloc and free these
persistent handles.

>+void V8DOMWindowShell::setContext(v8::Handle<v8::Context> context)
>+{
>+    m_context = v8::Persistent<v8::Context>::New(context);
>+}

Does this leak the persistent handle to the old m_context?  Do we need to
ASSERT that m_context is always empty here?  Should we use OwnHandle to help us
manage the lifetime of the persistent handle?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.