Bug 56602 – [Chromium] Inspector does not always shows transferred size correctly (e.g. gzipped/chunked content)

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

After resolving https://bugs.webkit.org/show_bug.cgi?id=19793 inspector supports showing real transfer size (when it is different from content size).
This should be implemented in chromium port.

[Vsevolod Vlasov]

Created attachment 86817 [details]
Patch

[Pavel Feldman]

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

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

One test failed on win bot, but these ExtensionApiTests fail frequently.

http://build.chromium.org/p/tryserver.chromium/builders/win/builds/22928

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

Created attachment 86980 [details]
Added changelog

[WebKit Commit Bot]

Comment on attachment 86980 [details]
Added changelog

Clearing flags on attachment: 86980

Committed r82195: <http://trac.webkit.org/changeset/82195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

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

[Yuta Kitamura]

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

[Yuta Kitamura]

Reverted r82195 for reason:

Broke Chromium Clang builds

Committed r82223: <http://trac.webkit.org/changeset/82223>

[Vsevolod Vlasov]

Created attachment 87333 [details]
Patch with didReceiveData2

[Pavel Feldman]

Comment on attachment 87333 [details]
Patch with didReceiveData2

Clearing flags on attachment: 87333

Committed r82257: <http://trac.webkit.org/changeset/82257>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[Darin Fisher (:fishd, Google)]

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.

[John Knottenbelt]

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?

[Vsevolod Vlasov]

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