Closed
Bug 587573
Opened 14 years ago
Closed 14 years ago
Log image requests to the WebConsole
Categories
(DevTools :: General, defect)
DevTools
General
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
Whiteboard: [kd4b5]
Assignee | ||
Comment 1•14 years ago
|
||
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•14 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: --- → ?
Comment 3•14 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•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #467358 -
Flags: review?(dietrich) → review?(sdwilsh)
Comment 7•14 years ago
|
||
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+
Comment 8•14 years ago
|
||
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•14 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.
Comment 10•14 years ago
|
||
(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•14 years ago
|
||
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
Comment 12•14 years ago
|
||
(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•14 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.
Comment 14•14 years ago
|
||
(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•14 years ago
|
||
Hmm. I would be ok with that, yes.
Assignee | ||
Comment 16•14 years ago
|
||
Adds a try{} catch{} for the getWindowForRequest function back (see comment 13 to 15).
Attachment #468670 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 17•14 years ago
|
||
(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•14 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•14 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?
Comment 20•14 years ago
|
||
(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•14 years ago
|
||
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 22•14 years ago
|
||
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•14 years ago
|
||
Removes the try{} catch{} introduced in Patch v1.2 again.
Attachment #469138 -
Attachment is obsolete: true
Comment 26•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
Comment 27•14 years ago
|
||
Comment 28•14 years ago
|
||
(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•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•