587573 - Log image requests to the WebConsole

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Julian Viereck]

Currently, images are not logged to the WebConsole. Log them and also display the images in the Network Panel when the request is inspected.

Comment 1

[Julian Viereck]

Attachment: Patch v1

Adds support for logging images to the WebConsole. This requires changing the origin detection from loadGroups to querying the origin directly from the httpRequest. The patch removes all code that deals with loadGroups as well.

Comment 2

[Julian Viereck]

Requesting blocking status as this patch is important to improve the WebConsole networking. Beside this, switching from request detection by loadGroup to detection by querying seems to improve the quality of the network detection overall (sometimes requests were not logged to the WebConsole - with this patch, I couldn't reproduce this anymore).

Comment 3

[David Dahl :ddahl]

Comment on attachment 466457 [details] [diff] [review]
Patch v1

Sweet!

fix the Firebug code: s/ex/exc/

I think you forgot to hg add the moz.png image.

You removed a BUNCH of code! nice.

Comment 4

[David Dahl :ddahl]

You might also Cu.reportError in some of those catch(ex){} - maybe. perhaps it won't matter to the reviewer

Comment 5

[Julian Viereck]

Attachment: Patch v1 improved + rebase

The moz.png was included in the patch. Changed exc to ex, rebased the patch to apply on tip + added function names NH_getWindowForRequest and NH_getRequestLoadContext. I haven't added reporting for errors in the NetworkHelper functions because it seems to me like these functions are allowed "to fail" and therefore there shouldn't be a notification to the Error Console.

Thanks for your feedback David!

Comment 6

[Shawn Wilsher :sdwilsh]

Blocks a blocker, so this gets the same status.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 467358 [details] [diff] [review]
Patch v1 improved + rebase

For review comments with expandable context, please see http://reviews.visophyte.org/r/467358/.

on file: toolkit/components/console/hudservice/HUDService.jsm line 117
> // FIREBUG CODE BEGIN.

Please make sure to file a bug about including this code.  We need to update
about:licesne (and cc gerv)


on file: toolkit/components/console/hudservice/HUDService.jsm line 181
>     try {
>       if (loadContext) {
>         return loadContext.associatedWindow;
>       }
>     } catch (ex) { }

I'm remembering why I hate firebug code so much.  Exceptions are slow, and I
am pretty sure it's impossible to even throw here.  Drop the try-catch please.


on file: toolkit/components/console/hudservice/HUDService.jsm line 198
>     try {
>       if (aRequest && aRequest.notificationCallbacks) {
>         return aRequest.notificationCallbacks.getInterface(Ci.nsILoadContext);
>       }
>     } catch (ex) { }

move the try-catch to inside the if please


on file: toolkit/components/console/hudservice/HUDService.jsm line 204
>     try {
>       if (aRequest && aRequest.loadGroup
>                    && aRequest.loadGroup.notificationCallbacks) {
>         return aRequest.loadGroup.notificationCallbacks.getInterface(Ci.nsILoadContext);
>       }
>     } catch (ex) { }
> 
>     return null;
>    }

try-catch inside the if please


