Bug 49778 - Add USE(CHROMIUM_NET)
Summary: Add USE(CHROMIUM_NET)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-18 19:11 PST by William Chan
Modified: 2010-11-19 20:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.16 KB, patch)
2010-11-18 19:15 PST, William Chan
no flags Details | Formatted Diff | Diff
Patch (1.17 KB, patch)
2010-11-18 19:30 PST, William Chan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description William Chan 2010-11-18 19:11:56 PST
Add USE(CHROMIUM_NET)
Comment 1 William Chan 2010-11-18 19:15:06 PST
Created attachment 74340 [details]
Patch
Comment 2 William Chan 2010-11-18 19:21:55 PST
Er, webkit-patch doesn't seem to have an option to add a description, unless I confused it with the title.  Sorry if I missed something.

I plan to use USE(CHROMIUM_NET) to unthrottle some preloading, like so:

diff --git a/WebCore/loader/cache/CachedResourceLoader.cpp b/WebCore/loader/cache/CachedResourceLoader.cpp
index 15c5f03..0e2a393 100644
--- a/WebCore/loader/cache/CachedResourceLoader.cpp
+++ b/WebCore/loader/cache/CachedResourceLoader.cpp
@@ -377,6 +377,9 @@ int CachedResourceLoader::requestCount()
     
 void CachedResourceLoader::preload(CachedResource::Type type, const String& url, const String& charset, bool referencedFromBody)
 {
+    // CHROMIUM_NET will prioritize network requests appropriately, so always
+    // request a preload.
+#if !USE(CHROMIUM_NET)
     bool hasRendering = m_document->body() && m_document->body()->renderer();
     if (!hasRendering && (referencedFromBody || type == CachedResource::ImageResource)) {
         // Don't preload images or body resources before we have something to draw. This prevents
@@ -385,6 +388,8 @@ void CachedResourceLoader::preload(CachedResource::Type type, const String& url,
         m_pendingPreloads.append(pendingPreload);
         return;
     }
+#endif  // !USE(CHROMIUM_NET)
+
     requestPreload(type, url, charset);
 }

Let me know if I should add this change into the proposed patch.  I wasn't sure if they should be separate or not.
Comment 3 WebKit Review Bot 2010-11-18 19:26:54 PST
Attachment 74340 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/Platform.h']" exit_code: 1
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 William Chan 2010-11-18 19:30:56 PST
Created attachment 74344 [details]
Patch
Comment 5 David Levin 2010-11-19 09:14:24 PST
Why do we need another flag as opposed to just using PLATFORM(CHROMIUM)? It feels like another flag is only needed if you would choose CHROMIUM_NET independently from PLATFORM(CHROMIUM).

Re description: In general, the ChangeLog is the description.
Comment 6 William Chan 2010-11-19 09:39:21 PST
(In reply to comment #5)
> Why do we need another flag as opposed to just using PLATFORM(CHROMIUM)? It feels like another flag is only needed if you would choose CHROMIUM_NET independently from PLATFORM(CHROMIUM).

I am ill-equipped to justify this, due to my lack of WebKit knowledge.  I asked abarth how to implement my change for preload scanning and he suggested a USE macro.  If PLATFORM is the way to go, I'm happy to use that instead.  I will note that the Android browser (which I believe is the Android port in the webkit tree, although I'm not totally sure) uses the Chromium network stack.  So if it doesn't/shouldn't use PLATFORM(CHROMIUM), then perhaps USE is more appropriate here.  But I don't particularly care.

> 
> Re description: In general, the ChangeLog is the description.

Cool, great.
Comment 7 WebKit Commit Bot 2010-11-19 17:12:20 PST
Comment on attachment 74344 [details]
Patch

Rejecting patch 74344 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:

    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/RenderSVGTransformableContainer.o /Projects/CommitQueue/WebCore/rendering/RenderSVGTransformableContainer.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/6265026
Comment 8 Ryosuke Niwa 2010-11-19 18:52:26 PST
Comment on attachment 74344 [details]
Patch

Try it again.
Comment 9 WebKit Commit Bot 2010-11-19 19:57:44 PST
The commit-queue encountered the following flaky tests while processing attachment 74344 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html
http/tests/appcache/fail-on-update-2.html

Please file bugs against the tests.  These tests were authored by ap@webkit.org and dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2010-11-19 20:40:42 PST
Comment on attachment 74344 [details]
Patch

Clearing flags on attachment: 74344

Committed r72464: <http://trac.webkit.org/changeset/72464>
Comment 11 WebKit Commit Bot 2010-11-19 20:40:48 PST
All reviewed patches have been landed.  Closing bug.