Bug 56216 – Rework IconDatabase disabling while allowing for a "pluggable" icon database

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Rework IconDatabase disabling while allowing for a "pluggable" icon database.

For WebKit2, we'll want to plug-in a different IconDatabase for both WebCore and WK2 to use.
For WebKit1 mac/win, we want the current WebCore IconDatabase to keep working as it does now.
And for ports who disable the IconDatabase, we want that to keep working.

By making a virtual base "IconDatabaseBase" class for anyone to inherit from, we can accomplish all of the above:

-The "DISABLE(IconDatabase)" folks will just get IconDatabaseBase as a no-op implementation.
-The current WebCore IconDatabase users will get the current IconDatabase code, now inheriting from IconDatabaseBase
-WK2 can make its own IconDatabaseBase subclass and assign it as the current IconDatabase. That will be in a different bug.

Patch for the above refactoring is on its way.

[Brady Eidson]

Created attachment 85514 [details]
Patch v1

[Brady Eidson]

In radar - <rdar://problem/8648311>

[Sam Weinig]

Comment on attachment 85514 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=85514&action=review

> Source/WebCore/loader/icon/IconDatabaseBase.cpp:58
> +} //namespace WebCore

Missing space.

> Source/WebCore/loader/icon/IconDatabaseBase.h:87
> +}; // class IconDatabaseBase

We don't put these // class comments usually.

[Brady Eidson]

Created attachment 85546 [details]
Patch v2 - Left out the WebKit pieces

[WebKit Review Bot]

Attachment 85546 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Android.mk', u'Source/WebCo..." exit_code: 1

Source/WebCore/loader/icon/IconDatabase.h:61:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/loader/icon/IconDatabase.h:93:  The parameter name "enabled" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Landed in r80900

[Build Bot]

Attachment 85546 [details] did not build on win:
Build output: http://queues.webkit.org/results/8149477

[Brady Eidson]

Windows build fix in 80902

[WebKit Review Bot]

http://trac.webkit.org/changeset/80900 might have broken Qt Linux Release minimal, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug

[Brady Eidson]

Hopefully fixed Qt in r80905

[Ryosuke Niwa]

Bots are still broken.

http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/13151/steps/compile-webkit/logs/stdio
http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/20433/steps/compile-webkit/logs/stdio

[Brady Eidson]

Reasonably confident fix for Windows in r80908

Complete shot in the dark fix for Qt in r80909

[Ryosuke Niwa]

(In reply to comment #12)
> Reasonably confident fix for Windows in r80908
> 
> Complete shot in the dark fix for Qt in r80909

Still broken :(  At this point, I'd really like you to consider rolling it out for now and re-land it later after verifying that your patch builds on all platforms.

[Brady Eidson]

(In reply to comment #13)
> (In reply to comment #12)
> > Reasonably confident fix for Windows in r80908
> > 
> > Complete shot in the dark fix for Qt in r80909
> 
> Still broken :(  At this point, I'd really like you to consider rolling it out for now and re-land it later after verifying that your patch builds on all platforms.

Since I'm here and actively trying to fix this, perhaps it would help us both if you could help track down what Qt is complaining about?

[Brady Eidson]

Ah figured out why my last attempt didn't take.  Let's go again...

[Ryosuke Niwa]

(In reply to comment #14)
> Since I'm here and actively trying to fix this, perhaps it would help us both if you could help track down what Qt is complaining about?

It seems like Qt's WebSettings access it:

../../../../Source/WebCore/loader/icon/IconDatabase.h: In static member function 'static void QWebSettings::setIconDatabasePath(const QString&)':
../../../../Source/WebCore/loader/icon/IconDatabase.h:63:17: error: 'static void WebCore::IconDatabase::delayDatabaseCleanup()' is private
../../../../Source/WebKit/qt/Api/qwebsettings.cpp:648:49: error: within this context

[Brady Eidson]

r80910

[Ryosuke Niwa]

It seems like the current Windows build failure is due to http://trac.webkit.org/changeset/80899.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #14)
> > Since I'm here and actively trying to fix this, perhaps it would help us both if you could help track down what Qt is complaining about?
> 
> It seems like Qt's WebSettings access it:
> 
> ../../../../Source/WebCore/loader/icon/IconDatabase.h: In static member function 'static void QWebSettings::setIconDatabasePath(const QString&)':
> ../../../../Source/WebCore/loader/icon/IconDatabase.h:63:17: error: 'static void WebCore::IconDatabase::delayDatabaseCleanup()' is private
> ../../../../Source/WebKit/qt/Api/qwebsettings.cpp:648:49: error: within this context

Yah, I added that in my 2nd to last build fix, but forgot to make it public.  I did in my last build fix.

I was pretty sure this whole time - until landing the initial patch - that Qt didn't use the icon database.

Therefore I was surprised they provided any form of support for it in its API layer.

Sorry for all the troubles - I'm going to keep an eye on it and make sure this one takes.

[Ryosuke Niwa]

(In reply to comment #19)
> Yah, I added that in my 2nd to last build fix, but forgot to make it public.  I did in my last build fix.
> I was pretty sure this whole time - until landing the initial patch - that Qt didn't use the icon database.
> Therefore I was surprised they provided any form of support for it in its API layer.
> Sorry for all the troubles - I'm going to keep an eye on it and make sure this one takes.

No problem!  Thanks for the timely build fixes.