Bug 79633 - ScriptExecutionContext has too many ifdef ENABLE(SQL_DATABASE)
Summary: ScriptExecutionContext has too many ifdef ENABLE(SQL_DATABASE)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 79327 79635 79636
  Show dependency tree / graph
 
Reported: 2012-02-26 23:39 PST by Adam Barth
Modified: 2012-02-29 23:46 PST (History)
6 users (show)

See Also:

Attachments
work in progress (23.51 KB, patch)
2012-02-26 23:40 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
work in progress (27.05 KB, patch)
2012-02-27 00:05 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (35.23 KB, patch)
2012-02-27 00:15 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (36.06 KB, patch)
2012-02-27 14:58 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (32.97 KB, patch)
2012-02-29 17:22 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (39.56 KB, patch)
2012-02-29 17:25 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (39.56 KB, patch)
2012-02-29 17:45 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (40.35 KB, patch)
2012-02-29 17:51 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (41.21 KB, patch)
2012-02-29 18:44 PST, Adam Barth 		abarth: commit-queue+
Details | Formatted Diff | Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-02-26 23:39:59 PST 
ScriptExecutionContext has too many ifdef ENABLE(SQL_DATABASE)
Comment 1 Adam Barth 2012-02-26 23:40:33 PST 
Created attachment 128966 [details]
work in progress
Comment 2 Adam Barth 2012-02-26 23:40:57 PST 
Currently just a work-in-progress.
Comment 3 Adam Barth 2012-02-27 00:05:56 PST 
Created attachment 128973 [details]
work in progress
Comment 4 Adam Barth 2012-02-27 00:15:12 PST 
Created attachment 128975 [details]
Patch
Comment 5 Hajime Morrita 2012-02-27 01:28:49 PST 
Comment on attachment 128975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128975&action=review

> Source/WebCore/history/PageCache.cpp:124
> +        if (DatabaseContext::from(frame->document())->hasOpenDatabases()) {

It looks this from() call makes the lazy instantiation making less sense.
How about to hide hide this behind a static method?

> Source/WebCore/history/PageCache.cpp:267
> +        && !DatabaseContext::from(document)->hasOpenDatabases()

Ditto.

> Source/WebCore/loader/FrameLoader.cpp:423
> +        DatabaseContext::from(doc)->stopDatabases(0);

Ditto on from().

> Source/WebCore/storage/DatabaseContext.cpp:78
> +    ASSERT(isContextThread());

Windows bot is right. We don't have isContextThread() on DatabaseContext.
Comment 6 Hajime Morrita 2012-02-27 01:30:48 PST 
BTW I'm totally unware that ScriptExceptionContext has such a ifdef-ed crap.
Supplementable effectively helps ;-)
Comment 7 Adam Barth 2012-02-27 11:00:53 PST 
Great points.  I'll update the patch.  Thanks!
Comment 8 Adam Barth 2012-02-27 14:58:33 PST 
Created attachment 129103 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-02-27 15:09:53 PST 
Comment on attachment 129103 [details]
Patch

LGTM.
Comment 10 Adam Barth 2012-02-29 17:22:34 PST 
Created attachment 129558 [details]
Patch for landing
Comment 11 Adam Barth 2012-02-29 17:25:05 PST 
Created attachment 129560 [details]
Patch for landing
Comment 12 Adam Barth 2012-02-29 17:45:45 PST 
Created attachment 129565 [details]
Patch for landing
Comment 13 Adam Barth 2012-02-29 17:51:48 PST 
Created attachment 129566 [details]
Patch for landing
Comment 14 Adam Barth 2012-02-29 18:44:10 PST 
Created attachment 129610 [details]
Patch for landing
Comment 15 Adam Barth 2012-02-29 23:46:36 PST 
Committed r109319: <http://trac.webkit.org/changeset/109319>