Closed
Bug 943510
Opened 11 years ago
Closed 10 years ago
Convert to Promise.jsm in the devtools framework
Categories
(DevTools :: Framework, defect, P2)
DevTools
Framework
Tracking
(firefox30 fixed, firefox31 fixed)
RESOLVED
FIXED
Firefox 31
People
(Reporter: bbenvie, Assigned: bbenvie)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 9 obsolete files)
27.15 KB,
patch
|
bbenvie
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
This patch converts devtools/toolbox to use Promise.jsm. It also fixes bugs that arose from this conversion in the inspector (but does not convert the inspector to Promise.jsm).
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1398502b0eaf
Assignee | ||
Comment 3•11 years ago
|
||
Fix the test failures. https://tbpl.mozilla.org/?tree=Try&rev=f639943af8a0
Attachment #8338695 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
I suspect this is a platform issue with focus. browser_toolbox_raise.js is such a fragile test. https://tbpl.mozilla.org/?tree=Try&rev=97a0cd333992
Assignee | ||
Comment 5•11 years ago
|
||
I cannot reproduce this locally, and it's apparently spotty on the platforms tested. =(
Assignee | ||
Comment 6•10 years ago
|
||
Rip out the changes to browser_toolbox_raise.js. Fix some new bugs introduced by the highlighter.
Attachment #8338908 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=df8b25e32f25
Attachment #8363359 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8363805 [details] [diff] [review] promise-framework.patch Try is looking good. * robcee - framework * pbrosset & bgrins - markupview, inspector, responsiveview, and styleinspector (you've both touched code/tests that are modified here so I wanted to make sure you both checked this over)
Attachment #8363805 -
Flags: review?(rcampbell)
Attachment #8363805 -
Flags: review?(pbrosset)
Attachment #8363805 -
Flags: review?(bgrinstead)
Comment 9•10 years ago
|
||
Comment on attachment 8363805 [details] [diff] [review] promise-framework.patch Review of attachment 8363805 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Just a couple of questions/comments related to the markup-view highlight events. ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js @@ +57,5 @@ > is(getHighlitNode(), doc.querySelector("h1"), "The highlighter highlights the right node"); > return promise.resolve(); > } > > function mouseLeaveMarkupView() { Can you explain why you decided to introduce an event for this test? Was it intermittently failing on try with your patch? @@ +59,5 @@ > } > > function mouseLeaveMarkupView() { > let deferred = promise.defer(); > + inspector.markup.once("highlight-hidden", deferred.resolve); If you decide to change the event name as my comment on markup-view.js suggests, you'll need to change it here too. ::: browser/devtools/markupview/markup-view.js @@ +200,5 @@ > if (toolbox.isRemoteHighlightable) { > + return toolbox.initInspector().then(() => { > + return toolbox.highlighter.hideBoxModel(); > + }).then(() => { > + this.emit("highlight-hidden"); Would make more sense to me if called "node-unhighlight" to be consistent with the other event, but up to you. @@ +208,3 @@ > // If not, no need to unhighlight as the older highlight method uses a > // setTimeout to hide itself > + return promise.resolve(); And, although it's only used in a test for now, I would also emit the event in this case. Again, just to make it consistent with the event emitted in |_showBoxModel| ::: browser/devtools/styleinspector/test/browser_bug946331_close_tooltip_on_new_selection.js @@ +29,4 @@ > inspector = aInspector; > ruleView = aRuleView; > + openView("computedview", aComputedView => { > + computedView = aComputedView; Thanks for these "openView" refactorings! Looking good!
Attachment #8363805 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Thanks for the review! (In reply to Patrick Brosset [:pbrosset] from comment #9) > ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js > @@ +57,5 @@ > > is(getHighlitNode(), doc.querySelector("h1"), "The highlighter highlights the right node"); > > return promise.resolve(); > > } > > > > function mouseLeaveMarkupView() { > > Can you explain why you decided to introduce an event for this test? Was it > intermittently failing on try with your patch? Yeah, it was actually a permanent failure [1]. The reason is that `toolbox.initInspector` now returns a Promise.jsm promise (which will resolve next tick at the soonest), so the executeSoon in mouseLeaveMarkupView is now too soon. [1] https://tbpl.mozilla.org/?tree=Try&rev=9f27f915e4ea > ::: browser/devtools/markupview/markup-view.js > @@ +200,5 @@ > > if (toolbox.isRemoteHighlightable) { > > + return toolbox.initInspector().then(() => { > > + return toolbox.highlighter.hideBoxModel(); > > + }).then(() => { > > + this.emit("highlight-hidden"); > > Would make more sense to me if called "node-unhighlight" to be consistent > with the other event, but up to you. Yeah, I didn't really like the name either. > @@ +208,3 @@ > > // If not, no need to unhighlight as the older highlight method uses a > > // setTimeout to hide itself > > + return promise.resolve(); > > And, although it's only used in a test for now, I would also emit the event > in this case. Again, just to make it consistent with the event emitted in > |_showBoxModel| Good point, I didn't think about that.
Comment 11•10 years ago
|
||
Comment on attachment 8363805 [details] [diff] [review] promise-framework.patch Review of attachment 8363805 [details] [diff] [review]: ----------------------------------------------------------------- framework changes look ok to me.
Attachment #8363805 -
Flags: review?(rcampbell) → review+
Comment 12•10 years ago
|
||
Comment on attachment 8363805 [details] [diff] [review] promise-framework.patch Review of attachment 8363805 [details] [diff] [review]: ----------------------------------------------------------------- I don't have anything to add beyond what Patrick has said
Attachment #8363805 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 13•10 years ago
|
||
Addresses Patrick's feedback. I noticed that closing the devtools after opening the style inspector causes some console spam; a last "hideBoxModel" message is trying to send after the connection is closed. Fortunately this is reproducible.
Attachment #8363805 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Rebase, since the highlighter stuff moved to framework.js. I'm still seeing the issue where hideBoxModel happens after the connection is already closing. This causes windows to leak until shutdown in styleinspector/test/browser_ruleview_original_source_link.js.
Attachment #8365263 -
Attachment is obsolete: true
Attachment #8382565 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Fixes the failures locally. https://tbpl.mozilla.org/?tree=Try&rev=81989aff7562
Attachment #8382565 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
(In reply to Brandon Benvie [:benvie] from comment #14) > Created attachment 8382565 [details] [diff] [review] > promise-framework.patch > > Rebase, since the highlighter stuff moved to framework.js. I'm still seeing > the issue where hideBoxModel happens after the connection is already > closing. This causes windows to leak until shutdown in > styleinspector/test/browser_ruleview_original_source_link.js. Can you try removing: inspector.selection.setNode(div); and instead adding: inspector.selection.setNode(div, "test"); The inspector selection triggers all sorts of server-side requests when it's assigned a new node. One of them is the highlighter which will briefly draw the box-model outline so the user can see what's the new selection. Now, passing "test" as a second argument (the reason) will tell it not to highlight, and therefore the showBoxModel and hideBoxModel methods won't be called. Except when actually testing the highlighter, we should always pass this "test" parameter.
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16) > (In reply to Brandon Benvie [:benvie] from comment #14) > > Created attachment 8382565 [details] [diff] [review] > > promise-framework.patch > > > > Rebase, since the highlighter stuff moved to framework.js. I'm still seeing > > the issue where hideBoxModel happens after the connection is already > > closing. This causes windows to leak until shutdown in > > styleinspector/test/browser_ruleview_original_source_link.js. > > Can you try removing: > inspector.selection.setNode(div); > and instead adding: > inspector.selection.setNode(div, "test"); > > The inspector selection triggers all sorts of server-side requests when it's > assigned a new node. > One of them is the highlighter which will briefly draw the box-model outline > so the user can see what's the new selection. Now, passing "test" as a > second argument (the reason) will tell it not to highlight, and therefore > the showBoxModel and hideBoxModel methods won't be called. > > Except when actually testing the highlighter, we should always pass this > "test" parameter. I've made the change but that didn't fix the issue (when I removed the actual fix). The fix I made was changing ToolboxHighlighterUtils#unhilight to not use initInspector, rather just check for the existence of this.toolbox.hightlighter. Prior to the fix (i.e. as it currently is on m-c) we have to initialize the inspector just to hide the box model. This is unnecessary because we can assume if the highlighter exists then the inspector is already initialized. It alsop introduces further complications during shutdown, because during shutdown we hide the box model, which causes the inspector to be shown. So in shutting down we attempt to initialize the inspector again, and this breaks when switching promise implementations due to it no longer being synchronous.
Assignee | ||
Comment 18•10 years ago
|
||
Rebase. One final try. https://tbpl.mozilla.org/?tree=Try&rev=5e09965cb2e9
Attachment #8382623 -
Attachment is obsolete: true
Attachment #8383831 -
Flags: review+
Comment 19•10 years ago
|
||
A simple tryserver run of the devtools framework patch looks green: https://tbpl.mozilla.org/?tree=Try&rev=1989d88f02ee I think this is ready to land, I'll go ahead unless there are reasons not to.
Assignee | ||
Comment 20•10 years ago
|
||
There's a lot of oranges between this one and the last one I ran and I'm not entirely sure if this patch is causing them or not.
Comment 21•10 years ago
|
||
(In reply to Brandon Benvie [:benvie] from comment #20) > There's a lot of oranges between this one and the last one I ran and I'm not > entirely sure if this patch is causing them or not. I'm not sure I understand what you're saying, the last run seems green except for known intermittent oranges. It is based on a recent mozilla-central revision with no other patches applied.
Assignee | ||
Comment 22•10 years ago
|
||
Ah, nevermind then. This is good to land then.
Comment 23•10 years ago
|
||
Actually, this conflicted with another recent m-c landing in a non-obvious way. Do you think you can take a look? I guess the patch will still work fine if adjusted in the correct way.
Assignee | ||
Comment 24•10 years ago
|
||
Rebase. https://tbpl.mozilla.org/?tree=Try&rev=b8244a054cb6
Attachment #8383831 -
Attachment is obsolete: true
Attachment #8392336 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8392336 -
Attachment is obsolete: true
Attachment #8392340 -
Flags: review+
Assignee | ||
Comment 26•10 years ago
|
||
Let's try that again... https://tbpl.mozilla.org/?tree=Try&rev=2b9c3b333716
https://hg.mozilla.org/mozilla-central/rev/ff438bc0b151
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment 29•10 years ago
|
||
Brandon, when you get a minute, can you please request Aurora uplift on this patch? Thanks :)
Flags: needinfo?(bbenvie)
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8392340 [details] [diff] [review] promise-framework.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Switch from deprecated addon-sdk promises to Promise.jsm User impact if declined: an epic uplift patch series by by RyanVM depends on this patch Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): none, it is standalone String or IDL/UUID changes made by this patch:
Attachment #8392340 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(bbenvie)
Updated•10 years ago
|
Attachment #8392340 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•