[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 #6 from Andrei Popescu <andreip at google.com>  2010-03-26 11:57:08 PST ---
Hi Adam,

Many thanks for the quick reply.

(In reply to comment #4)
> (From update of attachment 51755 [details])
> >+	Note that isolated worlds are not taken into account in any way,
> 
> Tab?
> 

Fixed.

> >+ * 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.
>

Well, I am from Android and our previous contributions all had the same
copyright line. For example see:

http://trac.webkit.org/browser/trunk/WebCore/page/GeolocationPositionCache.cpp
http://trac.webkit.org/browser/trunk/WebCore/page/Geolocation.cpp

Given these and other precedents, I think it should be fine to keep the current
copyright?

> >+#if PLATFORM(ANDROID)
> 
> Why is this android specific?  Seems like anyone who wants to use page cache
> and v8 will want this.
> 

Ok, removed.

> >+ScriptCachedFrameData::ScriptCachedFrameData(Frame* frame) : m_domWindow(frame->domWindow())
> 
> Please move initializer to the next line.
> 

Moved.

> >+    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.
>

True, although I'd stay away from that until Android starts using isolated
worlds. Else, if Chromium want page cache, somebody who understand isolated
worlds better than me should do this change?


> >- * 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.
> 

Fixed.

> >+#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.
> 

Hmm, I looked but all I found was a bug for it:

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

It's also possible that I missed something. If so, please let me know.

> >+// 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?
> 

Good point. Added a scary assert, it holds on Android but haven't tested on
Chromium. Please have a look, I can add more if you have any suggestions.

> >+        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.
> 

Oh, I didn't know about that one, thanks!

> >+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?

Well spotted, it won't always be empty. And indeed, I added the ASSERT and it
doesn't hold. 

I think it's better to clear the previous context manually and stick to using a
persistent handle. If we move to an OwnHandle, this change becomes quite
intrusive as I would have to change about 30 sites to call get() instead of
accessing m_context directly. But if you really think OwnHandle is better, I
can do that.

Many thanks,
Andrei

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