Bug 56425

Created attachment 85882 [details] Patch v1

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.

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

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.

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.

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

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

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

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

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

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.

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

Landed Patch #2 in r81484

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

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

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

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

> 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

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

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.

So much style embarrassment - new patch on its way.

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.

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

I'm working on this new direction right now.

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

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

<rdar://problem/8648311>

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.

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

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

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

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

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.