Log image requests to the WebConsole

RESOLVED FIXED in Firefox 4.0b5

Status

DevTools
General
RESOLVED FIXED
8 years ago
8 days ago

People

(Reporter: Julian Viereck, Assigned: Julian Viereck)

Tracking

Trunk
Firefox 4.0b5
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(Whiteboard: [kd4b5])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

8 years ago
Whiteboard: [kd4b5]
(Assignee)

Comment 1

8 years ago
Created attachment 466457 [details] [diff] [review]
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.
Attachment #466457 - Flags: feedback?(ddahl)
(Assignee)

Comment 2

8 years ago
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: --- → ?
(Assignee)

Updated

8 years ago
Blocks: 568634

Comment 3

8 years ago
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+

Comment 4

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

Comment 5

8 years ago
Created attachment 467358 [details] [diff] [review]
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!
Attachment #466457 - Attachment is obsolete: true
Attachment #467358 - Flags: review?(dietrich)
(Assignee)

Updated

8 years ago
Attachment #467358 - Flags: review?(dietrich) → review?(sdwilsh)
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
Created attachment 468670 [details] [diff] [review]
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.
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
(Assignee)

Comment 13

8 years ago
(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...
(Assignee)

Comment 16

8 years ago
Created attachment 469138 [details] [diff] [review]
Patch v1.2

Adds a try{} catch{} for the getWindowForRequest function back (see comment 13 to 15).
Attachment #468670 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
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).
(Assignee)

Comment 18

8 years ago
(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

Comment 19

8 years ago
(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).
Created attachment 469258 [details] [diff] [review]
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).
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+
(Assignee)

Comment 23

8 years ago
Created attachment 469426 [details] [diff] [review]
Patch v1.3

Removes the try{} catch{} introduced in Patch v1.2 again.
Attachment #469138 - Attachment is obsolete: true
Created attachment 469466 [details] [diff] [review]
docshell fix v1.1

per review comments
Attachment #469258 - Attachment is obsolete: true
(Assignee)

Comment 25

8 years ago
Note: land both patches at once!
Keywords: checkin-needed

Updated

8 years ago
Blocks: 568643

Comment 26

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9d6224925206
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.

Updated

8 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.