Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Description • 12 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 1 • 12 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 12 years ago

try=green

	Joe Walker [:jwalker]
	Comment 3 • 12 years ago

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", {});

	Victor Porof [:vporof][:vp]
	Comment 4 • 12 years ago

(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.

	Joe Walker [:jwalker]
	Comment 5 • 12 years ago

(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.

	Heather Arthur [:harth]
	Comment 6 • 12 years ago

Comment on attachment 722795 [details] [diff] [review]
Patch

Good looks. Patch looks good.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 7 • 12 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 8 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/672608b227c3

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/672608b227c3