Closed
Bug 595223
Opened 14 years ago
Closed 14 years ago
Keep track of file:// url 'loadGroups'
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(blocking2.0 .x+)
RESOLVED
FIXED
Firefox 6
Tracking | Status | |
---|---|---|
blocking2.0 | --- | .x+ |
People
(Reporter: ddahl, Assigned: msucan)
References
Details
(Whiteboard: [patchclean:0412][softblocker][console-1][fixed-in-devtools][merged-to-mozilla-central])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Now that I am using loadGroups to help surface css errors that fall through the cracks in bug 567165, we need to figure out how to keep track of file uri's that are loaded. The httpActivityObserver does not, afaik. If it can support file:// urls, then lets just do that.
Updated•14 years ago
|
Assignee: nobody → ddahl
Blocks: devtools4b8
Reporter | ||
Comment 1•14 years ago
|
||
We need to create a FileActivityObserver and file away the load groups for each url loaded so that css errors on those pages can correspond to the correct console.
We should think of any other protocols we will need to handle here and create a corresponding Observer for.
Updated•14 years ago
|
Assignee: ddahl → mihai.sucan
Assignee | ||
Comment 2•14 years ago
|
||
David: I looked into this today and there's no http activity equivalent for files, and I also asked in #developers - biesi confirmed my finding. If we want to show "network" requests for local file:// resources, we could use an nsIWebProgressListener, as biesi suggested. Do you think this is a good enough solution? With this progress listener we'll be able to track while files are loaded and display them on click - the network panel doesn't make sense in this context. Shall I go ahead and implement this?
Reporter | ||
Comment 3•14 years ago
|
||
(In reply to comment #2)
> David: I looked into this today and there's no http activity equivalent for
> files, and I also asked in #developers
Indeed, I assumed we would need some kind of observer to track the load groups of non-http traffic. Perhaps the listener can also track ftp:// connections as well?
(not important right now, but we may get if for free)
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3)
> (In reply to comment #2)
> > David: I looked into this today and there's no http activity equivalent for
> > files, and I also asked in #developers
>
> Indeed, I assumed we would need some kind of observer to track the load groups
> of non-http traffic. Perhaps the listener can also track ftp:// connections as
> well?
>
> (not important right now, but we may get if for free)
Yeah, I'll work on this and see if ftp requests are included as well. Thanks for the quick reply!
Assignee | ||
Comment 5•14 years ago
|
||
This is the proposed patch and test code.
Please note that it only makes "network" requests to file:// and ftp:// protocols show up in the Web Console log. The user can't click such network requests because they are not the typical HTTP requests - no request/response headers and the likes.
Additionally, the patch does not tie into the code you wrote for bug 567165. The WIP patch you have there is quite old and I presume you have made improvements.
I expect that bug 567165 will depend on this patch, or the way around.
Note that because this patch uses an nsIWebProgressListener, I am basing my work on top of bug 594523, which also uses a web progress listener.
Please let me know in which direction I should take things forward with this patch. Thanks!
Attachment #479124 -
Flags: feedback?(ddahl)
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 6•14 years ago
|
||
Rebased patch on top of the changes from bug 594523.
Attachment #479124 -
Attachment is obsolete: true
Attachment #480420 -
Flags: feedback?(ddahl)
Attachment #479124 -
Flags: feedback?(ddahl)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0928] → [patchclean:1002]
Reporter | ||
Comment 7•14 years ago
|
||
Comment on attachment 480420 [details] [diff] [review]
rebased patch
Looks good, We will have to write a bridge patch to hook it up to the error listeners for css and script errors.
Attachment #480420 -
Flags: feedback?(ddahl) → feedback+
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 480420 [details] [diff] [review]
rebased patch
Thanks David for the feedback+!
Asking for review from Gavin.
Attachment #480420 -
Flags: review?(gavin.sharp)
Comment 9•14 years ago
|
||
Requesting blocking for this bug. It is surprising behavior (to me at least) when I open the web console on a file and see *nothing* logged for the loading of the stylesheets, images, etc.
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → final+
Comment 10•14 years ago
|
||
Comment on attachment 480420 [details] [diff] [review]
rebased patch
Cancelling this request pending a new plan for bug 594523
Attachment #480420 -
Flags: review?(gavin.sharp)
Comment 14•14 years ago
|
||
ping. what's going to happen with this bug?
Assignee | ||
Comment 15•14 years ago
|
||
Will have to (strongly) rebase this patch on top of bug 611789, if I understood Gavin's plans correctly.
Updated•14 years ago
|
Severity: blocker → normal
Comment 16•14 years ago
|
||
I don't know whether this is the right bug to track or I should create a new one. There is a serious need in an extension to be able to add capacity to handle different types of files as well as different sorts of HTTP content-types. For HTTP content types on the tabulator extension we use the observers of HTTP activity. But we can't cahnge the behaviour of firefox when (a) a file is opened in Firefox from the command line or shell or (b) a file:/ URI is pt in the location bar and return hit.
Reporter | ||
Comment 17•14 years ago
|
||
(In reply to comment #16)
This bug is really focused on file:/// URIs - we may need new bugs for other protocols.
> (a) a file is opened in Firefox from the command line or shell or
(b) a file:/ URI is pt in the location bar and return hit.
I am thinking these actions will not matter - this bug will handle these cases.
*note* we do not have tests that open URIs from the command line. I wonder if browser-chrome tests can do that...
Comment 18•14 years ago
|
||
Will this bug allow the loading of files to be hijacked -- to implement a new file type -- or only logged?
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18)
> Will this bug allow the loading of files to be hijacked -- to implement a new
> file type -- or only logged?
That is not the purpose, I believe, of this bug.
We are only aiming to use the current Gecko API to log local file loads in the Web Console.
What you want is hijacking of file loading, to implement custom behavior on loading different types of files. I expect this is allowed in some way by Gecko already, but I do not have sufficient experience at the moment to tell you how you can do it in your extension (I presume?).
Reporter | ||
Comment 20•14 years ago
|
||
(In reply to comment #18)
> Will this bug allow the loading of files to be hijacked -- to implement a new
> file type -- or only logged?
Only logged. This functionality observes and reports the file that is loaded and logs the JS/CSS errors and warnings that happen. It is very passive.
Comment 21•14 years ago
|
||
Mihai: now that bug 611789 has landed, do we just need to rebase this one?
Updated•14 years ago
|
Whiteboard: [patchclean:1002]
Assignee | ||
Comment 22•14 years ago
|
||
Kevin: yes, something like that. A rebase, but a bit more than that, because there have been quite some changes to the HUDService.
I'll rebase the patch ASAP. Sorry I forgot it.
Assignee | ||
Comment 23•14 years ago
|
||
Rebased the patch.
Attachment #480420 -
Attachment is obsolete: true
Attachment #502056 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0107]
Updated•14 years ago
|
Whiteboard: [patchclean:0107] → [patchclean:0107][softblocker]
Comment 24•14 years ago
|
||
** PRODUCT DRIVERS PLEASE NOTE **
This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:
- it was marked as a soft blocking issue without a requirement for beta coverage
blocking2.0: final+ → .x+
Reporter | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0107][softblocker] → [patchclean:0107][softblocker] [console-1]
Assignee | ||
Comment 25•14 years ago
|
||
Comment on attachment 502056 [details] [diff] [review]
patch update 3
Asking for review from Dave Camp. Thanks!
Attachment #502056 -
Flags: review?(gavin.sharp) → review?(dcamp)
Comment 26•14 years ago
|
||
Just to make sure I'm understanding this correctly: is the this.suspended check strictly necessary? As I'm reading it the service is only suspended when there are no consoles open, at which point there shouldn't be other progress listeners left. Also, you could store the progress listener in the hud object rather than dangling it off the browser element, right?
Assignee | ||
Comment 27•14 years ago
|
||
Hm, right. The code changed/morphed a lot since when I initially wrote the patch. Back then the one-to-one mapping of progress listeners to HUD objects didn't quite work.
We've gone through some code improvements that now allow this. Will update the patch accordingly. Hopefully it will work fine.
As for the check for HUDService.suspended: you're correct, it's a "safety" measure that we do not actually need. If we have progress listeners that are not removed, we have a bug elsewhere in the code.
Thanks for looking into the patch!
Assignee | ||
Comment 28•14 years ago
|
||
Updated the patch as requested. Tests pass in opt and debug builds, locally.
Please let me know if further changes are needed. Thanks!
Attachment #502056 -
Attachment is obsolete: true
Attachment #523638 -
Flags: review?(dcamp)
Attachment #502056 -
Flags: review?(dcamp)
Assignee | ||
Updated•14 years ago
|
Hardware: x86 → All
Whiteboard: [patchclean:0107][softblocker] [console-1] → [patchclean:0401][softblocker] [console-1]
Comment 29•14 years ago
|
||
Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4
Looks good to me, handing over to dolske for a binding r+.
Attachment #523638 -
Flags: review?(dolske)
Attachment #523638 -
Flags: review?(dcamp)
Attachment #523638 -
Flags: review+
Updated•14 years ago
|
Attachment #523638 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 30•14 years ago
|
||
Thank you Dave and Justin for the review pluses!
Whiteboard: [patchclean:0401][softblocker] [console-1] → [patchclean:0401][softblocker] [console-1] [checkin]
Updated•14 years ago
|
Whiteboard: [patchclean:0401][softblocker] [console-1] [checkin] → [patchclean:0401][softblocker] [console-1] [in-devtools][merge-m-c]
Comment 31•14 years ago
|
||
Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4
http://hg.mozilla.org/projects/devtools/rev/102a2b309cc3
Attachment #523638 -
Attachment description: patch update 4 → [in-devtools] patch update 4
Updated•14 years ago
|
Whiteboard: [patchclean:0401][softblocker] [console-1] [in-devtools][merge-m-c] → [patchclean:0401][softblocker][console-1]
Comment 32•14 years ago
|
||
Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4
backed out:
http://hg.mozilla.org/projects/devtools/rev/a9676acc8981
crashed. See:
http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1302450137.1302451330.6255.gz&fulltext=1
Attachment #523638 -
Attachment description: [in-devtools] patch update 4 → [backed-out] patch update 4
Assignee | ||
Comment 33•14 years ago
|
||
That's surprising. I had no crash on my system, and I tested both opt and debug builds.
I'll look into it and see what I can do.
Assignee | ||
Comment 34•14 years ago
|
||
Yesterday I pushed the patch to the try server:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=74b12dea9377
Same failures.
On my system all tests pass and no crashes occur (opt and debug builds).
I suspect that the problem is with:
+ let rootDir = getResolvedURI(getRootDirectory(gTestPath));
+ addTab(rootDir.spec + TEST_FILE);
(in the test file)
These two lines determine the test physical path on the system and open a tab to the file I want. This might fail. Still, I found this code in some other tests which also tested loading of local files.
If the URI I try to load in the test is wrong, the test should simply fail. The crasher puzzles me, because it means something else is probably going wrong.
Assignee | ||
Comment 35•14 years ago
|
||
Updated the patch with an attempt to fix the test. I changed the way it tries to find the test file location.
Will push this again to the try server.
Attachment #523638 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0401][softblocker][console-1] → [patchclean:0412][softblocker][console-1]
Assignee | ||
Comment 36•14 years ago
|
||
Another patch update. Thanks to Dave Camp for his assistance with determining the problem: jars are used on the try server.
Try server results:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=bdfd0b62a1f7
Attachment #525419 -
Attachment is obsolete: true
Assignee | ||
Comment 37•14 years ago
|
||
Results are green!
Whiteboard: [patchclean:0412][softblocker][console-1] → [patchclean:0412][softblocker][console-1][checkin]
Assignee | ||
Comment 38•14 years ago
|
||
Comment on attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6
Pushed:
http://hg.mozilla.org/projects/devtools/rev/0d5421e23346
Attachment #525776 -
Attachment description: patch update 6 → [in-devtools] patch update 6
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0412][softblocker][console-1][checkin] → [patchclean:0412][softblocker][console-1][fixed-in-devtools]
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:0412][softblocker][console-1][fixed-in-devtools] → [patchclean:0412][softblocker][console-1][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
Comment 39•14 years ago
|
||
Comment on attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6
http://hg.mozilla.org/mozilla-central/rev/0d5421e23346
Attachment #525776 -
Attachment description: [in-devtools] patch update 6 → [checked-in][in-devtools] patch update 6
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•