Bug 35310 – Move database enable bit fully out of settings

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

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.

[Eric U.]

Sorry--bad title first time around.

[Eric U.]

Created attachment 49336 [details]
Patch

[WebKit Review Bot]

Attachment 49336 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/303130

[WebKit Review Bot]

Attachment 49336 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/303134

[Eric U.]

Created attachment 49349 [details]
Patch

[WebKit Review Bot]

Attachment 49349 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/304011

[Eric U.]

Created attachment 49405 [details]
Patch

[David Levin]

Comment on attachment 49405 [details]
Patch

> Index: WebCore/ChangeLog
> [phone]  Eric Uhrhane  <[e-mail]>
> +
> +        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
> [phone]  Eric Uhrhane  <[e-mail]>
> +
> +        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:]):
> +
> [phone]  Eric Uhrhane  <[e-mail]>
> +
> +        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?

[Eric U.]

(In reply to comment #8)
> (From update of attachment 49405 [details])
> > Index: WebCore/ChangeLog
> > [phone]  Eric Uhrhane  <[e-mail]>
> > +
> > +        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
> > [phone]  Eric Uhrhane  <[e-mail]>
> > +
> > +        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:]):
> > +
> > [phone]  Eric Uhrhane  <[e-mail]>
> > +
> > +        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?

[Darin Fisher (:fishd, Google)]

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 :)

[Darin Fisher (:fishd, Google)]

(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.

[Eric U.]

(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.

[Eric U.]

(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.

[Eric U.]

Created attachment 49840 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 49840 [details]
Patch

Clearing flags on attachment: 49840

Committed r55452: <http://trac.webkit.org/changeset/55452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

(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.

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 49931 [details]
buildfix

Not sure where to put the guard, so I put it up for review.

[Eric U.]

Very sorry about the breakage--yes Laszlo, that's the right place.  I'll have a GTK fix up shortly.

[Eric U.]

Nevermind--philn cleaned up my GTK mess already.

[WebKit Commit Bot]

Comment on attachment 49931 [details]
buildfix

Clearing flags on attachment: 49931

Committed r55504: <http://trac.webkit.org/changeset/55504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.