Closed Bug 802231 Opened 9 years ago Closed 9 years ago

Create a 'Target' API to allow us to agree on a thing being debugged

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: jwalker, Unassigned)

References

Details

Attachments

(1 file)

v0.0001
6.37 KB, patch
Details | Diff | Splinter Review 
My plan is to extract the API that we're currently creating in the developer tools window, and land it separately, so we can all use the same code.
righteous suggestion!

I think we'll ultimately want to have this as the single-point of contact to the debugger server. The dialog should probably deal with the initial connection and return either an error object or a connection object for the tool to use.

It could also be a container for dealing with various connection errors over the lifetime of the client/server connection.

These things are likely follow-ups to the initial connection dialog but wanted to get the ideas down.
Related bug: bug 800442
Joe: that looks good, but the patch doesn't show how I can get the actual tab from a given Target object. Is that planned or you have something else in mind?
Quick intro: Most of the code that we are creating for the developer tools window is stuff that sits around existing code. The exception is the concept of a 'target' so the window can say debug/inspect this other thing now.

I'd like to land this early so we can agree on what it looks like and what it does.

The actual integration will be done in the developer tools window.
(In reply to Mihai Sucan [:msucan] from comment #4)
> Joe: that looks good, but the patch doesn't show how I can get the actual
> tab from a given Target object. Is that planned or you have something else
> in mind?

You are right, the _tab property should be spelt tab.
That works for me, then.
Comment on attachment 672313 [details] [diff] [review]
v0.0001

+/**
+ * Construct a Target from a local XUL tab
+ */
+Target.newFromTab = function(tab) {
+  let target = Object.create(Target.prototype);
+  new EventEmitter(this);

should the Target object have quick access to things like the tab's docshell and contentWindow or is that burden going to be foisted onto the consumer?

+/**
+ * The listing counterpart to Target.newFromTab which gets an array of Targets
+ * for all available local web pages.
+ */
+Target.getLocalWebPages = function() {
+  let tabs = FixmeLocalThing.getTabs();
+  return tabs.map(function(tab) {

why not "getTabs"? Mixing nomenclature.

+/**
+ * Construct a Target from a local chrome Window
+ */
+Target.newFromChromeWindow = function(chromeWindow) {
+  let target = Object.create(Target.prototype);
+  new EventEmitter(this);

Are we going to be dealing with generic windows or can we assume these will always be Browsers?

newFromBrowser sounds nicer than newFromChromeWindow, but I guess if we end up debugging Thunderbird or $XULAPP we might have non browsery things.

...

+/**
+ * Construct a Target from a remote chrome window
+ */
+Target.newFromRemoteWebPage = function(connection, webPageName) {
+  let target = Object.create(Target.prototype);
+  new EventEmitter(this);

newFromRemoteTab?
(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> Comment on attachment 672313 [details] [diff] [review]
> v0.0001
> 
> +/**
> + * Construct a Target from a local XUL tab
> + */
> +Target.newFromTab = function(tab) {
> +  let target = Object.create(Target.prototype);
> +  new EventEmitter(this);
> 
> should the Target object have quick access to things like the tab's docshell
> and contentWindow or is that burden going to be foisted onto the consumer?

My initial gut reaction is that having many ways to do the same thing doesn't help users understand what's going on, and hurts refactoring, but we can perhaps see how this fares as we use the API.

> +/**
> + * The listing counterpart to Target.newFromTab which gets an array of
> Targets
> + * for all available local web pages.
> + */
> +Target.getLocalWebPages = function() {
> +  let tabs = FixmeLocalThing.getTabs();
> +  return tabs.map(function(tab) {
> 
> why not "getTabs"? Mixing nomenclature.

Yeah. Except that it's better to complain about newFromTab because getLocalWebPages fits the pattern of the other getXXX functions. Except that newFromTab makes sense given that we could concieve of a newFromWindow or newFromURL. I was trapped between 2 obvious naming conventions.

> +/**
> + * Construct a Target from a local chrome Window
> + */
> +Target.newFromChromeWindow = function(chromeWindow) {
> +  let target = Object.create(Target.prototype);
> +  new EventEmitter(this);
> 
> Are we going to be dealing with generic windows or can we assume these will
> always be Browsers?
> 
> newFromBrowser sounds nicer than newFromChromeWindow, but I guess if we end
> up debugging Thunderbird or $XULAPP we might have non browsery things.
> 
> ...
> 
> +/**
> + * Construct a Target from a remote chrome window
> + */
> +Target.newFromRemoteWebPage = function(connection, webPageName) {
> +  let target = Object.create(Target.prototype);
> +  new EventEmitter(this);
> 
> newFromRemoteTab?

But what if the thing we're debugging doesn't have tabs? And what if this makes people thing that we're debugging the tab (i.e. xul:tab) rather than the web-page content?

I don't have good answers. Just a string of bad ones :(

Can I suggest that we remember that we're not totally happy with the names that we've got and that we need some more experience with them?
Comment on attachment 672313 [details] [diff] [review]
v0.0001

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

::: browser/devtools/framework/Target.jsm
@@ +24,5 @@
> + * - close: The target window has been closed. All tools attached to this
> + *     target should close. This event is not currently cancelable.
> + * - navigate: The target window has navigated to a different URL
> + *     FIXME: What happens with HTML5 navigation events?
> + * - reload: The target window has been refreshed via F5 or a similar mechanism

Is there value in keeping these two separate, instead of having a single event with old and new URL arguments? I'm not sure about that, just wondering.

By HTML5 navigation events you mean pageshow/pagehide/pushstate/hashchanged, right? In the debugger we need the first two at least, and maybe even the others as well, but I haven't tested.

@@ +25,5 @@
> + *     target should close. This event is not currently cancelable.
> + * - navigate: The target window has navigated to a different URL
> + *     FIXME: What happens with HTML5 navigation events?
> + * - reload: The target window has been refreshed via F5 or a similar mechanism
> + * - change: One of the read-only

Premature EOL?

@@ +100,5 @@
> +
> +/**
> + * Construct a Target from a remote chrome window
> + */
> +Target.newFromRemoteWebPage = function(connection, webPageName) {

Isn't webPageName going to be the page URL (since document titles aren't that great as identifiers)? So maybe webPageUrl makes more sense?

@@ +119,5 @@
> +/**
> + * The listing counterpart to Target.newFromRemoteWebPage which gets an array of
> + * Targets for all available remote web pages.
> + */
> +Target.getRemoteWebPages = function(connectionOrHost, port) {

connectionOrHost seems to imply that the tool may have a cached connection object from a previous attempt, but I assume that would have come from another call to getRemoteWebPages which returns target objects. Connection is a private property of targets, so I think that we either need to remove the underscore or expect the tools to cache target objects.
So it's clear that I've not got the naming right. More discussion here:
https://github.com/joewalker/devtools-window/pull/72
We fixed that in the toolbox patch.

triage. Filter on PINKISBEAUTIFUL
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.