https://bugs.webkit.org/show_bug.cgi?id=22725
Dmitry Titov <dimich at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #46968|review? |review-
Flag| |
--- Comment #24 from Dmitry Titov <dimich at chromium.org> 2010-01-20 12:40:39 PST ---
(From update of attachment 46968)
Just a few more. Lets try to get to a clean r+ so we can flip cq+ on it.
> Index: WebCore/page/DOMWindow.cpp
> Document* document = m_frame->document();
> - if (!document->securityOrigin()->canAccessDatabase())
> + if (!document->securityOrigin()->canAccessDatabase()) {
> + ec = SECURITY_ERR;
> return 0;
> + }
This is a new addition to the previously reviewed patch. It will take less time
if we keep the scope of the change rather then expand it. In this particular
case, it's a change of already shipped behavior which may have compatibility
issues. Basically, if some website will happen to be broken, this change may
have to be reverted. Adding Database to Workers is one thing (no compatibility
issues since the stuff is new), changing behavior of Database on the regular
pages is a different matter, and should have a separate bug. Lets move this
into separate bug.
> Index: WebCore/storage/Database.cpp
>
> Database::~Database()
> {
> + // The reference to the ScriptExecutionContext needs to be cleared on the JavaScript thread. If we're on that thread already, we can just let the RefPtr's destruction do the dereffing.
> + if (!m_scriptExecutionContext->isContextThread()) {
> + m_scriptExecutionContext->postTask(DerefContextTask::create());
> + m_scriptExecutionContext.release().releaseRef();
This is something we'll want to cleanup, to possibly avoid having a destructor
that can run on either thread, since Database may have ownership of
non-thread-safe objects... But it's outside of scope of this change since this
one simply refactors the existing code.
> void Database::close()
> {
> if (!m_opened)
> return;
>
> + // Must ref() before calling databaseThread()->recordDatabaseClosed().
> + RefPtr<Database> holder = this;
The pattern used often in WebKit is to create a RefPtr named 'protect' at the
very beginning of the function and then not use it in the code (since 'this' is
guaranteed to be valid), like this:
RefPtr<Database> protect = this;
> + m_scriptExecutionContext->databaseThread()->unscheduleDatabaseTasks(this);
> + m_scriptExecutionContext->postTask(ContextRemoveOpenDatabaseTask::create(holder));
Can use 'this' for both, for consistency. It's guaranteed protected.
> Index: LayoutTests/ChangeLog
The Layouttest change should go with the DOMWindow change, in a different bug.
> + No bug. This is part of the checkin for
> + https://bugs.webkit.org/show_bug.cgi?id=22725
Normally, all the ChangeLog files that are part of the same checkin, refer to
the same bug number. So the first line here would not be needed.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.