Closed
Bug 845822
Opened 12 years ago
Closed 12 years ago
Inplace editor to be moved to devtools/shared/InplaceEditor.jsm
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: miker, Assigned: miker)
Details
Attachments
(1 file, 1 obsolete file)
70.34 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Whiteboard: [has-patch]
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
Attachment #722795 -
Flags: review?(jwalker)
Assignee | ||
Comment 2•12 years ago
|
||
try=green
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", {});
Attachment #722795 -
Flags: review?(jwalker)
Attachment #722795 -
Flags: review?(harthur)
Attachment #722795 -
Flags: review+
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.
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.
Updated•12 years ago
|
Attachment #722795 -
Flags: review?(harthur) → review?(fayearthur)
Comment 6•12 years ago
|
||
Comment on attachment 722795 [details] [diff] [review] Patch Good looks. Patch looks good.
Attachment #722795 -
Flags: review?(fayearthur) → review+
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.
Attachment #722795 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Whiteboard: [has-patch] → [land-in-fx-team]
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/672608b227c3
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/672608b227c3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Updated•11 years ago
|
No longer blocks: DevToolsPaperCuts
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•