Bug 62810 – Web Inspector: Network panel does not show main resource content for iframes deleted from the document

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

[Vsevolod Vlasov]

Network panel does not show main resource content for iframes deleted from the document.

chromium bug: https://code.google.com/p/chromium/issues/detail?id=82899

The following change includes InspectorPageAgent refactoring exposing some of the content decoding methods to InspectorResourceAgent.
Note that this refactoring will be needed for fixing bugs related to loading the same url in different context (e.g. loading the same url as an image and as a stylesheet).

[Vsevolod Vlasov]

Created attachment 97484 [details]
patch

[Collabora GTK+ EWS bot]

Comment on attachment 97484 [details]
patch

Attachment 97484 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8874304

[Alexander Pavlov (apavlov)]

Comment on attachment 97484 [details]
patch

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

The resource buffer extraction changes look sane in general.

> Source/WebCore/inspector/InspectorPageAgent.cpp:156
> +    return InspectorPageAgent::sharedBufferContent(buffer, textEncodingName, withBase64Encode, result);

optional: You might go without the class qualification here and below. Do we have a determined rule for this in the guidelines?

> Source/WebCore/inspector/InspectorPageAgent.cpp:159
> +bool InspectorPageAgent::sharedBufferContent(PassRefPtr<SharedBuffer> buffer, const String& textEncodingName, bool withBase64Encode, String* result)

For static members, we add the "// static" comment on the preceding line

> Source/WebCore/inspector/InspectorPageAgent.cpp:174
> +void InspectorPageAgent::resourceContent(ErrorString* errorString, Frame* frame, const KURL& url, bool base64Encode, String* result)

Please add // static above - I must have missed it.

[Pavel Feldman]

Comment on attachment 97484 [details]
patch

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

> LayoutTests/http/tests/inspector/network/network-iframe-load-and-delete.html:12
> +    iframe.addEventListener('load', iframeLoaded);

iframe.onload = iframeLoaded;

> LayoutTests/http/tests/inspector/network/network-iframe-load-and-delete.html:18
> +    iframe.removeEventListener('load', iframeLoaded);

No need to remove the listener.

[Vsevolod Vlasov]

Created attachment 97572 [details]
patch with build fixes + comments addressed

[Vsevolod Vlasov]

(In reply to comment #3)
> (From update of attachment 97484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97484&action=review
> 
> The resource buffer extraction changes look sane in general.
> 
> > Source/WebCore/inspector/InspectorPageAgent.cpp:156
> > +    return InspectorPageAgent::sharedBufferContent(buffer, textEncodingName, withBase64Encode, result);
> 
> optional: You might go without the class qualification here and below. Do we have a determined rule for this in the guidelines?

According to what I see in MainResourceLoader I removed class qualification for InspectorPageAgent::resourceContent() call. This one, however, is called from file static method, so class qualification could not be omitted.

[Gustavo Noronha (kov)]

Comment on attachment 97572 [details]
patch with build fixes + comments addressed

Attachment 97572 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8876564

[Vsevolod Vlasov]

Created attachment 97586 [details]
build fixes

[Pavel Feldman]

Comment on attachment 97586 [details]
build fixes

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

> Source/WebCore/inspector/InspectorPageAgent.cpp:160
> +bool InspectorPageAgent::sharedBufferContent(PassRefPtr<SharedBuffer> buffer, const String& textEncodingName, bool withBase64Encode, String* result)

Consider renaming to non-getter name (fetchSharedBufferConnect).

> Source/WebCore/inspector/NetworkResourcesData.h:72
> +        InspectorPageAgent::ResourceType type() const { return m_type; }

I'd try not to increase the amount of network->page dependencies. Can we keep them separate?

[WebKit Review Bot]

Comment on attachment 97586 [details]
build fixes

Clearing flags on attachment: 97586

Committed r89163: <http://trac.webkit.org/changeset/89163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.