Closed
Bug 990074
Opened 10 years ago
Closed 10 years ago
Sources linked via the optionsURL in install.rdf do not show up in addon debugger.
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: Optimizer, Assigned: mossop)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
24.89 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
For ex. this addon : https://addons.mozilla.org/en-US/firefox/addon/user-style-manager/ has an options window, and thus a js file linked with that xul window. The addon installs the options window using install.rdf's optionsURL property. this options.js file is not listed in Addon Debugger.
Assignee | ||
Comment 1•10 years ago
|
||
We need to add DOM windows created from add-on URLs as debuggees. I'm not sure if the best way to do that is in the onNewGlobal event or if we should use a chrome-document-global-created observer. The latter I know will have the location set so we can tie it to add-ons. The former might have through aGlobal.unsafeDereference.location.
Assignee | ||
Comment 2•10 years ago
|
||
Hrm except that chrome documents load about:blank first so in both cases all we get is that. We'll have to use a load listener to spot anything more useful which means we could miss scripts.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dtownsend+bugmail
Assignee | ||
Comment 3•10 years ago
|
||
This listens for new opened windows as long as the AddonThreadActor is alive. Any that map to the add-on get added as debuggees. The notification is necessary because checkGlobal is called very early in the window creation, at a time when it's location is still about:blank for chrome windows. The notification should be sent before any scripts have run though. This depends on bug 990685.
Attachment #8404283 -
Flags: review?(nfitzgerald)
Comment 4•10 years ago
|
||
Comment on attachment 8404283 [details] [diff] [review] patch Review of attachment 8404283 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, just some comments below. ::: browser/devtools/debugger/test/head.js @@ +119,5 @@ > onInstallEnded: function(aAddon, aAddonInstall) { > aInstaller.removeListener(listener); > + executeSoon(function() { > + deferred.resolve(aAddonInstall); > + }); :( Why is this necessary? It shouldn't be necessart after bug 940537, right? If so, add a TODO comment that links the bug number. ::: toolkit/devtools/server/actors/script.js @@ +4777,5 @@ > // A constant prefix that will be used to form the actor ID by the server. > actorPrefix: "addonThread", > > + onAttach: function(aRequest) { > + if (!this.attached) Nit: Brackets. Here and throughout. @@ +4788,5 @@ > + Services.obs.removeObserver(this, "document-element-inserted"); > + return ThreadActor.prototype.disconnect.call(this); > + }, > + > + observe: function(aSubject, aTopic, aData) { Nit: comment above this function explaining what it is observing and what it does when the observer fires, etc. @@ +4871,1 @@ > if (metadata) Nit from another time: brackets here please. @@ +4888,2 @@ > try { > + var uri = Services.io.newURI(uridescriptor.value, null, null); Rather than using |var|, can we just add |let uri;| at a high enough scope that it is visible to both scopes that use it? That looks like right before the try block to me.
Attachment #8404283 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #4) > Comment on attachment 8404283 [details] [diff] [review] > patch > > Review of attachment 8404283 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM, just some comments below. > > ::: browser/devtools/debugger/test/head.js > @@ +119,5 @@ > > onInstallEnded: function(aAddon, aAddonInstall) { > > aInstaller.removeListener(listener); > > + executeSoon(function() { > > + deferred.resolve(aAddonInstall); > > + }); > > :( Why is this necessary? > > It shouldn't be necessart after bug 940537, right? If so, add a TODO comment > that links the bug number. I've added a comment but this is bug 997408. If bug 940537 causes promise callbacks to be called asynchronously then I guess that will fix this but I think it makes some sense to still use this so we don't even resolve the promise until what we're waiting for has actually happened.
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e666661da03c
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e666661da03c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Assignee | ||
Comment 8•10 years ago
|
||
Backed out: https://hg.mozilla.org/integration/fx-team/rev/a9cddc54b6f4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•10 years ago
|
||
I could have sworn that when I tested chrome-document-global-created before it didn't have the url set right but here we are...
Attachment #8404283 -
Attachment is obsolete: true
Attachment #8412144 -
Flags: review?(nfitzgerald)
Comment 10•10 years ago
|
||
Comment on attachment 8412144 [details] [diff] [review] patch Review of attachment 8412144 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/script.js @@ +4796,5 @@ > + }, > + > + /** > + * Called when a new DOM document global is created. Check if the DOM was > + * laoded from an add-on and if so make the window a debuggee. laoded
Attachment #8412144 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e063eb25827a
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e063eb25827a
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 13•10 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #9) > > I could have sworn that when I tested chrome-document-global-created before > it didn't have the url set right but here we are... This behavior is not guaranteed! For windows like Options or browser.xul, window.location will be empty or about:blank.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Jeferson Hultmann from comment #13) > (In reply to Dave Townsend [:mossop] from comment #9) > > > > I could have sworn that when I tested chrome-document-global-created before > > it didn't have the url set right but here we are... > > > This behavior is not guaranteed! For windows like Options or browser.xul, > window.location will be empty or about:blank. What makes you say that and why is this the case (it wasn't in my tests)?
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•