Closed Bug 587573 Opened 10 years ago Closed 10 years ago

Log image requests to the WebConsole

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 beta5+)

RESOLVED FIXED
Firefox 4.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: julian.viereck, Assigned: julian.viereck)

References

Details

(Whiteboard: [kd4b5])

Attachments

(2 files, 5 obsolete files)

Currently, images are not logged to the WebConsole. Log them and also display the images in the Network Panel when the request is inspected.
Whiteboard: [kd4b5]
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #466457 - Flags: feedback?(ddahl)
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).
blocking2.0: --- → ?
Blocks: 568634
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.
Attachment #466457 - Flags: feedback?(ddahl) → feedback+
You might also Cu.reportError in some of those catch(ex){} - maybe. perhaps it won't matter to the reviewer
Attached patch Patch v1 improved + rebase (obsolete) — Splinter Review
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!
Attachment #466457 - Attachment is obsolete: true
Attachment #467358 - Flags: review?(dietrich)
Attachment #467358 - Flags: review?(dietrich) → review?(sdwilsh)
Blocks: 583476
Blocks a blocker, so this gets the same status.
blocking2.0: ? → beta5+
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.
Attachment #467358 - Flags: review?(sdwilsh) → review+
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.
(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.
(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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #467358 - Attachment is obsolete: true
(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
(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.
(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...
Hmm.  I would be ok with that, yes.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Adds a try{} catch{} for the getWindowForRequest function back (see comment 13 to 15).
Attachment #468670 - Attachment is obsolete: true
Keywords: checkin-needed
(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).
(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.
Keywords: checkin-needed
(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?
(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).
Attached patch docshell fix v1.0 (obsolete) — Splinter Review
Fixes docshell bit and actually brings the implementation inline with the documentation (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl#53).
Attachment #469258 - Flags: review?(bzbarsky)
Comment on attachment 469258 [details] [diff] [review]
docshell fix v1.0

s/nsILoadGroup/nsILoadContext/

Fix GetTopWindow too? r=me with that.
Attachment #469258 - Flags: review?(bzbarsky) → review+
Attached patch Patch v1.3Splinter Review
Removes the try{} catch{} introduced in Patch v1.2 again.
Attachment #469138 - Attachment is obsolete: true
per review comments
Attachment #469258 - Attachment is obsolete: true
Note: land both patches at once!
Keywords: checkin-needed
Blocks: 568643
http://hg.mozilla.org/mozilla-central/rev/9d6224925206
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.