[webkit-reviews] review denied: [Bug 35310] Move database enable bit fully out of settings : [Attachment 49405] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 35310: Move database enable bit fully out of settings
https://bugs.webkit.org/show_bug.cgi?id=35310

Attachment 49405: Patch
https://bugs.webkit.org/attachment.cgi?id=49405&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: WebCore/ChangeLog
> +2010-02-23  Eric Uhrhane  <ericu at 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 at 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 at 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?