Bug 35763 - Remove the now-redundant Settings fields for the Database
Summary: Remove the now-redundant Settings fields for the Database
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Minor
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-04 13:22 PST by Eric U.
Modified: 2010-03-10 14:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.73 KB, patch)
2010-03-04 13:30 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2010-03-04 15:58 PST, Eric U.
no flags Details | Formatted Diff | Diff
Fix the build fix. (1.58 KB, patch)
2010-03-08 16:34 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (1.66 KB, patch)
2010-03-09 13:38 PST, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-03-04 13:22:12 PST
Bug 35310 finished adding the new interface; I then removed the last use of the old interface from Chromium.  It's now save to remove the Database fields from Settings and WebSettings.
Comment 1 Eric U. 2010-03-04 13:30:20 PST
Created attachment 50049 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-03-04 13:56:04 PST
Comment on attachment 50049 [details]
Patch

Patch fails to apply:

Failed to run "['/mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply', '--force']" exit_code: 1
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/WebCore.base.exp
patching file WebCore/page/Settings.cpp
Hunk #1 succeeded at 73 (offset 1 line).
Hunk #2 FAILED at 261.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/page/Settings.cpp.rej

Ok.  Why do we have a separate way of managing database settings?
Comment 3 Eric U. 2010-03-04 14:05:37 PST
I'll update to fix the patch; my code was a few days old.

The Settings object isn't available to Workers.  In order to get the Database accessible to Workers, the enable bit has to live somewhere that they can get at it.  WebSockets implemented their enable bit this way [as a static class member], so I followed that model.
Comment 4 Eric U. 2010-03-04 15:58:16 PST
Created attachment 50060 [details]
Patch
Comment 5 WebKit Commit Bot 2010-03-08 08:42:04 PST
Comment on attachment 50060 [details]
Patch

Clearing flags on attachment: 50060

Committed r55666: <http://trac.webkit.org/changeset/55666>
Comment 6 WebKit Commit Bot 2010-03-08 08:42:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Daniel Bates 2010-03-08 09:07:24 PST
This change broke the Windows bots. Looks like the corresponding Windows changes were not landed.
Comment 8 Daniel Bates 2010-03-08 09:33:31 PST
Applied corresponding change to file WebView.cpp to fix the Windows build.

Committed fix in r55667 <http://trac.webkit.org/changeset/55667>.
Comment 9 Eric U. 2010-03-08 16:34:45 PST
Created attachment 50265 [details]
Fix the build fix.

That build fix patch fixed the compile, but failed to restore the path by which the database enable preference can actually enable the database API.  This puts the right call in.
Comment 10 Eric U. 2010-03-08 16:35:23 PST
Reopening for the new fix patch.
Comment 11 David Levin 2010-03-08 21:16:31 PST
Comment on attachment 50265 [details]
Fix the build fix.

> Index: WebKit/win/ChangeLog
> +2010-03-08  Eric Uhrhane  <ericu@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The build fix for my patch on bug #35763 wasn't quite right--it removed
> +        the call entirely, instead of replacing it with the new API.  This adds
> +        the call to Database::setIsAvailable.

Please add a bug link here.

> Index: WebKit/win/WebView.cpp
> +    hr = prefsPrivate->databasesEnabled(&enabled);
> +    if (FAILED(hr))
> +        return hr;
> +    Database::setIsAvailable(enabled);
> +

This needs an appropriate if ENABLE guard.
Comment 12 Eric U. 2010-03-09 13:38:46 PST
Created attachment 50342 [details]
Patch
Comment 13 Eric U. 2010-03-09 13:40:11 PST
(In reply to comment #11)
> (From update of attachment 50265 [details])
> > Index: WebKit/win/ChangeLog
> > +2010-03-08  Eric Uhrhane  <ericu@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The build fix for my patch on bug #35763 wasn't quite right--it removed
> > +        the call entirely, instead of replacing it with the new API.  This adds
> > +        the call to Database::setIsAvailable.
> 
> Please add a bug link here.

Done.

> > Index: WebKit/win/WebView.cpp
> > +    hr = prefsPrivate->databasesEnabled(&enabled);
> > +    if (FAILED(hr))
> > +        return hr;
> > +    Database::setIsAvailable(enabled);
> > +
> 
> This needs an appropriate if ENABLE guard.

Done, and I now have a post-it on my Mac's monitor reminding me to check for that every time.
Comment 14 WebKit Commit Bot 2010-03-10 14:44:08 PST
Comment on attachment 50342 [details]
Patch

Clearing flags on attachment: 50342

Committed r55809: <http://trac.webkit.org/changeset/55809>
Comment 15 WebKit Commit Bot 2010-03-10 14:44:14 PST
All reviewed patches have been landed.  Closing bug.