+1 to small patches. I've reviewed a couple of iterations of the patch in https://bugs.webkit.org/show_bug.cgi?id=22725 and it was so far manageable, although it could be split in 2 at least (inheritance change and WorkerThread termination change). For future patches, definitely lets split into smaller ones, 60kb is on a biggish side.
There is no DatabaseManager class anymore since the database-supporting methods are being moved to ScriptExecutionContext from which both Document and WorkerContext are derived from. Having a 'Context' to own another 'Manager' seemed like too much layers. I lean to r+ the first patch after latest set of notes is addressed, more eyes are always welcome though :-) Dmitry On Thu, Jan 14, 2010 at 3:06 PM, Darin Adler <da...@apple.com> wrote: > On Jan 14, 2010, at 2:35 PM, Eric Uhrhane wrote: > > > I think it would have been hard to break this chunk any smaller. > > Any time you have a chunk and you’re wondering how it could be broken up > smaller, please feel free to ask that question after cc'ing me on a bug. > Maciej and I, in particular, have a lot of experience with this, much of > dating from the first year of the Safari project. I made changes that were > too large and learned “no apologies” strategies to break them up into much > smaller pieces that were easier to get right and to read and review. > > -- Darin > > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
