Closed Bug 845822 Opened 8 years ago Closed 8 years ago

Inplace editor to be moved to devtools/shared/InplaceEditor.jsm

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: miker, Assigned: miker)

Details

Attachments

(1 file, 1 obsolete file)

CssRuleView.jsm currently exports editableItem, _editableField and _getInplaceEditorForSpan.

This means that e.g. in MarkupView.jsm we Cu.import("resource:///modules/devtools/CssRuleView.jsm"); in order to be able to create editable fields. This does not make sense.

We should move editableItem, _editableField and _getInplaceEditorForSpan to devtools/shared/InplaceEditor.jsm in order to allow them to be included in a sensible way.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Whiteboard: [has-patch]
Attached patch Patch (obsolete) — Splinter Review
This looks big but it is not as big as it looks. Apart from extracting the editor mechanics to shared/InplaceEditor.jsm (almost an exact copy) I have made the following changes.

- Added basic usage instructions to top of jsm.
- Removed _ prefix from jsm's EXPORTED_SYMBOLS ... we don't want it to look like we are playing with some other object's privates.
- Get focusManager using services.jsm and moved it into a lazy getter.
- Removed editableField and inplaceEditor assignments from styleinspector tests.
  This is because waitForEditorFocus() in head.js needs inplaceEditor. Having a
  method in head.js that depends on an import from a test does not make sense.

Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=47143b3891b6
Attachment #722795 - Flags: review?(jwalker)
Comment on attachment 722795 [details] [diff] [review]
Patch

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

Sorry this has taken me a while, it looks OK to me, but I think Heather should be given a chance to comment.

::: browser/devtools/styleinspector/test/head.js
@@ +14,5 @@
>  Cu.import("resource:///modules/devtools/Target.jsm", tempScope);
>  let TargetFactory = tempScope.TargetFactory;
> +Cu.import("resource:///modules/devtools/InplaceEditor.jsm", tempScope);
> +let editableField = tempScope.editableField;
> +let inplaceEditor = tempScope.getInplaceEditorForSpan;

It probably makes sense to go with the existing style, but it feels hacky for everything to be throwing stuff into tempScope. The following does the same, but more cleanly.

let {
  editableField: editableField,
  inplaceEditor: inplaceEditor
} = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});
Attachment #722795 - Flags: review?(jwalker)
Attachment #722795 - Flags: review?(harthur)
Attachment #722795 - Flags: review+
(In reply to Joe Walker [:jwalker] from comment #3)
> 
> let {
>   editableField: editableField,
>   inplaceEditor: inplaceEditor
> } = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});

I believe 

let {
  editableField,
  inplaceEditor
} = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});

should also work if you don't need to assign the destructured properties to differently named variables.
(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Joe Walker [:jwalker] from comment #3)
> > 
> > let {
> >   editableField: editableField,
> >   inplaceEditor: inplaceEditor
> > } = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});
> 
> I believe 
> 
> let {
>   editableField,
>   inplaceEditor
> } = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});
> 
> should also work if you don't need to assign the destructured properties to
> differently named variables.

Oh, nice. Thanks.
Attachment #722795 - Flags: review?(harthur) → review?(fayearthur)
Comment on attachment 722795 [details] [diff] [review]
Patch

Good looks. Patch looks good.
Attachment #722795 - Flags: review?(fayearthur) → review+
Attached patch Patch v2Splinter Review
Addressed Joe's nit ... we actually needed:
let {
  editableField,
  getInplaceEditorForSpan: inplaceEditor
} = Cu.import("resource:///modules/devtools/InplaceEditor.jsm", {});

But I agree that it is much tidier.
Attachment #722795 - Attachment is obsolete: true
Whiteboard: [has-patch] → [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/672608b227c3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.