Bug 56425 – WebKit2 Icon Database

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

WebKit2 needs an Icon Database

Our plan now is to host the icon DB in the UI process and have the web process talk to it through messages.

This will allow multiple WebProcesses to easily get at the same database.

I'm planning on landing work in stages - first up will be a patch that adds the new files and lays the messaging groundwork between Web <--> UI processes, but is basically a no-op IconDatabaseBase implementation.

[Brady Eidson]

Created attachment 85882 [details]
Patch v1

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/WebIconDatabase.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebIconDatabase.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/IconDatabase/WebIconDatabaseProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebProcessProxy.cpp:294:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 4 in 32 files


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

[Brady Eidson]

I've fixed (some of) the style bot errors, but will not reload the patch at this time - they were minor, and I'd rather give EWS a full chance to run.

[Sam Weinig]

Comment on attachment 85882 [details]
Patch v1

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

> Source/WebKit2/WebProcess/IconDatabase/WebIconDatabaseProxy.cpp:109
> +String WebIconDatabaseProxy::iconURLForPageURL(const String& pageURL)
> +{
> +    String result;
> +    if (!m_process->connection()->sendSync(Messages::WebIconDatabase::IconURLForPageURL(pageURL), Messages::WebIconDatabase::IconURLForPageURL::Reply(result), 0))
> +        return String();
> +    
> +    return result;
> +}
> +
> +bool WebIconDatabaseProxy::iconDataKnownForIconURL(const String& iconURL)
> +{
> +    bool result;
> +    if (!m_process->connection()->sendSync(Messages::WebIconDatabase::IconDataKnownForIconURL(iconURL), Messages::WebIconDatabase::IconDataKnownForIconURL::Reply(result), 0))
> +        return false;
> +    
> +    return result;
> +}
> +
> +IconLoadDecision WebIconDatabaseProxy::loadDecisionForIconURL(const String& iconURL, DocumentLoader* documentLoader)
> +{
> +    IconLoadDecision result;
> +    if (!m_process->connection()->sendSync(Messages::WebIconDatabase::LoadDecisionForIconURL(iconURL), Messages::WebIconDatabase::LoadDecisionForIconURL::Reply((int&)result), 0))
> +        return IconLoadNo;
> +    
> +    return result;
> +}

These should not be sync messages.  Please don't land this part.

[Brady Eidson]

The Qt bot being unable to apply the .vcproj part of the patch seems bizarre and broken...

I landed this in r81208 and will watch the bots.

[Brady Eidson]

Visual studio is apparently a lot pickier than the rest of our platforms, because it got confused by a forward declaration I don't think it should have...  trying to fix the windows build.

[Brady Eidson]

Attempted VS appeasement in r81211

I'll check in again in a few hours (traveling for a bit) to make sure it stuck.

[Build Bot]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/81208 might have broken GTK Linux 32-bit Release

[Brady Eidson]

