Bug 84566 - Web Inspector: [chromium] show more useful information for throttled requests.
Summary: Web Inspector: [chromium] show more useful information for throttled requests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: yzshen
URL:
Keywords:
Depends on:
Blocks: 79666
  Show dependency treegraph
 
Reported: 2012-04-22 23:32 PDT by yzshen
Modified: 2012-05-01 05:28 PDT (History)
20 users (show)

See Also:


Attachments
Patch (4.76 KB, patch)
2012-04-23 15:51 PDT, yzshen
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2012-04-25 18:21 PDT, yzshen
pfeldman: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yzshen 2012-04-22 23:32:24 PDT
Currently, when a request is intentionally throttled by URLRequestThrottler, it shows up as a failure in the DevTools console and the network tab, without any specific information. It may cause confusion for users because it looks the same as other failures, such as the request is sent but no response is received.

The corresponding Chromium bug is http://code.google.com/p/chromium/issues/detail?id=109857.
Comment 1 yzshen 2012-04-22 23:32:41 PDT
I will upload a patch soon.
Comment 2 yzshen 2012-04-23 15:51:42 PDT
Created attachment 138444 [details]
Patch
Comment 3 WebKit Review Bot 2012-04-23 15:57:27 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 yzshen 2012-04-23 16:10:34 PDT
Hi, Inspector experts.

This patch shows the description for failed requests in the Status column of the Network tab.

Currently the Status column shows things like:
successful request: "304 \n Not Modified"
failed request: "(failed)"

This patch will show the following for failed requests:
"(failed) \n <localizedFailDescription>"

To me, this adds more useful information for failures (e.g., for requests rejected by the Chromium-specific feature of HTTP throttling). And it matches the successful cases, which have already had a human-friendly description.

Please let me know if this looks okay to you. I would be happy to make changes if you have better ideas. Thanks in advance!
Comment 5 WebKit Review Bot 2012-04-24 11:33:14 PDT
Comment on attachment 138444 [details]
Patch

Clearing flags on attachment: 138444

Committed r115086: <http://trac.webkit.org/changeset/115086>
Comment 6 WebKit Review Bot 2012-04-24 11:33:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2012-04-25 02:03:29 PDT
It made a test fail on Qt platform:

--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/inspector/network-status-non-http-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/inspector/network-status-non-http-actual.txt 
@@ -1,6 +1,6 @@
 {
     0 : "data:application/javascript: Success"
     1 : "network-test.js: Success"
-    2 : "non-existent-file.js: (failed)"
+    2 : "non-existent-file.js: (failed)Error opening /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/LayoutTests/inspector/non-existent-file.js: No such file or directory"
 }
 
Could you check it, please?
Comment 8 Szilard Ledan 2012-04-25 05:29:50 PDT
This patch inspector/network-status-non-http.html fails on qt. This test has been skipped until it is fixed. See http://trac.webkit.org/changeset/115188
Please unskip it with the proper fix.
Comment 9 yzshen 2012-04-25 09:40:18 PDT
Sorry for the inconvenience.
I will fix it as soon as possible.
Comment 10 yzshen 2012-04-25 18:21:57 PDT
Created attachment 138916 [details]
Patch
Comment 11 yzshen 2012-04-25 18:23:54 PDT
Comment on attachment 138916 [details]
Patch

This patch fixes the test regression. Please take a look.
Comment 12 Yury Semikhatsky 2012-04-25 22:04:07 PDT
Comment on attachment 138916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138916&action=review

> LayoutTests/inspector/network-status-non-http.html:24
> +                if (outputStatus.indexOf("(failed)") == 0)

Please use .startsWith for prefix checks.
Comment 13 yzshen 2012-04-26 10:03:31 PDT
(In reply to comment #12)
> (From update of attachment 138916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138916&action=review
> 
> > LayoutTests/inspector/network-status-non-http.html:24
> > +                if (outputStatus.indexOf("(failed)") == 0)
> 
> Please use .startsWith for prefix checks.

Javascript string doesn't have .startsWith method, does it?
Comment 14 Pavel Feldman 2012-04-28 00:31:09 PDT
Comment on attachment 138916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138916&action=review

>>> LayoutTests/inspector/network-status-non-http.html:24
>>> +                if (outputStatus.indexOf("(failed)") == 0)
>> 
>> Please use .startsWith for prefix checks.
> 
> Javascript string doesn't have .startsWith method, does it?

We added it to the String's prototype in utilities.js
Comment 15 WebKit Review Bot 2012-04-30 07:44:59 PDT
Comment on attachment 138916 [details]
Patch

Rejecting attachment 138916 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ueue/

Parsed 3 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/network-status-non-http.html
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 2616.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Pavel Feld..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12597070
Comment 16 yzshen 2012-04-30 12:55:35 PDT
(In reply to comment #14)
> (From update of attachment 138916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138916&action=review
> 
> >>> LayoutTests/inspector/network-status-non-http.html:24
> >>> +                if (outputStatus.indexOf("(failed)") == 0)
> >> 
> >> Please use .startsWith for prefix checks.
> > 
> > Javascript string doesn't have .startsWith method, does it?
> 
> We added it to the String's prototype in utilities.js

(Sorry I am not familiar with this part of codebase.)
Is it allowed to directly refer to utilities.js from the LayoutTests folder?
Or we have other ways to access startsWith?
Comment 17 Andreas Kling 2012-05-01 05:28:11 PDT
Committed r115720: <http://trac.webkit.org/changeset/115720>