867109 - tools reload not working correctly

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Girish Sharma [:Optimizer]]

My source code is in D:/fx-team directory.
I run the following command first :
>> tools srcdir d:/fx-team

It works as no error is shown. (But any success popup is also not shown)

Then when I try to run the command 
>> tools reload

It gives error

Module `main` is not found at file://d:fx-team\browser/devtools/main.js

even though that file exists and the source code is at fx-team HEAD.

Comment 1

[Girish Sharma [:Optimizer]]

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#59

and couple of lines above are directly adding strings. One windows, the variable devtoolsDir will have '\' in path. Concatenated string will end up being an incorrect uri. 

 OS.Path.join should be used instead.

Comment 2

[Girish Sharma [:Optimizer]]

Actually, devtoolsDir variable is created via OS.Path.join only, but on Windows, join results in \ as per the OS directory paths, while file:// uri have / so the concatenation gives wrong result in line 57-59

Comment 3

[Girish Sharma [:Optimizer]]

What is the suggested solution here ? I really want to use this loader magic.

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Uses nsiFIle and ioService to create file:// uri's. But due to a gcli bug, my present repo directory, D:\\fx-team gets converted to D:\x-team and thus it still does not work for my fx-team repo. I tried with D:\\mozilla-centra, but it gets converted to D:\mozilla-central which when used by OS.File, always return false as m is escaped. so basically, gcli should allow \\ to be present and not convert it to \ automatically.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

I have fixed the gcli issue of not being able to type X:\\xyz and made appropriate changes in the loader logic to handle native file locations and file:// locations properly. Still this does not work on Windows.

After setting the srcdir, all tools are blank, if the developer toolbar is closed, it will not reappear. All tool take around 30 seconds to appear (still blank).

Now I have no idea on what is going wrong.

Comment 6

[Dave Camp (:dcamp)]

Comment on attachment 748457 [details] [diff] [review]
patch v0.2

Review of attachment 748457 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/gDevTools.jsm
@@ +51,5 @@
>  
> +    // function to determine the file:// path from the OS path.
> +    try {
> +      let iOService = Cc["@mozilla.org/network/io-service;1"]
> +                        .getService(Ci.nsIIOService);

can we call that IOService or ioService?

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Dave Camp (:dcamp) from comment #6)
> Comment on attachment 748457 [details] [diff] [review]
> patch v0.2
> 
> Review of attachment 748457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +51,5 @@
> >  
> > +    // function to determine the file:// path from the OS path.
> > +    try {
> > +      let iOService = Cc["@mozilla.org/network/io-service;1"]
> > +                        .getService(Ci.nsIIOService);
> 
> can we call that IOService or ioService?

why? no iOS fan ? :P (okay!)

how can this get r+ ?
Its still not working.

Comment 8

[Dave Camp (:dcamp)]

Comment on attachment 748457 [details] [diff] [review]
patch v0.2

Review of attachment 748457 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, yeah, wrong flag - I am away from my windows computer this week, so I'm not sure how to solve.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: rebased patch v0.2

Rebased the patch based on latest fx-team. Some part was not needed (the menu entry part) as it is fixed by someone else already.

Comment 10

[Dave Camp (:dcamp)]

Attachment: new version

This version works for me, but depends on a few patches, will set up dependencies...

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 755074 [details] [diff] [review]
new version

Review of attachment 755074 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/main.js
@@ +101,4 @@
>    modifiers: osString == "Darwin" ? "accel,alt" : "accel,shift",
>    icon: "chrome://browser/skin/devtools/tool-inspector.png",
>    url: "chrome://browser/content/devtools/inspector/inspector.xul",
> +  label: "NEW INSPECTOR LABEL", //l10n("inspector.label", inspectorStrings),

This is a test, right?

Comment 12

[Girish Sharma [:Optimizer]]

Okay, now it works :) Thanks Dave.

so the difference was made through line 726 of gDevTools.jsm : devtools.main("main"); ?

Comment 13

[Dave Camp (:dcamp)]

(In reply to Girish Sharma [:Optimizer] from comment #12)
> Okay, now it works :) Thanks Dave.
> 
> so the difference was made through line 726 of gDevTools.jsm :
> devtools.main("main"); ?

Yeah, that was a big one.

Comment 14

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/8b8102fdb5fd

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8b8102fdb5fd