[Chromium] Inspector does not show resources loaded by plugins

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

[Yury Semikhatsky]

What steps will reproduce the problem?
1. Open a page with flash e.g. yourube.com
2. Right-click anywhere and select "Inspect element".
3. Go to resources in the Inspector window.
4. Reload the URL in the same tab.

What is the expected result?
Show lots of files, including SWFs.

What happens instead?
Only the scripts are shown and an image (beside the HTML).
No SWFs nor their resources.

[Yury Semikhatsky]

Created attachment 43770 [details]
patch

[Pavel Feldman]

Comment on attachment 43770 [details]
patch

> -void InspectorController::didFinishLoading(DocumentLoader*, unsigned long identifier)
> +void InspectorController::didFinishLoading(unsigned long identifier)

This does not seem to be used. Should we call into it from ResourceLoadNotifier::didFailToLoad?


> +    // Update load time.
> +    m_endTime = currentTime();
> +    m_changes.set(TimingChange);

Could you comment on why it is needed?

There are clearly two orthogonal changes in this patch: one for WebCore, one for WebKit/chromium. Could you split them?

[Yury Semikhatsky]

Created attachment 43775 [details]
patch

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 43770 [details])
> > -void InspectorController::didFinishLoading(DocumentLoader*, unsigned long identifier)
> > +void InspectorController::didFinishLoading(unsigned long identifier)
> 
> This does not seem to be used. Should we call into it from
> ResourceLoadNotifier::didFailToLoad?
>
Done.
 
> 
> > +    // Update load time.
> > +    m_endTime = currentTime();
> > +    m_changes.set(TimingChange);
> 
> Could you comment on why it is needed?
>
Extended the comment.
 
> There are clearly two orthogonal changes in this patch: one for WebCore, one
> for WebKit/chromium. Could you split them?
Chromium side of the fix depends on both of the changes so I'd prefer to submit them together.

[Eric Seidel (no email)]

Comment on attachment 43775 [details]
patch

Style violations:
 int WebFrameImpl::createUniqueIdentifierForRequest() {
 847   return m_frame->page()->progress()->createUniqueIdentifier();
 848 }

Please run check-webkit-style on your patch.

I'm not sure why we have "unsigned long" how is that different from just "unsigned", I thought that to be 64 bit one needed "long long" and that "long" was 32 bit, but maybe I'm remembering wrong.

Why is loader unused?  Why was it added in the first place?

Can we test this?

[Darin Fisher (:fishd, Google)]

Comment on attachment 43775 [details]
patch

> +++ b/WebKit/chromium/public/WebDevToolsAgent.h
...
> +    virtual void identifierForInitialRequestForPlugin(int resourceId, const WebURLRequest& request) = 0;
> +    virtual void willSendRequestForPlugin(int resourceId, const WebURLRequest&) = 0;
> +    virtual void didReceiveDataForPlugin(int resourceId, int length) = 0;
> +    virtual void didReceiveResponseForPlugin(int resourceId, const WebURLResponse& response) = 0;
> +    virtual void didFinishLoadingForPlugin(int resourceId) = 0;
> +    virtual void didFailLoadingForPlugin(int resourceId, const WebURLError& error) = 0;

nit: in webkit style, you should exclude names for parameters if the name
doesn't add any information.  So, drop "request", "response", and "error"
above.


> +++ b/WebKit/chromium/public/WebFrame.h
...
> +    // Returns next unused request identifier which is unique within the
> +    // parent Page.
> +    virtual int createUniqueIdentifierForRequest() = 0;

This should be a method on WebView, not WebFrame.  WebView corresponds
to Page.

[Yury Semikhatsky]

Created attachment 43838 [details]
patch

[Yury Semikhatsky]

