Bug 35310 - Move database enable bit fully out of settings
Summary: Move database enable bit fully out of settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks: 34990
  Show dependency treegraph
 
Reported: 2010-02-23 11:57 PST by Eric U.
Modified: 2010-03-03 21:19 PST (History)
8 users (show)

See Also:


Attachments
Patch (21.20 KB, patch)
2010-02-23 16:10 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (25.43 KB, patch)
2010-02-23 17:47 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (25.44 KB, patch)
2010-02-24 10:06 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (23.40 KB, patch)
2010-03-02 13:44 PST, Eric U.
no flags Details | Formatted Diff | Diff
buildfix (1.02 KB, patch)
2010-03-03 12:15 PST, Laszlo Gombos
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-02-23 11:57:47 PST
My patch for bug 22725 began the move of the database enable bit out of the settings and into the Database class.  I need to finish the job in order to get the database working in workers.  They can't access the settings directly, and this method of getting them the enable bit matches what was done for WebSockets.

See bug 34990 for the larger context.
Comment 1 Eric U. 2010-02-23 11:58:22 PST
Sorry--bad title first time around.
Comment 2 Eric U. 2010-02-23 16:10:00 PST
Created attachment 49336 [details]
Patch
Comment 3 WebKit Review Bot 2010-02-23 17:15:51 PST
Attachment 49336 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/303130
Comment 4 WebKit Review Bot 2010-02-23 17:18:35 PST
Attachment 49336 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/303134
Comment 5 Eric U. 2010-02-23 17:47:25 PST
Created attachment 49349 [details]
Patch
Comment 6 WebKit Review Bot 2010-02-23 18:43:17 PST
Attachment 49349 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/304011
Comment 7 Eric U. 2010-02-24 10:06:03 PST
Created attachment 49405 [details]
Patch
Comment 8 David Levin 2010-02-24 10:26:17 PST
Comment on attachment 49405 [details]
Patch

> Index: WebCore/ChangeLog
> +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Move database enable bit fully out of settings
> +        https://bugs.webkit.org/show_bug.cgi?id=35310
> +
> +        It now goes from preferences straight into Database::isAvailable, as
> +        with WebSockets.
> +
> +        No new tests.

This should explain why there are no new test.
Is it because it is fixing a test or is it because no new functionality is exposed.

> +
> +        * WebCore.base.exp:
> +        * WebCore.xcodeproj/project.pbxproj:

It would be good to comment about why a change was done in a particular file in this part of the ChangeLog.

For example, the project.pbxproj had a lot of header files get "settings = {ATTRIBUTES = (Private, ); };"  (I think I know why but) it would be good to comment about why this was done.

> +        * page/DOMWindow.cpp:
> +        (WebCore::DOMWindow::openDatabase):
> +        * page/Settings.cpp:
> +        (WebCore::Settings::Settings):
> +        * page/Settings.h:
> +


> Index: WebKit/chromium/public/WebSettings.h
> -    virtual void setDatabasesEnabled(bool) = 0;

Darin Fisher should give this part a glance because I don't know the rules regarding changing the public interface (but I thought that we tried not to do this).

Actually, I don't understand why this is being removed (a changelog comment may have helped about this file), but it seems that this method could have just called Database::setIsAvailable() now and remained around.


> Index: WebKit/gtk/webkit/webkitwebview.cpp
> +    Database::setIsAvailable(enableHTML5Database);


Shouldn't this have "#if ENABLE(DATABASE)" around it?

>      else if (name == g_intern_string("enable-html5-database"))
> +        Database::setIsAvailable(g_value_get_boolean(&value));

Shouldn't this have "#if ENABLE(DATABASE)" around it?



> Index: WebKit/mac/ChangeLog
> +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Move database enable bit fully out of settings
> +        https://bugs.webkit.org/show_bug.cgi?id=35310
> +
> +        It now goes from preferences straight into Database::isAvailable, as
> +        with WebSockets.
> +
> +        * WebView/WebView.mm:
> +        (-[WebView _preferencesChangedNotification:]):
> +
> +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Move database enable bit fully out of settings
> +        https://bugs.webkit.org/show_bug.cgi?id=35310
> +
> +        It now goes from preferences straight into Database::isAvailable, as
> +        with WebSockets.
> +
> +        * WebView/WebView.mm:
> +        (-[WebView _preferencesChangedNotification:]):
> +

