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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

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

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

try=green

Comment 3

[Joe Walker [: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", {});

Comment 4

[Victor Porof [:vporof][:vp]]

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

Comment 5

[Joe Walker [:jwalker]]

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

Comment 6

[Heather Arthur [:harth]]

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

Good looks. Patch looks good.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v2

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.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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