[webkit-dev] Database in Worker context

[Michael Nordman]

>  not clear to me what benefit could be had by effectively splitting a
class in two

Perhaps more easily worked on by multiple hands in parallel with fewer
conflicts. Various bits of "database stuff" are better encapsulated in
database specific classes (so maybe easier to learn how the system works
when code snorkling).

Either way would work of course, the direction eric's heading seems like a
clean way to factor things.


On Thu, Dec 10, 2009 at 11:09 AM, Eric Uhrhane <ericu at chromium.org> wrote:

> On Wed, Dec 9, 2009 at 6:55 PM, Dmitry Titov <dimich at chromium.org> wrote:
> > Thanks for the link to the prototype code!
> > If I understand it right and the DatabaseManager can not have any other
> > relationship with ScriptExecutionContext but 1-1 and their lifetime is
> the
> > same then it's not clear to me what benefit could be had by effectively
> > splitting a class in two. The fact that DatabaseManager is RefCounted
> also
> > hints that some other object could take ownership of it as well outside
> of
> > lifetime of ScriptExecutionContext, but is this true?
>
> That's there because the DatabaseThread [and therefore the Database]
> can stick around for a bit after the ScriptExecutionContext is deleted
> if I make it non-refcounted.  If I move the DatabaseManager parts into
> the ScriptExecutionContext, then I have to make sure that the SEC
> sticks around until after the Database and DatabaseThread have gone
> away.
>
> I can do that--the benefit I thought I was getting wasn't one of
> object lifetime.  I just thought the classes were cleaner this way.
> But if you really think it's better to have it all in together, I can
> certainly move it.
>
> > ScriptExecutionContext collects functionality common for Workers and
> > Documents, and as such is a home for a few 'groups' of methods, like a
> few
> > methods to deal with MessagePorts. MessagePort, in turn, has a raw
> pointer
> > back to ScriptExecutionContext - so it's clear that MessagePorts do not
> > outlive the SEC. Same pattern could be used for
> > ScriptExecutionContext/Database, for consistency. It also simplifies
> design
> > a bit - no need for a special refcounted manager class, and things
> > like callOnJavaScriptThread could be replaced by SEC::postTask() which is
> > already implemented.
>
> The point of CallOnJavascriptThread is that the JS thread is the Main
> Thread in the Document context, but [in Chromium's implementation] NOT
> in the Worker context.  I needed a virtual method to distinguish
> between the two cases, since the Database code previously treated
> isMainThread as synonymous with isJavascriptThread.  But of course
> that virtual method can be moved to Document and SEC instead of DDM
> and WDM.
>
> > On Wed, Dec 9, 2009 at 5:21 PM, Eric Uhrhane <ericu at chromium.org> wrote:
> >>
> >> On Wed, Dec 9, 2009 at 4:11 PM, Dmitry Titov <dimich at chromium.org>
> wrote:
> >> > On Wed, Dec 9, 2009 at 12:58 PM, Eric Uhrhane <ericu at chromium.org>
> >> > wrote:
> >> >>
> >> >>  I've pulled the database-related members out of Document and made a
> >> >> new class for them, DatabaseManager.  An instance of that is owned by
> >> >> each ScriptExecutionContext.  There are two flavors,
> >> >> DocumentDatabaseManager and WorkerDatabaseManager.  They're not very
> >> >> different, but in a few cases it was necessary to distinguish between
> >> >> them
> >> >
> >> > I don't see your code, just generic thought: If those classes are
> small,
> >> > then perhaps ScriptExecutionContext could absorb a couple of methods
> to
> >> > deal
> >> > with that? Some of them could be virtual and implemented differently
> on
> >> > Document and WorkerContext.
> >> > Dmitry
> >>
> >> You don't see the code because I sent this as an early
> >> warning--there's nothing checked in yet.
> >> The classes aren't huge, and in fact most of the code was in Document
> >> before, but given that it's all to do with a separable concept [the
> >> Database], I thought it nicer to pull it out.  Let's
> >> see...DatabaseManger.cpp is 172 lines [including copyright headers,
> >> etc.] and the header is 107.  Seems like a lot to sprinkle through
> >> ScriptExecutionContext, Database, and WorkerContext.  You can take a
> >> look at my current code [where I'm backing it up--this won't actually
> >> be sent for review from this repository] at
> >> http://codereview.chromium.org/401016/show [mostly storage + v8
> >> bindings] and http://codereview.chromium.org/464018/show [mostly
> >> Chrome browser process stuff] and see what you think.
> >>
> >> Thanks,
> >>
> >>     Eric
> >
> >
> > _______________________________________________
> > webkit-dev mailing list
> > webkit-dev at lists.webkit.org
> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >
> >
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20091210/d5c5f793/attachment.html>