(In reply to comment #6)
> nit: in webkit style, you should exclude names for parameters if the name
> doesn't add any information.  So, drop "request", "response", and "error"
> above.
>
Done.
  
> > +++ b/WebKit/chromium/public/WebFrame.h
> 
> This should be a method on WebView, not WebFrame.  WebView corresponds
> to Page.
Done. Thanks.


(In reply to comment #5)
> (From update of attachment 43775 [details])
> Style violations:
>  int WebFrameImpl::createUniqueIdentifierForRequest() {
>  847   return m_frame->page()->progress()->createUniqueIdentifier();
>  848 }
> 
> Please run check-webkit-style on your patch.
>
Fixed.


> I'm not sure why we have "unsigned long" how is that different from just
> "unsigned", I thought that to be 64 bit one needed "long long" and that "long"
> was 32 bit, but maybe I'm remembering wrong.
> 
I don't know why exactly the identifier type was a while ago changed from void* to unsigned long and not to unsigned int but currently WebCore code uses unsigned long for resource ids. On my 64 bit machine sizeof(int)==4 and sizeof(unsigned long) == 8 so there is a difference.


> Why is loader unused?  Why was it added in the first place?
> 
I guess it might have been added for consistency with InspectorController::willSendRequest and InspectorController::identifierForInitialRequest but the parameter has never been used in the methods I'm removing it from.


> Can we test this?
I'd like to test the failure notification but I don't quite understand how to test this in layout tests. Apart from that there is no new functionality.

[Pavel Feldman]

Comment on attachment 43838 [details]
patch

It is not clear why WebDevToolsAgent has int for resourceID whereas everything else uses unsigned long. (r- for this).
Also note that this can't be landed until implementation of the agent is landed downstream.

[Yury Semikhatsky]

Created attachment 43850 [details]
patch

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 43838 [details])
> It is not clear why WebDevToolsAgent has int for resourceID whereas everything
> else uses unsigned long. (r- for this).
Done.

> Also note that this can't be landed until implementation of the agent is landed
> downstream.
I'll send you Chromium part of the fix for review in a minute.

[Pavel Feldman]

Comment on attachment 43850 [details]
patch

I think you want to land it manually.

[Yury Semikhatsky]

Created attachment 43857 [details]
patch

Removed *ForPlugin suffix from WebDevToolsAgent methods as we agreed offline.

[Yury Semikhatsky]

Created attachment 43922 [details]
patch

Removed DocumentLoader* parameter from willSendRequest since it's not needed there.

Pass WebFrame* as a parameter to WebDevToolsAgent::indentifierForInitialRequest.

[Yury Semikhatsky]

Created attachment 43923 [details]
patch

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorResource.cpp
	M	WebCore/loader/ResourceLoadNotifier.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebDevToolsAgent.h
	M	WebKit/chromium/public/WebView.h
	M	WebKit/chromium/src/WebViewImpl.cpp
	M	WebKit/chromium/src/WebViewImpl.h
Committed r51421

[Yury Semikhatsky]

It was reverted in r51422 due to Snow Leopard build failures.

(In reply to comment #16)
> Committing to http://svn.webkit.org/repository/webkit/trunk ...
>     M    WebCore/ChangeLog
>     M    WebCore/inspector/InspectorController.cpp
>     M    WebCore/inspector/InspectorController.h
>     M    WebCore/inspector/InspectorResource.cpp
>     M    WebCore/loader/ResourceLoadNotifier.cpp
>     M    WebKit/chromium/ChangeLog
>     M    WebKit/chromium/public/WebDevToolsAgent.h
>     M    WebKit/chromium/public/WebView.h
>     M    WebKit/chromium/src/WebViewImpl.cpp
>     M    WebKit/chromium/src/WebViewImpl.h
> Committed r51421

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorResource.cpp
	M	WebCore/loader/ResourceLoadNotifier.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebDevToolsAgent.h
	M	WebKit/chromium/public/WebView.h
	M	WebKit/chromium/src/WebViewImpl.cpp
	M	WebKit/chromium/src/WebViewImpl.h
Committed r51440