Bug 35310

Sorry--bad title first time around.

Created attachment 49336 [details] Patch

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

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

Created attachment 49349 [details] Patch

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

Created attachment 49405 [details] Patch

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?

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

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

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

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

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

Created attachment 49840 [details] Patch

Comment on attachment 49840 [details] Patch Clearing flags on attachment: 49840 Committed r55452: <http://trac.webkit.org/changeset/55452>

All reviewed patches have been landed. Closing bug.

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

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.

Created attachment 49931 [details] buildfix Not sure where to put the guard, so I put it up for review.

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

Nevermind--philn cleaned up my GTK mess already.

Comment on attachment 49931 [details] buildfix Clearing flags on attachment: 49931 Committed r55504: <http://trac.webkit.org/changeset/55504>

All reviewed patches have been landed. Closing bug.