Keep track of file:// url 'loadGroups'

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ddahl, Assigned: msucan)

Tracking

Trunk
Firefox 6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Whiteboard: [patchclean:0412][softblocker][console-1][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 529086

Updated

7 years ago
Assignee: nobody → ddahl
Blocks: 594225
(Reporter)

Comment 1

7 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

7 years ago
Assignee: ddahl → mihai.sucan
(Assignee)

Comment 2

7 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

7 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

7 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

7 years ago
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!
Attachment #479124 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Depends on: 594523
Whiteboard: [patchclean:0928]
(Assignee)

Comment 6

7 years ago
Created attachment 480420 [details] [diff] [review]
rebased patch

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

7 years ago
Whiteboard: [patchclean:0928] → [patchclean:1002]
(Reporter)

Comment 7

7 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

7 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

7 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: --- → ?
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)

Comment 11

7 years ago
mass change: filter on PRIORITYSETTING
Priority: -- → P1

Comment 12

7 years ago
mass change: filter on PRIORITYSETTING
Blocks: 593956

Comment 13

7 years ago
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker

Comment 14

7 years ago
ping. what's going to happen with this bug?
(Assignee)

Comment 15

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

Updated

7 years ago
Severity: blocker → normal

Comment 16

6 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

6 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

6 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

6 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

6 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

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

Updated

6 years ago
Whiteboard: [patchclean:1002]
(Assignee)

Comment 22

6 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

6 years ago
Created attachment 502056 [details] [diff] [review]
patch update 3

Rebased the patch.
Attachment #480420 - Attachment is obsolete: true
Attachment #502056 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
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+
(Reporter)

Updated

6 years ago
Whiteboard: [patchclean:0107][softblocker] → [patchclean:0107][softblocker] [console-1]
(Assignee)

Comment 25

6 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

6 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

6 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

6 years ago
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!
Attachment #502056 - Attachment is obsolete: true
Attachment #523638 - Flags: review?(dcamp)
Attachment #502056 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Hardware: x86 → All
Whiteboard: [patchclean:0107][softblocker] [console-1] → [patchclean:0401][softblocker] [console-1]

Comment 29

6 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+
Attachment #523638 - Flags: review?(dolske) → review+
(Assignee)

Comment 30

6 years ago
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
(Assignee)

Comment 33

6 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

6 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

6 years ago
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.
Attachment #523638 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [patchclean:0401][softblocker][console-1] → [patchclean:0412][softblocker][console-1]
(Assignee)

Comment 36

6 years ago
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
Attachment #525419 - Attachment is obsolete: true
(Assignee)

Comment 37

6 years ago
Results are green!
Whiteboard: [patchclean:0412][softblocker][console-1] → [patchclean:0412][softblocker][console-1][checkin]
(Assignee)

Comment 38

6 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

6 years ago
Whiteboard: [patchclean:0412][softblocker][console-1][checkin] → [patchclean:0412][softblocker][console-1][fixed-in-devtools]
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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
You need to log in before you can comment on or make changes to this bug.