Use a default allowConnection handler in dbg-server.js so that add-ons don't have to provide their own

RESOLVED FIXED in Firefox 18

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: past, Assigned: past)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

The intent of the change in bug 758696 was to allow debugger server embedders to supply their own user-acceptance handler. However, the way I implemented it requires *all* embedders to do so, even add-ons, like NetMonitor and the Profiler. It would be much easier to use the remote debugging protocol in other tools if the server specified its own default user-acceptance handler, while allowing it to be overridden by embedders at the same time.

In the same patch I'll fix the user-acceptance check to only occur on socket connections, not nsIPipe ones.
Posted patch Patch v1Splinter Review
This appears to work fine and doesn't regress b2g desktop. I'll try fennec as well. Rob, I copied the localization file under a new devtools directory, since that's what we do in browser/ as well.

Mark, since this is security sensitive code you may want to take a look, although there should be no user perceivable change.

Axel, do I need to do anything special when moving strings from browser/ to toolkit/? There are no identifier or content changes, just a location change.
Attachment #650552 - Flags: review?(rcampbell)
Attachment #650552 - Flags: feedback?(mgoodwin)
Attachment #650552 - Flags: feedback?(l10n)
Fennec is unaffected as well.
Comment on attachment 650552 [details] [diff] [review]
Patch v1

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

Thanks for asking, this looks good. Once this lands, it'd be great if you could do a quick post to mozilla.dev.l10n giving people a heads up that already translated these strings.
Attachment #650552 - Flags: feedback?(l10n) → feedback+
Thanks Axel, I'll make a note to post to mozilla.dev.l10n.
Comment on attachment 650552 [details] [diff] [review]
Patch v1

+/**
+ * Localization convenience methods.
+ */
+let L10N = {
+
+  /**
+   * L10N shortcut function.
+   *
+   * @param string aName
+   * @return string
+   */
+  getStr: function L10N_getStr(aName) {
+    return this.stringBundle.GetStringFromName(aName);
+  }
+};
+

you could just make this a straight-up function, but if you like having the object, you can keep it.

Ok, I think this change makes sense. Allowing actors to specify their own connection callback seems to make sense rather than foisting one onto them.
Attachment #650552 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> +/**
> + * Localization convenience methods.
> + */
> +let L10N = {
> +
> +  /**
> +   * L10N shortcut function.
> +   *
> +   * @param string aName
> +   * @return string
> +   */
> +  getStr: function L10N_getStr(aName) {
> +    return this.stringBundle.GetStringFromName(aName);
> +  }
> +};
> +
> 
> you could just make this a straight-up function, but if you like having the
> object, you can keep it.

I suspect that we may need to add a getFormatStr at some point in the future, like we do in debugger-controller.js:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1615
Comment on attachment 650552 [details] [diff] [review]
Patch v1

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

This looks good to me.
Attachment #650552 - Flags: feedback?(mgoodwin) → feedback+
Assignee: past → mh+mozilla
Assignee: mh+mozilla → past
Blocks: 751034
Depends on: 790952
Blocks: 790952
No longer depends on: 790952
https://hg.mozilla.org/mozilla-central/rev/22af72b981a0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.