Last Comment Bug 595223 - Keep track of file:// url 'loadGroups'
: Keep track of file:// url 'loadGroups'
Status: RESOLVED FIXED
[patchclean:0412][softblocker][consol...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on: 594523
Blocks: 529086 devtools4 devtools4b8
  Show dependency treegraph
 
Reported: 2010-09-10 09:58 PDT by David Dahl :ddahl
Modified: 2011-05-09 12:12 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
proposed patch and test code (11.06 KB, patch)
2010-09-28 12:05 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
rebased patch (6.81 KB, patch)
2010-10-02 12:16 PDT, Mihai Sucan [:msucan]
bugzilla: feedback+
Details | Diff | Splinter Review
patch update 3 (11.43 KB, patch)
2011-01-07 11:50 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[backed-out] patch update 4 (11.48 KB, patch)
2011-04-01 11:23 PDT, Mihai Sucan [:msucan]
dcamp: review+
dolske: review+
Details | Diff | Splinter Review
patch update 5 (11.53 KB, patch)
2011-04-12 10:08 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[checked-in][in-devtools] patch update 6 (11.63 KB, patch)
2011-04-13 13:05 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2010-09-10 09:58:42 PDT
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.
Comment 1 David Dahl :ddahl 2010-09-10 18:17:16 PDT
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.
Comment 2 Mihai Sucan [:msucan] 2010-09-26 04:01:06 PDT
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?
Comment 3 David Dahl :ddahl 2010-09-26 04:14:49 PDT
(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)
Comment 4 Mihai Sucan [:msucan] 2010-09-26 04:28:46 PDT
(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!
Comment 5 Mihai Sucan [:msucan] 2010-09-28 12:05:12 PDT
Created attachment 479124 [details] [diff] [review]
proposed patch and test code

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!
Comment 6 Mihai Sucan [:msucan] 2010-10-02 12:16:24 PDT
Created attachment 480420 [details] [diff] [review]
rebased patch

Rebased patch on top of the changes from bug 594523.
Comment 7 David Dahl :ddahl 2010-10-06 14:23:00 PDT
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.
Comment 8 Mihai Sucan [:msucan] 2010-10-07 01:40:03 PDT
Comment on attachment 480420 [details] [diff] [review]
rebased patch

Thanks David for the feedback+!

Asking for review from Gavin.
Comment 9 Kevin Dangoor 2010-10-13 07:18:03 PDT
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.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-16 18:48:19 PST
Comment on attachment 480420 [details] [diff] [review]
rebased patch

Cancelling this request pending a new plan for bug 594523
Comment 11 Kevin Dangoor 2010-11-22 11:14:28 PST
mass change: filter on PRIORITYSETTING
Comment 12 Kevin Dangoor 2010-11-22 11:32:10 PST
mass change: filter on PRIORITYSETTING
Comment 13 Kevin Dangoor 2010-11-23 06:46:41 PST
mass change: filter mail on BLOCKERSETTING
Comment 14 Kevin Dangoor 2010-12-01 07:35:07 PST
ping. what's going to happen with this bug?
Comment 15 Mihai Sucan [:msucan] 2010-12-01 12:28:18 PST
Will have to (strongly) rebase this patch on top of bug 611789, if I understood Gavin's plans correctly.
Comment 16 Tim Berners-Lee 2011-01-03 08:02:10 PST
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.
Comment 17 David Dahl :ddahl 2011-01-03 09:55:58 PST
(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 Tim Berners-Lee 2011-01-03 11:17:10 PST
Will this bug allow the loading of files to be hijacked -- to implement a new file type -- or only logged?
Comment 19 Mihai Sucan [:msucan] 2011-01-03 11:21:10 PST
(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?).
Comment 20 David Dahl :ddahl 2011-01-03 11:26:38 PST
(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 Kevin Dangoor 2011-01-06 12:37:41 PST
Mihai: now that bug 611789 has landed, do we just need to rebase this one?
Comment 22 Mihai Sucan [:msucan] 2011-01-07 10:28:31 PST
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.
Comment 23 Mihai Sucan [:msucan] 2011-01-07 11:50:02 PST
Created attachment 502056 [details] [diff] [review]
patch update 3

Rebased the patch.
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-03 07:21:52 PST
** 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
Comment 25 Mihai Sucan [:msucan] 2011-03-31 09:38:27 PDT
Comment on attachment 502056 [details] [diff] [review]
patch update 3

Asking for review from Dave Camp. Thanks!
Comment 26 Dave Camp (:dcamp) 2011-03-31 19:27:36 PDT
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?
Comment 27 Mihai Sucan [:msucan] 2011-04-01 08:47:25 PDT
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!
Comment 28 Mihai Sucan [:msucan] 2011-04-01 11:23:01 PDT
Created attachment 523638 [details] [diff] [review]
[backed-out] patch update 4

Updated the patch as requested. Tests pass in opt and debug builds, locally.

Please let me know if further changes are needed. Thanks!
Comment 29 Dave Camp (:dcamp) 2011-04-01 11:35:43 PDT
Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4

Looks good to me, handing over to dolske for a binding r+.
Comment 30 Mihai Sucan [:msucan] 2011-04-10 06:30:58 PDT
Thank you Dave and Justin for the review pluses!
Comment 31 Rob Campbell [:rc] (:robcee) 2011-04-10 08:15:58 PDT
Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4

http://hg.mozilla.org/projects/devtools/rev/102a2b309cc3
Comment 33 Mihai Sucan [:msucan] 2011-04-11 01:43:01 PDT
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.
Comment 34 Mihai Sucan [:msucan] 2011-04-12 08:45:28 PDT
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.
Comment 35 Mihai Sucan [:msucan] 2011-04-12 10:08:46 PDT
Created attachment 525419 [details] [diff] [review]
patch update 5

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.
Comment 36 Mihai Sucan [:msucan] 2011-04-13 13:05:50 PDT
Created attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6

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
Comment 37 Mihai Sucan [:msucan] 2011-04-14 12:08:11 PDT
Results are green!
Comment 38 Mihai Sucan [:msucan] 2011-04-15 08:34:17 PDT
Comment on attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6

Pushed:
http://hg.mozilla.org/projects/devtools/rev/0d5421e23346
Comment 39 Rob Campbell [:rc] (:robcee) 2011-05-09 12:12:00 PDT
Comment on attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6

http://hg.mozilla.org/mozilla-central/rev/0d5421e23346

Note You need to log in before you can comment on or make changes to this bug.