Closed Bug 1152721 Opened 5 years ago Closed 4 years ago

Move shared helper functions e.g. createChild() into a shared file

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: miker, Assigned: julian.descottes)

Details

Attachments

(1 file, 3 obsolete files)

From a review comment by bgrins:

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +1540,5 @@
> > + *        The tag name.
> > + * @param {object} aAttributes
> > + *        A set of attributes to set on the node.
> > + */
> > +function createChild(aParent, aTag, aAttributes={}) {
> 
> We should export this from a shared module since it's copy/pasted from the
> rule view.  Surprisingly, I don't think we have a shared file for this type
> of thing yet.  Maybe viewhelpers.jsm, or styleeditor/utils.js.  We can do
> that in a follow up though
>
Assigning this to me as follow up to Bug 1164343.
Assignee: nobody → julian.descottes
Attached patch Bug1152721.v1.patch (obsolete) — Splinter Review
This patch moves helper methods from rule-view and computed-view to a new styleinspector/utils.js

I left some helper methods related to rule-view#getNodeInfo, because they are too specific to rule-view to be shared at the moment. 

Also removed the custom setTimeout clearTimeout to rely on timer.jsm instead.

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=55810880ce57
Attachment #8639183 - Flags: review?(mratcliffe)
Attachment #8639183 - Flags: review?(mratcliffe) → review+
Thanks for the review ! 

Will rebase, do a new try push, and add checkin-needed if everything is fine.
Attached patch Bug1152721.v2.patch (obsolete) — Splinter Review
New patch after rebase. No changes otherwise. 

New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b8f28f54f8
Attachment #8639183 - Attachment is obsolete: true
Attachment #8643224 - Flags: review+
Try is green, adding checkin-needed.
Keywords: checkin-needed
sorry had to backout this change for causing xpcshell bustage https://treeherder.mozilla.org/logviewer.html#?job_id=4072767&repo=fx-team
Flags: needinfo?(julian.descottes)
Sorry, my mistake.

I thought unit tests would be executed by my try push, but looks like I got the syntax wrong.
Flags: needinfo?(julian.descottes)
Attached patch Bug1152721.v3.patch (obsolete) — Splinter Review
New push try, this time including xpcshell tests : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f378fe7ac5cb

@Mike : since the previous patch was busted I prefer to get your approval on this one before going forward.d
Attachment #8643224 - Attachment is obsolete: true
Attachment #8643916 - Flags: review?(mratcliffe)
Attachment #8643916 - Flags: review?(mratcliffe) → review+
Same as v3, just rebased. 

Try push (incl. xpcshell tests) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d8d520cd86

Will add checkin-needed if try is green.
Attachment #8643916 - Attachment is obsolete: true
Attachment #8647194 - Flags: review+
Except for unrelated issues, try is green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa9db435c2b2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.