(In reply to comment #9)
> http://trac.webkit.org/changeset/81208 might have broken GTK Linux 32-bit Release

Maybe... but whether or not it did, that build is fine right now,

[Brady Eidson]

Created attachment 86128 [details]
Patch #2, v1 - Change IconDatabaseBase and FrameLoader icon logic to support an asynchronous mode.

[WebKit Review Bot]

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

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

Source/WebCore/loader/icon/IconDatabase.cpp:289:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/loader/icon/IconDatabaseBase.h:49:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/loader/icon/IconDatabaseBase.h:88:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/loader/icon/IconDatabaseBase.h:156:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/loader/icon/IconDatabaseBase.h:157:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/loader/FrameLoader.cpp:760:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 6 in 28 files


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

[Brady Eidson]

(In reply to comment #12)
> Attachment 86128 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/loader/icon/IconDatabase.cpp:289:  Missing space after ,  [whitespace/comma] [3]
> Source/WebCore/loader/icon/IconDatabaseBase.h:49:  Missing space before {  [whitespace/braces] [5]
> Source/WebCore/loader/icon/IconDatabaseBase.h:88:  Missing space before {  [whitespace/braces] [5]
> Source/WebCore/loader/icon/IconDatabaseBase.h:156:  Missing space before {  [whitespace/braces] [5]
> Source/WebCore/loader/icon/IconDatabaseBase.h:157:  Missing space before {  [whitespace/braces] [5]
> Source/WebCore/loader/FrameLoader.cpp:760:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
> Total errors found: 6 in 28 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed all of these locally.  Will let EWS bots all have their swipe at this then land in the A.M.

[Brady Eidson]

Landed Patch #2 in r81484

[WebKit Review Bot]

http://trac.webkit.org/changeset/81484 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
webarchive/test-link-rel-icon.html

[Brady Eidson]

(In reply to comment #15)
> http://trac.webkit.org/changeset/81484 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> webarchive/test-link-rel-icon.html

Working on this right now.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #15)
> > http://trac.webkit.org/changeset/81484 might have broken SnowLeopard Intel Release (Tests)
> > The following tests are not passing:
> > webarchive/test-link-rel-icon.html
> 
> Working on this right now.

I cannot reproduce this failure locally, either using run-webkit-tests or manually running the test in Safari.

[Brady Eidson]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > http://trac.webkit.org/changeset/81484 might have broken SnowLeopard Intel Release (Tests)
> > > The following tests are not passing:
> > > webarchive/test-link-rel-icon.html
> > 
> > Working on this right now.
> 
> I cannot reproduce this failure locally, either using run-webkit-tests or manually running the test in Safari.

This failed once on Leopard, too, after passing a few times:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r81509%20(29954)/results.html

I can't reproduce this failure locally on SnowLeopard, or any other OS.

I can log in to the Leopard bot that showed the failure once and I can *not* reproduce it there.

I can log in to the SnowLeopard bot that shows the failure and *can* reproduce it there.

It appears this test is now "flakey" - I'm filing a new bug and adding it to the skipped list until cycles can be spared to get to the bottom of it.

[Brady Eidson]

> It appears this test is now "flakey" - I'm filing a new bug and adding it to the skipped list until cycles can be spared to get to the bottom of it.

https://bugs.webkit.org/show_bug.cgi?id=56685

[Brady Eidson]

Created attachment 86475 [details]
Patch #3, v1 - Hookup an actual threaded icon database that manages I/O.  A direct port from WebCore/loader/icon/IconDatabase.cpp

The meatiest part of this patch is WebIconDatabaseImpl which is a direct copy of WebCore/loader/icon/IconDatabase.cpp

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/WebIconDatabaseImpl.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.h:68:  Missing space before {  [whitespace/braces] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:16:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:62:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:445:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:505:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:554:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:611:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:684:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:718:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:902:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:949:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:986:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1195:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1320:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1405:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1525:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebIconDatabaseImpl.cpp:1526:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit2/UIProcess/WebContext.h:159:  The parameter name "path" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 21 in 25 files


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

[Brady Eidson]

So much style embarrassment - new patch on its way.

[Maciej Stachowiak]

Comment on attachment 86475 [details]
Patch #3, v1 - Hookup an actual threaded icon database that manages I/O.  A direct port from WebCore/loader/icon/IconDatabase.cpp

I'm concerned that all this code is being moved from WebCore to WebKit2. That seems like a bad overall direction. We don't want to have totally separate implementations for WebKit1 and WebKit2 IMO. Is there any way the code could live in WebCore, in a way that it could be used directly by WebKit1, and used via a proxy and then called from the UI process in WebKit2? I think that would be much better than forking the implementation.

[Brady Eidson]

(In reply to comment #23)
> (From update of attachment 86475 [details])
> I'm concerned that all this code is being moved from WebCore to WebKit2. That seems like a bad overall direction. We don't want to have totally separate implementations for WebKit1 and WebKit2 IMO. Is there any way the code could live in WebCore, in a way that it could be used directly by WebKit1, and used via a proxy and then called from the UI process in WebKit2? I think that would be much better than forking the implementation.

We've reached the design decision that the actual IconDatabase needs to be hosted in the UIProcess, so part of what you suggest isn't the direction we want to go.

But your suggestion of refactoring the one copy of the code in WebCore to be used by both WebCore and the WebKit2 UIProcess is quite feasible.  I guess it just hadn't occurred to anyone.

The WebCore icon database code could be refactored to support this.  It would run in "the process" for WK1 clients, and would run in the UI process as the WK2 icon database for WK2 apps.

This would be a gear shift that will put this out another day or so, but is probably worth it.

[Brady Eidson]

I'm working on this new direction right now.

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

<rdar://problem/8648311>

[Brady Eidson]

With the other related bugs here, and other bugs I might've forgotten to relate, the WK2 icon database is in place.

A patch is coming any moment to expose the most basic API, then I'll add-on as needed using other bugs.

[Brady Eidson]

Created attachment 86991 [details]
Patch - Add minimal WK2 API

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/C/cg/WKIconDatabaseCG.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Sam Weinig]

Comment on attachment 86991 [details]
Patch - Add minimal WK2 API

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

> Source/WebKit2/UIProcess/API/C/WKIconDatabase.h:38
> +WK_EXPORT void WKIconDatabaseRetainIconForURL(WKIconDatabaseRef iconDatabase, WKStringRef pageURL);
> +WK_EXPORT void WKIconDatabaseReleaseIconForURL(WKIconDatabaseRef iconDatabase, WKStringRef pageURL);

Why don't these take WKURLRefs?

> Source/WebKit2/UIProcess/API/C/cg/WKIconDatabaseCG.cpp:39
> +    WebCore::Image* image = toImpl(iconDatabaseRef)->imageForPageURL(toWTFString(urlStringRef));

WebCore:: not necessary here.

> Source/WebKit2/UIProcess/API/C/cg/WKIconDatabaseCG.h:36
> +WK_EXPORT CGImageRef WKIconDatabaseGetCGImageForURL(WKIconDatabaseRef iconDatabase, WKStringRef urlString);

Why does this not take a WKURLRef?

[Brady Eidson]

Created attachment 86996 [details]
Patch v2 - Fix Qt also

[Brady Eidson]

Ignore the "Fix Qt also" - it's in my autocomplete and that's too fast for me.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/C/cg/WKIconDatabaseCG.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Brady Eidson]

r82005