595223 - Keep track of file:// url 'loadGroups'

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

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]

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]]

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]

(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]]

(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]]

Attachment: 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]]

Attachment: rebased patch

Rebased patch on top of the changes from bug 594523.

Comment 7

[David Dahl :ddahl]

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]]

Comment on attachment 480420 [details] [diff] [review]
rebased patch

Thanks David for the feedback+!

Asking for review from Gavin.

Comment 9

[Kevin Dangoor]

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: [e-mail]]]

Comment on attachment 480420 [details] [diff] [review]
rebased patch

Cancelling this request pending a new plan for bug 594523

Comment 11

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 12

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 13

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 14

[Kevin Dangoor]

ping. what's going to happen with this bug?

Comment 15

[Mihai Sucan [:msucan]]

Will have to (strongly) rebase this patch on top of bug 611789, if I understood Gavin's plans correctly.

Comment 16

[Tim Berners-Lee]

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]

(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]

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]]

(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]

(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]

Mihai: now that bug 611789 has landed, do we just need to rebase this one?

Comment 22

[Mihai Sucan [:msucan]]

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]]

Attachment: patch update 3

Rebased the patch.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

** 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]]

Comment on attachment 502056 [details] [diff] [review]
patch update 3

Asking for review from Dave Camp. Thanks!

Comment 26

[Dave Camp (: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?

Comment 27

[Mihai Sucan [:msucan]]

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]]

Attachment: [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)]

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]]

Thank you Dave and Justin for the review pluses!

Comment 31

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 523638 [details] [diff] [review]
[backed-out] patch update 4

http://hg.mozilla.org/projects/devtools/rev/102a2b309cc3

Comment 32

[Rob Campbell [:rc] (:robcee)]

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

Comment 33

[Mihai Sucan [:msucan]]

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]]

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]]

Attachment: 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]]

Attachment: [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]]

Results are green!

Comment 38

[Mihai Sucan [:msucan]]

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)]

Comment on attachment 525776 [details] [diff] [review]
[checked-in][in-devtools] patch update 6

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