Echo :)


> Index: WebKit/qt/Api/qwebsettings.cpp
> +        WebCore::Database::setIsAvailable(value);

Shouldn't this have "#if ENABLE(DATABASE)" around it?
Comment 9 Eric U. 2010-02-24 11:20:04 PST
(In reply to comment #8)
> (From update of attachment 49405 [details])
> > Index: WebCore/ChangeLog
> > +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Move database enable bit fully out of settings
> > +        https://bugs.webkit.org/show_bug.cgi?id=35310
> > +
> > +        It now goes from preferences straight into Database::isAvailable, as
> > +        with WebSockets.
> > +
> > +        No new tests.
> 
> This should explain why there are no new test.
> Is it because it is fixing a test or is it because no new functionality is
> exposed.
> 
> > +
> > +        * WebCore.base.exp:
> > +        * WebCore.xcodeproj/project.pbxproj:
> 
> It would be good to comment about why a change was done in a particular file in
> this part of the ChangeLog.
> 
> For example, the project.pbxproj had a lot of header files get "settings =
> {ATTRIBUTES = (Private, ); };"  (I think I know why but) it would be good to
> comment about why this was done.
> 
> > +        * page/DOMWindow.cpp:
> > +        (WebCore::DOMWindow::openDatabase):
> > +        * page/Settings.cpp:
> > +        (WebCore::Settings::Settings):
> > +        * page/Settings.h:
> > +
> 
> 
> > Index: WebKit/chromium/public/WebSettings.h
> > -    virtual void setDatabasesEnabled(bool) = 0;
> 
> Darin Fisher should give this part a glance because I don't know the rules
> regarding changing the public interface (but I thought that we tried not to do
> this).
> 
> Actually, I don't understand why this is being removed (a changelog comment may
> have helped about this file), but it seems that this method could have just
> called Database::setIsAvailable() now and remained around.

I could do it that way.  It just seemed like we didn't need to have two ways of setting/getting that value, since there was only one underlying source of truth, and it wasn't going to be the Settings object any more.  I could make Settings call out to Database, but we'd still need to set the bit through the Database interface in Worker context, where you don't have access to the Settings.  I think the fewer ways you have to remember to set the bit, the better.  It's complicated enough tracking enables through WebRuntimeFeatures [in Chrome], WebPreferences, WebSettings [and its implementation], and WebCore::Settings.

However, if this is an interface that we really don't want to change, I can easily do it the other way.  Darin, can you comment?

> > Index: WebKit/gtk/webkit/webkitwebview.cpp
> > +    Database::setIsAvailable(enableHTML5Database);
> 
> 
> Shouldn't this have "#if ENABLE(DATABASE)" around it?
> 
> >      else if (name == g_intern_string("enable-html5-database"))
> > +        Database::setIsAvailable(g_value_get_boolean(&value));
> 
> Shouldn't this have "#if ENABLE(DATABASE)" around it?
> 
> 
> 
> > Index: WebKit/mac/ChangeLog
> > +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Move database enable bit fully out of settings
> > +        https://bugs.webkit.org/show_bug.cgi?id=35310
> > +
> > +        It now goes from preferences straight into Database::isAvailable, as
> > +        with WebSockets.
> > +
> > +        * WebView/WebView.mm:
> > +        (-[WebView _preferencesChangedNotification:]):
> > +
> > +2010-02-23  Eric Uhrhane  <ericu@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Move database enable bit fully out of settings
> > +        https://bugs.webkit.org/show_bug.cgi?id=35310
> > +
> > +        It now goes from preferences straight into Database::isAvailable, as
> > +        with WebSockets.
> > +
> > +        * WebView/WebView.mm:
> > +        (-[WebView _preferencesChangedNotification:]):
> > +
> 
> Echo :)
> 
> 
> > Index: WebKit/qt/Api/qwebsettings.cpp
> > +        WebCore::Database::setIsAvailable(value);
> 
> Shouldn't this have "#if ENABLE(DATABASE)" around it?
Comment 10 Darin Fisher (:fishd, Google) 2010-02-26 15:51:02 PST
It is OK to remove Chromium WebKit APIs, but please ensure that all Chromium references have been removed first.  Usually, the process for doing that involves 3 patches: 1) introduce a new API to WebKit, 2) switch Chromium over to the new API, and 3) remove the old API from WebKit.  In this case, I guess there is no step #1 :)
Comment 11 Darin Fisher (:fishd, Google) 2010-02-26 15:52:02 PST
(In reply to comment #10)
> It is OK to remove Chromium WebKit APIs, but please ensure that all Chromium
> references have been removed first.  Usually, the process for doing that
> involves 3 patches: 1) introduce a new API to WebKit, 2) switch Chromium over
> to the new API, and 3) remove the old API from WebKit.  In this case, I guess
> there is no step #1 :)

