Bug 56425 - WebKit2 Icon Database
Summary: WebKit2 Icon Database
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar, LayoutTestFailure
Depends on: 56876 57058 57069
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-15 16:38 PDT by Brady Eidson
Modified: 2011-03-25 17:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (67.33 KB, patch)
2011-03-15 17:13 PDT, Brady Eidson
sam: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Patch #2, v1 - Change IconDatabaseBase and FrameLoader icon logic to support an asynchronous mode. (43.86 KB, patch)
2011-03-17 18:15 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff
Patch #3, v1 - Hookup an actual threaded icon database that manages I/O. A direct port from WebCore/loader/icon/IconDatabase.cpp (122.54 KB, patch)
2011-03-22 10:36 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch - Add minimal WK2 API (deleted)
2011-03-25 16:09 PDT, Brady Eidson
sam: review-
Details | Formatted Diff | Diff
Patch v2 - Fix Qt also (16.17 KB, patch)
2011-03-25 17:02 PDT, Brady Eidson
sam: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2011-03-15 16:38:42 PDT
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.
Comment 1 Brady Eidson 2011-03-15 17:13:10 PDT
Created attachment 85882 [details]
Patch v1
Comment 2 WebKit Review Bot 2011-03-15 17:14:56 PDT
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.
Comment 3 Brady Eidson 2011-03-15 17:20:22 PDT
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.
Comment 4 Sam Weinig 2011-03-15 18:31:30 PDT
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.
Comment 5 Brady Eidson 2011-03-15 19:16:45 PDT
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.
Comment 6 Brady Eidson 2011-03-15 19:39:03 PDT
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.
Comment 7 Brady Eidson 2011-03-15 19:50:55 PDT
Attempted VS appeasement in r81211

I'll check in again in a few hours (traveling for a bit) to make sure it stuck.
Comment 8 Build Bot 2011-03-15 19:59:32 PDT
Attachment 85882 [details] did not build on win:
Build output: http://queues.webkit.org/results/8178839
Comment 9 WebKit Review Bot 2011-03-16 09:58:07 PDT
http://trac.webkit.org/changeset/81208 might have broken GTK Linux 32-bit Release
Comment 10 Brady Eidson 2011-03-16 10:11:26 PDT
(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,
Comment 11 Brady Eidson 2011-03-17 18:15:34 PDT
Created attachment 86128 [details]
Patch #2, v1 - Change IconDatabaseBase and FrameLoader icon logic to support an asynchronous mode.
Comment 12 WebKit Review Bot 2011-03-17 18:18:32 PDT
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.
Comment 13 Brady Eidson 2011-03-17 18:49:04 PDT
(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.
Comment 14 Brady Eidson 2011-03-18 10:07:58 PDT
Landed Patch #2 in r81484
Comment 15 WebKit Review Bot 2011-03-18 10:50:43 PDT
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
Comment 16 Brady Eidson 2011-03-18 11:02:58 PDT
(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.
Comment 17 Brady Eidson 2011-03-18 12:15:44 PDT
(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.
Comment 18 Brady Eidson 2011-03-18 14:45:58 PDT
(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.
Comment 19 Brady Eidson 2011-03-18 14:50:38 PDT
> 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
Comment 20 Brady Eidson 2011-03-22 10:36:44 PDT
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
Comment 21 WebKit Review Bot 2011-03-22 10:40:52 PDT
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.
Comment 22 Brady Eidson 2011-03-22 11:16:08 PDT
So much style embarrassment - new patch on its way.
Comment 23 Maciej Stachowiak 2011-03-22 11:19:18 PDT
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.
Comment 24 Brady Eidson 2011-03-22 11:30:00 PDT
(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.
Comment 25 Brady Eidson 2011-03-22 11:45:52 PDT
I'm working on this new direction right now.
Comment 26 Build Bot 2011-03-22 11:49:11 PDT
Attachment 86475 [details] did not build on win:
Build output: http://queues.webkit.org/results/8222403
Comment 27 Build Bot 2011-03-22 12:10:21 PDT
Attachment 86475 [details] did not build on win:
Build output: http://queues.webkit.org/results/8218918
Comment 28 Brady Eidson 2011-03-25 15:50:07 PDT
<rdar://problem/8648311>
Comment 29 Brady Eidson 2011-03-25 15:54:33 PDT
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.
Comment 30 Brady Eidson 2011-03-25 16:09:21 PDT
Created attachment 86991 [details]
Patch - Add minimal WK2 API
Comment 31 WebKit Review Bot 2011-03-25 16:11:09 PDT
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.
Comment 32 Sam Weinig 2011-03-25 16:52:53 PDT
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?
Comment 33 Brady Eidson 2011-03-25 17:02:33 PDT
Created attachment 86996 [details]
Patch v2 - Fix Qt also
Comment 34 Brady Eidson 2011-03-25 17:02:57 PDT
Ignore the "Fix Qt also" - it's in my autocomplete and that's too fast for me.
Comment 35 WebKit Review Bot 2011-03-25 17:05:09 PDT
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.
Comment 36 Brady Eidson 2011-03-25 17:09:28 PDT
r82005