Bug 56602

Created attachment 86817 [details] Patch

Comment on attachment 86817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86817&action=review > Source/WebKit/chromium/public/WebDevToolsAgent.h:97 > + virtual void didReceiveData(unsigned long resourceId, int length, int lengthReceived) = 0; This will break upon the roll. You should make 3 patches: 1) add new method, preserve old method - wait until deps are rolled 2) migrate chromium to the new method 3) nuke old method upstream

Created attachment 86833 [details] Patch that does not break chromium I have started try bots to be sure this patch does not break chromium, I will write once they have finished.

One test failed on win bot, but these ExtensionApiTests fail frequently. http://build.chromium.org/p/tryserver.chromium/builders/win/builds/22928

Comment on attachment 86833 [details] Patch that does not break chromium Rejecting attachment 86833 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 2 Last 500 characters of output: 9b69f455c6e1d456fe987a8ec14aeb1caed763c7 r81968 = 99f2897a0668627b2e1a5ad2ae3fa0e53ba784c5 r81969 = fede50ddeb05dab38f40fb34c6a3e57680a34e15 r81970 = 66e1a3a6619dbe9d9f04793001909735c77d07ea r81971 = 80a4fce588aca60b144f21524d2518fab7e5d001 r81972 = 5869263c747ee704ae7a9976eea0082a48153085 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://queues.webkit.org/results/8252177

Created attachment 86980 [details] Added changelog

Comment on attachment 86980 [details] Added changelog Clearing flags on attachment: 86980 Committed r82195: <http://trac.webkit.org/changeset/82195>

All reviewed patches have been landed. Closing bug.

It seems this change broke Chromium Clang builds. I'm rolling out this change...

Error message: In file included from content/renderer/render_view.cc:195: ./webkit/plugins/npapi/webplugin_impl.h:85:16: error: 'webkit::npapi::WebPluginImpl::didReceiveData' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void didReceiveData(const char* data, int data_length); ^ In file included from content/renderer/render_view.cc:181: In file included from ./webkit/glue/image_resource_fetcher.h:10: In file included from ./webkit/glue/resource_fetcher.h:22: ./third_party/WebKit/Source/WebKit/chromium/public/WebURLLoaderClient.h:61:18: note: hidden overloaded virtual function 'WebKit::WebURLLoaderClient::didReceiveData' declared here virtual void didReceiveData(WebURLLoader*, const char* data, int dataLength, int lengthReceived) { } ^ 1 error generated. make: *** [out/Debug/obj.target/content_renderer/content/renderer/render_view.o] Error 1

Reverted r82195 for reason: Broke Chromium Clang builds Committed r82223: <http://trac.webkit.org/changeset/82223>

Created attachment 87333 [details] Patch with didReceiveData2

Comment on attachment 87333 [details] Patch with didReceiveData2 Clearing flags on attachment: 87333 Committed r82257: <http://trac.webkit.org/changeset/82257>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 87333 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 87333 [details] Patch with didReceiveData2 View in context: https://bugs.webkit.org/attachment.cgi?id=87333&action=review > Source/WebKit/chromium/public/WebURLLoaderClient.h:61 > + // FIXME(vsevik): rename once original didReceiveData() is removed. nit: webkit style is to not put names next to FIXME. nit: you should document lengthReceived. it is not obvious how dataLength and lengthReceived differ. if you read the comment "Called when a chunk of response data is received", then you might think that lengthReceived indicates the amount received this time, but actually that is dataLength. also, there is no indication that lengthReceived somehow relates to the length before the data was decompressed.

I notice that there a bunch of skipped tests in Chromium's test_expectations.txt, which refer to this bug: BUGWK56602 SKIP : http/tests/inspector/network/network-timing.html = FAIL BUGWK56602 SKIP : http/tests/inspector/network/network-size.html = FAIL BUGWK56602 SKIP : http/tests/inspector/network/network-size-chunked.html = FAIL BUGWK56602 SKIP : http/tests/inspector/network/network-size-sync.html = FAIL Now that this bug is marked as fixed, should these tests still be skipped?

(In reply to comment #17) > I notice that there a bunch of skipped tests in Chromium's test_expectations.txt, which refer to this bug: > > BUGWK56602 SKIP : http/tests/inspector/network/network-timing.html = FAIL > BUGWK56602 SKIP : http/tests/inspector/network/network-size.html = FAIL > BUGWK56602 SKIP : http/tests/inspector/network/network-size-chunked.html = FAIL > BUGWK56602 SKIP : http/tests/inspector/network/network-size-sync.html = FAIL > > Now that this bug is marked as fixed, should these tests still be skipped? They should be kept skipped as DRT does not support transferred time/size in inspector. I added a comment in test expectations and marked them as WONTFIX.