Bug 78445 - Web Inspector: include failed and canceled in FrameResourceTree
Summary: Web Inspector: include failed and canceled in FrameResourceTree
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: Timothy Hatcher
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-12 17:47 PST by Timothy Hatcher
Modified: 2012-02-14 11:43 PST (History)
16 users (show)

See Also:


Attachments
Proposed Change (5.55 KB, patch)
2012-02-12 17:51 PST, Timothy Hatcher
timothy: review-
pnormand: commit-queue-
Details | Formatted Diff | Diff
Proposed Change (with build fix) (9.11 KB, patch)
2012-02-12 18:15 PST, Timothy Hatcher
pfeldman: review-
Details | Formatted Diff | Diff
Proposed Change (round 2) (38.53 KB, patch)
2012-02-14 03:23 PST, Timothy Hatcher
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 Timothy Hatcher 2012-02-12 17:47:20 PST
The protocol should include some more info about resources in FrameResourceTree. Like failed, canceld, loading, resource size and request identifier.
Comment 1 Timothy Hatcher 2012-02-12 17:51:01 PST
Created attachment 126693 [details]
Proposed Change
Comment 2 Philippe Normand 2012-02-12 17:58:33 PST
Comment on attachment 126693 [details]
Proposed Change

Attachment 126693 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11507627
Comment 3 Early Warning System Bot 2012-02-12 17:59:19 PST
Comment on attachment 126693 [details]
Proposed Change

Attachment 126693 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11507628
Comment 4 Gyuyoung Kim 2012-02-12 18:00:15 PST
Comment on attachment 126693 [details]
Proposed Change

Attachment 126693 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11509553
Comment 5 Timothy Hatcher 2012-02-12 18:05:31 PST
Comment on attachment 126693 [details]
Proposed Change

Forgot the CachedResource.h change adding loader().
Comment 6 Timothy Hatcher 2012-02-12 18:15:44 PST
Created attachment 126698 [details]
Proposed Change (with build fix)
Comment 7 Pavel Feldman 2012-02-13 00:06:26 PST
Comment on attachment 126698 [details]
Proposed Change (with build fix)

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

At this moment, we only add methods to the protocol that are used by the front-end. This way we can guarantee it is useful and we can test it. Are you planning on using this data in the front-end or is there alternative front-end it is for? I am fine with changing the rules to the "we can expose functionality not used by the inspector front-end", but we need to make sure these methods are tested.

> Source/WebCore/inspector/Inspector.json:141
> +                                { "name": "decodedSize", "type": "integer", "optional": true, "description": "Decoded resource size." },

These are going to be wrong for the ports that have network stack responsible for encoding.

> Source/WebCore/inspector/Inspector.json:143
> +                                { "name": "loading", "type": "boolean", "optional": true, "description": "True if the resource is still loading." },

I am not sure how relevant this is given the asynchronous nature of the front-end - backend interaction. There is no guarantee that it is still "true" at the moment message is received.
Comment 8 Timothy Hatcher 2012-02-13 07:32:43 PST
(In reply to comment #7)
> (From update of attachment 126698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126698&action=review
> 
> At this moment, we only add methods to the protocol that are used by the front-end. This way we can guarantee it is useful and we can test it. Are you planning on using this data in the front-end or is there alternative front-end it is for? I am fine with changing the rules to the "we can expose functionality not used by the inspector front-end", but we need to make sure these methods are tested.

Do we have other tests that just test the protocol and not the front-end?

> 
> > Source/WebCore/inspector/Inspector.json:141
> > +                                { "name": "decodedSize", "type": "integer", "optional": true, "description": "Decoded resource size." },
> 
> These are going to be wrong for the ports that have network stack responsible for encoding.

I'm fine just having one property, "size",  and using encodedSize.

> > Source/WebCore/inspector/Inspector.json:143
> > +                                { "name": "loading", "type": "boolean", "optional": true, "description": "True if the resource is still loading." },
> 
> I am not sure how relevant this is given the asynchronous nature of the front-end - backend interaction. There is no guarantee that it is still "true" at the moment message is received.

You would also observe the Network.loadingFailed and Network.loadingFinished events. You would have those events queued up to make it false.
Comment 9 Timothy Hatcher 2012-02-14 03:23:13 PST
Created attachment 126951 [details]
Proposed Change (round 2)
Comment 10 WebKit Review Bot 2012-02-14 03:26:01 PST
Attachment 126951 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Pavel Feldman 2012-02-14 03:34:42 PST
Comment on attachment 126951 [details]
Proposed Change (round 2)

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

> LayoutTests/inspector/protocol/page-agent-expected.txt:57
> +                {

I wonder if this could result in flakiness. Do you know if the order cached resources are created and persisted is fixed?
Comment 12 WebKit Review Bot 2012-02-14 03:59:00 PST
Comment on attachment 126951 [details]
Proposed Change (round 2)

Attachment 126951 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514268

New failing tests:
inspector/protocol/page-agent.html
Comment 13 Tony Chang 2012-02-14 11:27:57 PST
Comment on attachment 126951 [details]
Proposed Change (round 2)

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

>> LayoutTests/inspector/protocol/page-agent-expected.txt:57
>> +                {
> 
> I wonder if this could result in flakiness. Do you know if the order cached resources are created and persisted is fixed?

This is indeed causing flakiness.  We're getting a different order on the 3 bots that have run this test so far.  This is also why the ews bot failed.
Comment 14 Timothy Hatcher 2012-02-14 11:31:13 PST
Sigh. I'm going to disable the test. I want to keep the change.
Comment 15 Timothy Hatcher 2012-02-14 11:43:14 PST
Removed the test in r107720 and filed bug 78621 about making a better test.
Comment 16 Timothy Hatcher 2012-02-14 11:43:56 PST
Fixed in r107714.