Bug 56602 - [Chromium] Inspector does not always shows transferred size correctly (e.g. gzipped/chunked content)
: [Chromium] Inspector does not always shows transferred size correctly (e.g. g...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 56691
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-17 15:38 PDT by Vsevolod Vlasov
Modified: 2012-01-10 05:06 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.10 KB, patch)
2011-03-24 13:01 PDT, Vsevolod Vlasov
pfeldman: review-
Details | Formatted Diff | Diff
Patch that does not break chromium (6.57 KB, patch)
2011-03-24 14:09 PDT, Vsevolod Vlasov
pfeldman: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
Added changelog (7.74 KB, patch)
2011-03-25 13:52 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch with didReceiveData2 (4.90 KB, patch)
2011-03-29 08:09 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-03-17 15:38:28 PDT
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.
Comment 1 Vsevolod Vlasov 2011-03-24 13:01:28 PDT
Created attachment 86817 [details]
Patch
Comment 2 Pavel Feldman 2011-03-24 13:04:47 PDT
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
Comment 3 Vsevolod Vlasov 2011-03-24 14:09:06 PDT
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.
Comment 4 Vsevolod Vlasov 2011-03-24 16:08:57 PDT
One test failed on win bot, but these ExtensionApiTests fail frequently.

http://build.chromium.org/p/tryserver.chromium/builders/win/builds/22928
Comment 5 WebKit Commit Bot 2011-03-25 11:37:18 PDT
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
Comment 6 Vsevolod Vlasov 2011-03-25 13:52:38 PDT
Created attachment 86980 [details]
Added changelog
Comment 7 WebKit Commit Bot 2011-03-28 22:54:19 PDT
Comment on attachment 86980 [details]
Added changelog

Clearing flags on attachment: 86980

Committed r82195: <http://trac.webkit.org/changeset/82195>
Comment 8 WebKit Commit Bot 2011-03-28 22:54:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Yuta Kitamura 2011-03-29 03:34:57 PDT
It seems this change broke Chromium Clang builds. I'm rolling out this change...
Comment 10 Yuta Kitamura 2011-03-29 03:35:28 PDT
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
Comment 11 Yuta Kitamura 2011-03-29 03:38:23 PDT
Reverted r82195 for reason:

Broke Chromium Clang builds

Committed r82223: <http://trac.webkit.org/changeset/82223>
Comment 12 Vsevolod Vlasov 2011-03-29 08:09:39 PDT
Created attachment 87333 [details]
Patch with didReceiveData2
Comment 13 Pavel Feldman 2011-03-29 08:33:51 PDT
Comment on attachment 87333 [details]
Patch with didReceiveData2

Clearing flags on attachment: 87333

Committed r82257: <http://trac.webkit.org/changeset/82257>
Comment 14 Pavel Feldman 2011-03-29 08:34:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Commit Bot 2011-03-29 10:33:07 PDT
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 16 Darin Fisher (:fishd, Google) 2011-04-05 11:48:01 PDT
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.
Comment 17 John Knottenbelt 2012-01-09 06:23:17 PST
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?
Comment 18 Vsevolod Vlasov 2012-01-10 05:06:31 PST
(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.