on file: toolkit/components/console/hudservice/HUDService.jsm line 1141
>             if (win == null) {

you just want |if (win) {|, right?


on file: toolkit/components/console/hudservice/HUDService.jsm line 1147
>             if (hudId == null) {

just |if (hudId) {| here


on file: toolkit/components/console/hudservice/tests/browser/Makefile.in line 61
> 	moz.png \

please choose a more descriptive name like the rest of these for this please


r=sdwilsh with these changes.

Comment 8

[Rob Campbell [:rc] (:robcee)]

s(In reply to comment #7)
> Comment on attachment 467358 [details] [diff] [review]
> Patch v1 improved + rebase
> 
> For review comments with expandable context, please see
> http://reviews.visophyte.org/r/467358/.
> 
> on file: toolkit/components/console/hudservice/HUDService.jsm line 117
> > // FIREBUG CODE BEGIN.
> 
> Please make sure to file a bug about including this code.  We need to update
> about:licesne (and cc gerv)

When I was investigating incorporating pieces of Firebug code into the inspector.

(see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/stylePanel.jsm)

For stand-alone files, I was told we should create a license block with clear separators from it and our own tri-licensed code.

about:license wasn't mentioned to me at the time, but it makes sense that we should need to update it. I'd prefer not blocking on it of course.

Comment 9

[Julian Viereck]

(In reply to comment #7)
> 
> Please make sure to file a bug about including this code.  We need to update
> about:licesne (and cc gerv)

Opened bug 590093.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> about:license wasn't mentioned to me at the time, but it makes sense that we
> should need to update it. I'd prefer not blocking on it of course.
Don't need to block on this bug landing, but we do need to block the release on it.

Comment 11

[Julian Viereck]

Attachment: Patch v1.1

Improved based on Shawn's review. Some comments:

(In reply to comment #7)
> Comment on attachment 467358 [details] [diff] [review]
> on file: toolkit/components/console/hudservice/HUDService.jsm line 1141
> >             if (win == null) {
> 
> you just want |if (win) {|, right?
> 
> 
> on file: toolkit/components/console/hudservice/HUDService.jsm line 1147
> >             if (hudId == null) {
> 
> just |if (hudId) {| here
> 
> 

The test needs to check if win or hudId is undefined/null. In the patch I've changed this to !win and !hudId. Is that want you wanted to address in your comments?

Also, I've changed testNet function to not use a `DOMContentLoaded` but instead a `load` event as the image might not be requested at the point the `DOMContentLoaded` is fired.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> The test needs to check if win or hudId is undefined/null. In the patch I've
> changed this to !win and !hudId. Is that want you wanted to address in your
> comments?
Er, yes.  That is exactly what I meant.

> Also, I've changed testNet function to not use a `DOMContentLoaded` but instead
> a `load` event as the image might not be requested at the point the
> `DOMContentLoaded` is fired.
that's fine; I don't need to review that trivial change

Comment 13

[Julian Viereck]

(In reply to comment #7)
> Comment on attachment 467358 [details] [diff] [review]
> Patch v1 improved + rebase
>  
> on file: toolkit/components/console/hudservice/HUDService.jsm line 181
> >     try {
> >       if (loadContext) {
> >         return loadContext.associatedWindow;
> >       }
> >     } catch (ex) { }
> 

From the error console:

Error: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsILoadContext.associatedWindow]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: resource://gre/modules/HUDService.jsm :: NH_getWindowForRequest :: line 405"  data: no]
Source File: resource://gre/modules/HUDService.jsm
Line: 405

This sees to happen when a request is going on and the tab gets closed. Will add it back again.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> (In reply to comment #7)
> > on file: toolkit/components/console/hudservice/HUDService.jsm line 181
> > >     try {
> > >       if (loadContext) {
> > >         return loadContext.associatedWindow;
> > >       }
> > >     } catch (ex) { }
> Error: [Exception... "Component returned failure code: 0x80004002
> (NS_NOINTERFACE) [nsILoadContext.associatedWindow]"  nsresult: "0x80004002
> (NS_NOINTERFACE)"  location: "JS frame :: resource://gre/modules/HUDService.jsm
> :: NH_getWindowForRequest :: line 405"  data: no]
> Source File: resource://gre/modules/HUDService.jsm
> Line: 405
*sigh*

Looks like nsDocShell::GetAssociatedWindow just calls CallGetInterface, which will throw if it can't get the interface.  We should probably see if that fails, and then return null instead...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  I would be ok with that, yes.

Comment 16

[Julian Viereck]

Attachment: Patch v1.2

Adds a try{} catch{} for the getWindowForRequest function back (see comment 13 to 15).

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Adds a try{} catch{} for the getWindowForRequest function back (see comment 13
> to 15).
per comment 15 though, we should just fix the docshell code so you don't have to do that (two line fix).

Comment 18

[Julian Viereck]

(In reply to comment #17)
> (In reply to comment #16)
> > Adds a try{} catch{} for the getWindowForRequest function back (see comment 13
> > to 15).
> per comment 15 though, we should just fix the docshell code so you don't have
> to do that (two line fix).

Sorry, didn't got that.

Comment 19

[David Dahl :ddahl]

(In reply to comment #18)
> > per comment 15 though, we should just fix the docshell code so you don't have
> > to do that (two line fix).
> 
> Sorry, didn't got that.

So is this r+ or not? Is there another dependency?

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> So is this r+ or not? Is there another dependency?
attach the patch for this change and I'll promise to r+ it quickly (I assume bz is OK with me reviewing that change in nsDocShell).

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: docshell fix v1.0

Fixes docshell bit and actually brings the implementation inline with the documentation (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl#53).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 469258 [details] [diff] [review]
docshell fix v1.0

s/nsILoadGroup/nsILoadContext/

Fix GetTopWindow too? r=me with that.

Comment 23

[Julian Viereck]

Attachment: Patch v1.3

Removes the try{} catch{} introduced in Patch v1.2 again.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: docshell fix v1.1

per review comments

Comment 25

[Julian Viereck]

Note: land both patches at once!

Comment 26

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/9d6224925206

Comment 27

[David Dahl :ddahl]

and 

http://hg.mozilla.org/mozilla-central/rev/b99d25697307

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> http://hg.mozilla.org/mozilla-central/rev/b99d25697307
This didn't land with the commit message that I had already in the patch which was far more descriptive.  In the future, please land with the commit message in the patch unless something is wrong with it.