Closed Bug 595223 Opened 9 years ago Closed 9 years ago

Keep track of file:// url 'loadGroups'

Categories

(DevTools :: General, defect, P1)

defect

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)

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.
Blocks: 529086
Assignee: nobody → ddahl
Blocks: devtools4b8
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.
Assignee: ddahl → mihai.sucan
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?
(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)
(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!
Attached patch proposed patch and test code (obsolete) — Splinter Review
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)
Status: NEW → ASSIGNED
Depends on: 594523
Whiteboard: [patchclean:0928]
Attached patch rebased patch (obsolete) — Splinter Review
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)
Whiteboard: [patchclean:0928] → [patchclean:1002]
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+
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)
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: --- → ?
blocking2.0: ? → final+
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)
mass change: filter on PRIORITYSETTING
Priority: -- → P1
mass change: filter on PRIORITYSETTING
Blocks: devtools4
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker
ping. what's going to happen with this bug?
Will have to (strongly) rebase this patch on top of bug 611789, if I understood Gavin's plans correctly.
Severity: blocker → normal
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.
(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...
Will this bug allow the loading of files to be hijacked -- to implement a new file type -- or only logged?
(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?).
(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.
Mihai: now that bug 611789 has landed, do we just need to rebase this one?
Whiteboard: [patchclean:1002]
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.
Attached patch patch update 3 (obsolete) — Splinter Review
Rebased the patch.
Attachment #480420 - Attachment is obsolete: true
Attachment #502056 - Flags: review?(gavin.sharp)
Whiteboard: [patchclean:0107]
Whiteboard: [patchclean:0107] → [patchclean:0107][softblocker]
** 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+
Whiteboard: [patchclean:0107][softblocker] → [patchclean:0107][softblocker] [console-1]
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)
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?
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!
Attached patch [backed-out] patch update 4 (obsolete) — Splinter Review
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)
Hardware: x86 → All
Whiteboard: [patchclean:0107][softblocker] [console-1] → [patchclean:0401][softblocker] [console-1]
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+
Attachment #523638 - Flags: review?(dolske) → review+
Thank you Dave and Justin for the review pluses!
Whiteboard: [patchclean:0401][softblocker] [console-1] → [patchclean:0401][softblocker] [console-1] [checkin]
Whiteboard: [patchclean:0401][softblocker] [console-1] [checkin] → [patchclean:0401][softblocker] [console-1] [in-devtools][merge-m-c]
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
Whiteboard: [patchclean:0401][softblocker] [console-1] [in-devtools][merge-m-c] → [patchclean:0401][softblocker][console-1]
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
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.
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.
Attached patch patch update 5 (obsolete) — Splinter Review
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
Whiteboard: [patchclean:0401][softblocker][console-1] → [patchclean:0412][softblocker][console-1]
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
Results are green!
Whiteboard: [patchclean:0412][softblocker][console-1] → [patchclean:0412][softblocker][console-1][checkin]
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
Whiteboard: [patchclean:0412][softblocker][console-1][checkin] → [patchclean:0412][softblocker][console-1][fixed-in-devtools]
Status: ASSIGNED → RESOLVED
Closed: 9 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 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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.