Also, it is common to mark a WebKit API "DEPRECATED" in step #1 by just adding a comment to that effect.
Comment 12 Eric U. 2010-02-26 16:42:24 PST
(In reply to comment #11)
> (In reply to comment #10)
> > It is OK to remove Chromium WebKit APIs, but please ensure that all Chromium
> > references have been removed first.  Usually, the process for doing that
> > involves 3 patches: 1) introduce a new API to WebKit, 2) switch Chromium over
> > to the new API, and 3) remove the old API from WebKit.  In this case, I guess
> > there is no step #1 :)
> 
> Also, it is common to mark a WebKit API "DEPRECATED" in step #1 by just adding
> a comment to that effect.

#1 is already done.
#2 is a one-liner; I'll send the review shortly, then upload the new version of this patch, addressing David's other points.  I actually thought I'd already removed all calls to the old API, but I just checked and found a file I've modified locally but not yet checked in.
Comment 13 Eric U. 2010-02-26 18:40:22 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > It is OK to remove Chromium WebKit APIs, but please ensure that all Chromium
> > > references have been removed first.  Usually, the process for doing that
> > > involves 3 patches: 1) introduce a new API to WebKit, 2) switch Chromium over
> > > to the new API, and 3) remove the old API from WebKit.  In this case, I guess
> > > there is no step #1 :)
> > 
> > Also, it is common to mark a WebKit API "DEPRECATED" in step #1 by just adding
> > a comment to that effect.
> 
> #1 is already done.
> #2 is a one-liner; I'll send the review shortly, then upload the new version of
> this patch, addressing David's other points.  I actually thought I'd already
> removed all calls to the old API, but I just checked and found a file I've
> modified locally but not yet checked in.

Arg.  It's a little more complicated than that--I do actually have to do all 3 steps, because DOMWindow.cpp is still checking the old location in the Settings, which Chrome won't be setting after the one-liner I was going to use.

I'll make a patch for that [step 1] and attach it to this bug for review, then do the Chromium change, then upload the polished version of the patch David's already reviewed.
Comment 14 Eric U. 2010-03-02 13:44:49 PST
Created attachment 49840 [details]
Patch
Comment 15 WebKit Commit Bot 2010-03-02 23:43:19 PST
Comment on attachment 49840 [details]
Patch

Clearing flags on attachment: 49840

Committed r55452: <http://trac.webkit.org/changeset/55452>
Comment 16 WebKit Commit Bot 2010-03-02 23:43:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Xan Lopez 2010-03-03 00:31:39 PST
(In reply to comment #16)
> All reviewed patches have been landed.  Closing bug.

This broke GTK+ builds. If EWS does not run it would be nice to at least stick around to check if the code builds in the normal bots.
Comment 18 Laszlo Gombos 2010-03-03 12:12:27 PST
This also broke certain QtWebKit builds (e.g. when DATABASE support disabled build time , or the "minimal build"). 

As David Levin noted during the review few thing should be still guarded with "if ENABLE(DATABASE)". Patch will follow soon.
Comment 19 Laszlo Gombos 2010-03-03 12:15:51 PST
Created attachment 49931 [details]
buildfix

Not sure where to put the guard, so I put it up for review.
Comment 20 Eric U. 2010-03-03 12:55:30 PST
Very sorry about the breakage--yes Laszlo, that's the right place.  I'll have a GTK fix up shortly.
Comment 21 Eric U. 2010-03-03 13:22:35 PST
Nevermind--philn cleaned up my GTK mess already.
Comment 22 WebKit Commit Bot 2010-03-03 21:19:34 PST
Comment on attachment 49931 [details]
buildfix

Clearing flags on attachment: 49931

Committed r55504: <http://trac.webkit.org/changeset/55504>
Comment 23 WebKit Commit Bot 2010-03-03 21:19:40 PST
All reviewed patches have been landed.  Closing bug.