Closed
Bug 1014547
Opened 11 years ago
Closed 10 years ago
[highlighter] CSS transform highlighter
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(relnote-firefox 33+)
RESOLVED
FIXED
Firefox 33
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 33+ |
People
(Reporter: pbro, Assigned: pbro)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 30 obsolete files)
170.54 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
As part of bug 942176, implement a CSS transform highlighter which shows both the non-transformed box and the transformed box.
Just like the CSS transform preview tooltip, but on the content.
Assignee | ||
Comment 1•11 years ago
|
||
Part 1 - Completely remove the css transform preview tooltip and associated tests
Assignee: nobody → pbrosset
Assignee | ||
Comment 2•11 years ago
|
||
Part 2 - Moves the highlighterutils API to a separate module as it's going to grow and it totally should live in a separate file.
I took this opportunity to clean up the code and simplify it quite a bit:
- functions that need the inspector to be init don't have to do it themselves anymore
- all async functions are now flat \o/ thanks to Task.async
- there's no need to test whether the highlighter actor exists or not anymore since FF29 is the current release.
Assignee | ||
Comment 3•11 years ago
|
||
part 3 - Make the highligther API better.
The highlighter actor which was created some time ago was assuming there would be only one type of highlighter and only one instance of highlighter at a time.
We now know these assumptions do not hold as we want to display multiple highlighters in parallel (see bug 971662) or display different kinds of highlighter (this bug).
This patch makes sure the inspector's getHighlighter method still exists and return the same object as before, so that backward compatibility isn't going to be an issue. It also adds a new inspector method: getHighlighterByType which I will use in the next patch to get a (to be created) CssTransformHighlighter.
Assignee | ||
Comment 4•11 years ago
|
||
Part 3 - v2 - Works better. Actually building and testing sometimes help.
Attachment #8427055 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
Forgot some local changes.
Attachment #8427223 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
part 4 - Actual implementation of the css transform highlighter.
It makes use of the new API done in part 3.
Using it, the rule-view can get an instance of the highlighter and show/hide it on hover of a 'transform' property.
I'm still to make it work for the computed-view too.
It should be more or less a copy/paste.
Having said this, I'm getting a bit frustrated with the way we added "decoration" interactions on the rule/computed-view: the tooltips and now the highlighter.
I think I may work on a part 5 patch to change this. Maybe find a way to extract everything that has nothing to do with read/write of the properties into a separate style-inspector module, dedicated to enriching the views with tooltips, highlighters and what not.
Video teaser: http://quick.as/L469IDD7
Assignee | ||
Comment 7•11 years ago
|
||
part 4 v2 - Fixed a few bugs and made it work in the computed-view too.
Next up: tests
Attachment #8427740 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8427012 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•11 years ago
|
Attachment #8427013 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•11 years ago
|
Attachment #8427225 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 8•11 years ago
|
||
Now with tests (separate tests for the UI and for the actors).
Attachment #8427812 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
part 2 - v2
This v2 fixes some try build failures that appeared after moving and refactoring the highlighterUtils API.
Attachment #8427013 -
Attachment is obsolete: true
Attachment #8427013 -
Flags: review?(bgrinstead)
Attachment #8429901 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 10•11 years ago
|
||
part 3 - v4
Rebased after part 2 changes
Attachment #8427225 -
Attachment is obsolete: true
Attachment #8427225 -
Flags: review?(bgrinstead)
Attachment #8429905 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 11•11 years ago
|
||
part 4 - v4
Fixed a failing test on try.
New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=811ffdd21d30
Attachment #8429139 -
Attachment is obsolete: true
Attachment #8429914 -
Flags: review?(bgrinstead)
Comment 12•11 years ago
|
||
Comment on attachment 8427012 [details] [diff] [review]
bug1014547-transform-highlighter-1-remove_tooltip v1.patch
Review of attachment 8427012 [details] [diff] [review]:
-----------------------------------------------------------------
Please rebase this:
1 out of 2 hunks FAILED -- saving rejects to file browser/devtools/shared/widgets/Tooltip.js.rej
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/styleinspector/computed-view.js.rej
Attachment #8427012 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 13•11 years ago
|
||
rebased
Attachment #8427012 -
Attachment is obsolete: true
Attachment #8430239 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 14•11 years ago
|
||
rebased
Attachment #8429901 -
Attachment is obsolete: true
Attachment #8429901 -
Flags: review?(bgrinstead)
Attachment #8430240 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 15•11 years ago
|
||
rebased
Attachment #8429905 -
Attachment is obsolete: true
Attachment #8429905 -
Flags: review?(bgrinstead)
Attachment #8430241 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 16•11 years ago
|
||
rebased.
Also, the try build I started earlier today shows an intermittent failure in one of the tests I added. I'll work on this next.
Other than this and modulo review comments, this is pretty much ready.
Attachment #8429914 -
Attachment is obsolete: true
Attachment #8429914 -
Flags: review?(bgrinstead)
Attachment #8430243 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 17•11 years ago
|
||
part 4, rebased again
Attachment #8430243 -
Attachment is obsolete: true
Attachment #8430243 -
Flags: review?(bgrinstead)
Attachment #8431425 -
Flags: review?(bgrinstead)
Comment 18•11 years ago
|
||
Comment on attachment 8430239 [details] [diff] [review]
bug1014547-transform-highlighter-1-remove_tooltip v2.patch
Review of attachment 8430239 [details] [diff] [review]:
-----------------------------------------------------------------
I'm OK with removing this, as the overlay is more useful. As we discussed earlier, I'd prefer that we just don't show the overlay on overridden or disabled transform properties to prevent breaking the semantics of the tooltip / overlays. Specifically, the expectation that when working with a property you are seeing a preview of *that* property (even if it isn't applied) vs the computed property (which the overlay is displaying).
Attachment #8430239 -
Flags: review?(bgrinstead) → review+
Comment 19•11 years ago
|
||
Comment on attachment 8430241 [details] [diff] [review]
bug1014547-transform-highlighter-3-revisit_highlighter_api v5.patch
Review of attachment 8430241 [details] [diff] [review]:
-----------------------------------------------------------------
Haven't finished reviewing the code on this yet, but I get an error when debugging an older server with this applied (simulator 1.4/1.5). We will need to make sure this has backwards compatibility, and ideally in a way that we can easily use in the future as new highlighter classes are added
TypeError: v is undefined
Stack trace:
types.addActorType/type<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:257:21
RetVal<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:496:5
Response<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:664:5
frontProto/</proto[name]/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1208:13
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1112:7
DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:863:9
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:905:1
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:456:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
::: toolkit/devtools/server/actors/highlighter.js
@@ +247,5 @@
> }
> })
> });
>
> +let MainHighlighterFront = protocol.FrontClass(MainHighlighterActor, {});
This front is never used (neither is CustomHighlighterFront) . Seems like the frontend works directly with the MainHighlighterActor through getHighlighter(). Is this intended? If so, we don't need to bother with the front.
Attachment #8430241 -
Flags: review?(bgrinstead) → review-
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8430241 [details] [diff] [review]
> bug1014547-transform-highlighter-3-revisit_highlighter_api v5.patch
>
> Review of attachment 8430241 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Haven't finished reviewing the code on this yet, but I get an error when
> debugging an older server with this applied (simulator 1.4/1.5). We will
> need to make sure this has backwards compatibility, and ideally in a way
> that we can easily use in the future as new highlighter classes are added
Hmm that's weird, I had made it so that it would work with older targets.
I will take a look again.
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +247,5 @@
> > }
> > })
> > });
> >
> > +let MainHighlighterFront = protocol.FrontClass(MainHighlighterActor, {});
>
> This front is never used (neither is CustomHighlighterFront) . Seems like
> the frontend works directly with the MainHighlighterActor through
> getHighlighter(). Is this intended? If so, we don't need to bother with
> the front.
The inspectorActor's getHighlighter indeed returns an instance of MainHighlighterActor, which then gets translated into a MainHighlighterFront by protocol.js, so the front-end does work with Fronts. And the thing is that, even if they're empty, they are required by protocol.js. Simply not defining fronts for actors results in errors.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> (In reply to Brian Grinstead [:bgrins] from comment #19)
> > Comment on attachment 8430241 [details] [diff] [review]
> > bug1014547-transform-highlighter-3-revisit_highlighter_api v5.patch
> >
> > Review of attachment 8430241 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Haven't finished reviewing the code on this yet, but I get an error when
> > debugging an older server with this applied (simulator 1.4/1.5). We will
> > need to make sure this has backwards compatibility, and ideally in a way
> > that we can easily use in the future as new highlighter classes are added
> Hmm that's weird, I had made it so that it would work with older targets.
> I will take a look again.
Having said this, I haven't actually tested it in a while :) It's very possible that I broke something.
Comment 22•11 years ago
|
||
Comment on attachment 8430240 [details] [diff] [review]
bug1014547-transform-highlighter-2-move_highlighter_utils v3.patch
Review of attachment 8430240 [details] [diff] [review]:
-----------------------------------------------------------------
I'm fine with moving this to its own file - just have a couple of notes
::: browser/devtools/shared/highlighter-utils.js
@@ +21,5 @@
> + */
> +
> +/**
> + * Get the highighterUtils instance for a given toolbox.
> + * This should be done once only by the toolbox itself and stored there so that
There is a comment indicating that this should not be used by anything but the toolbox, but this living in shared and having utils in the name is bound to cause confusion. I'd say move it into framework/ and maybe rename to the original toolbox-highlighter-utils.js
@@ +78,5 @@
> + }
> +
> + yield toolbox.selectTool("inspector");
> +
> + toolbox._pickerButton.setAttribute("checked", "true");
Should we be accessing the _prefixed variable on the toolbox here? Seems like there is an expectation that it wouldn't be used from outside of that object. Don't see any reason why it needs to be 'private' - could just un-underscore the variable name there. Or add a setter for toolbox.pickerButtonChecked.
Attachment #8430240 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 8431425 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v6.patch
Clearing R? flag as I'm working on addressing the feedback.
Attachment #8431425 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 24•11 years ago
|
||
Thanks for the review.
Here's a new part 2 with the following changes:
- moved the utils file to /framework and renamed it toolbox-highlighter-utils.js
- added a pickerButtonChecked setter to Toolbox to avoid accessing a private property in the utils
Attachment #8430240 -
Attachment is obsolete: true
Attachment #8432382 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 25•11 years ago
|
||
Changes done in part 3:
- rebased after changes to part 2
- fixed the backward compatibility with older targets:
- this was due to a typo I fixed in the InspectorActor (see bug 1014295)
- and also due to the fact that I renamed the HighlighterActor to MainHighlighterActor in an effort to differentiate it from the newly added CustomHighlighterActor. I reverted that change.
Attachment #8430241 -
Attachment is obsolete: true
Attachment #8432400 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 26•11 years ago
|
||
Part 4, v7. Ready for another round of review.
Here are the changes in this version:
- rebased after changes done in part 2 and 3
- the rule-view doesn't show the highlighter on overriden and disabled props (test added for this)
- cleaned up a little the isNodeValid function in highlighter.js and used it in XULBasedHighlighter in various places to avoid failing when nodes are dead when hiding the highlighter or detaching event listeners
- added a new trait to the root actor to avoid trying to show the highlighter on older targets
- removed the now unused highlighter trait
Attachment #8431425 -
Attachment is obsolete: true
Attachment #8432522 -
Flags: review?(bgrinstead)
Comment 27•11 years ago
|
||
Comment on attachment 8432382 [details] [diff] [review]
bug1014547-transform-highlighter-2-move_highlighter_utils v4.patch
Review of attachment 8432382 [details] [diff] [review]:
-----------------------------------------------------------------
Code changes look good - startPicker/stopPicker/highlightNodeFront are now much clearer
::: browser/devtools/framework/toolbox-highlighter-utils.js
@@ +85,5 @@
> + toolbox.walker.on("picker-node-picked", onPickerNodePicked);
> +
> + yield toolbox.highlighter.pick();
> +
> + isPicking = true;
Should we mark isPicking to true first thing before yielding for highlighter.pick() and toolbox.selectTool()? This way if startPicker was called twice in a row it would not bind multiple times to walker events and emit picker-started twice. This is how it is handled inside of stopPicker.
I know this is how it was done before in toolbox.js, just checking to see if this is a possible bug
@@ +127,5 @@
> + */
> + function onPickerNodePicked(data) {
> + toolbox.selection.setNodeFront(data.node, "picker-node-picked");
> + stopPicker();
> + };
Nit: no need for semicolon here
Attachment #8432382 -
Flags: review?(bgrinstead) → review+
Comment 28•11 years ago
|
||
I mentioned this to pbrosset earlier, but leaving this here for reference. I'm not sure exactly where in the patch sequence I've started seeing this, but I was seeing issues where the box model highlighter would not disappear after leaving a node in the markup view.
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 29•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #28)
> Created attachment 8432662 [details]
> box-model-sticky.gif
>
> I mentioned this to pbrosset earlier, but leaving this here for reference.
> I'm not sure exactly where in the patch sequence I've started seeing this,
> but I was seeing issues where the box model highlighter would not disappear
> after leaving a node in the markup view.
That would be part 4. I'm on it right now.
It's a race condition between the markup-view mouseleave that hides the highlighter and the mousemove on a markup-view node that shows it again. Since toggling is async (via protocol.js), there are cases where these can race.
In part 4, I refactored a little bit the server-side toggling logic, and I have probably introduced the bug then.
Assignee | ||
Comment 30•11 years ago
|
||
part 2, with changes as per review feedback.
Both points very valid, and fixed in the code. Thanks for the review.
Attachment #8432382 -
Attachment is obsolete: true
Attachment #8433215 -
Flags: review+
Assignee | ||
Comment 31•11 years ago
|
||
part 3 - rebased after changes to part 2
Attachment #8432400 -
Attachment is obsolete: true
Attachment #8432400 -
Flags: review?(bgrinstead)
Attachment #8433216 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 8432522 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v7.patch
Clearing the R? flag on part 4 again while working on the bug bgrins reported.
I found a fix for this.
I will post a new part 4 patch as soon as I'm done working on an issue I discovered while running try builds.
Attachment #8432522 -
Flags: review?(bgrinstead)
Comment 33•11 years ago
|
||
Comment on attachment 8432522 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v7.patch
Review of attachment 8432522 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/styleinspector/computed-view.js
@@ +187,5 @@
>
> this._buildContextMenu();
> this.createStyleViews();
> +
> + if (this.styleInspector.inspector.hasCustomHighlighters) {
This check works perfectly for now, because the css transform highlighter is being added exactly at the same time as the trait. What will the check be for supporting backwards compat when a new custom highlighter type is added to the server?
In that case the CustomHighlighterActor initializer will throw (typeName + " isn't a valid highlighter class")
Assignee | ||
Comment 34•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #33)
> Comment on attachment 8432522 [details] [diff] [review]
> bug1014547-transform-highlighter-4-implement_transform_highlighter v7.patch
>
> Review of attachment 8432522 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/devtools/styleinspector/computed-view.js
> @@ +187,5 @@
> >
> > this._buildContextMenu();
> > this.createStyleViews();
> > +
> > + if (this.styleInspector.inspector.hasCustomHighlighters) {
>
> This check works perfectly for now, because the css transform highlighter is
> being added exactly at the same time as the trait. What will the check be
> for supporting backwards compat when a new custom highlighter type is added
> to the server?
>
> In that case the CustomHighlighterActor initializer will throw (typeName + "
> isn't a valid highlighter class")
I can see 2 solutions:
- turn the hasCustomHighlighters trait into an array of custom highlighter type strings, so that the client-side can know which types do exist on the server
- make the InspectorActor's getHighlighterByType method return null when a type isn't known.
I have a preference for the latter.
Comment 35•11 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> (In reply to Brian Grinstead [:bgrins] from comment #33)
> > Comment on attachment 8432522 [details] [diff] [review]
> > bug1014547-transform-highlighter-4-implement_transform_highlighter v7.patch
> >
> > Review of attachment 8432522 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: browser/devtools/styleinspector/computed-view.js
> > @@ +187,5 @@
> > >
> > > this._buildContextMenu();
> > > this.createStyleViews();
> > > +
> > > + if (this.styleInspector.inspector.hasCustomHighlighters) {
> >
> > This check works perfectly for now, because the css transform highlighter is
> > being added exactly at the same time as the trait. What will the check be
> > for supporting backwards compat when a new custom highlighter type is added
> > to the server?
> >
> > In that case the CustomHighlighterActor initializer will throw (typeName + "
> > isn't a valid highlighter class")
> I can see 2 solutions:
> - turn the hasCustomHighlighters trait into an array of custom highlighter
> type strings, so that the client-side can know which types do exist on the
> server
> - make the InspectorActor's getHighlighterByType method return null when a
> type isn't known.
> I have a preference for the latter.
There are tradeoffs to both. The second one relies less on traits and wouldn't need a duplicated list of supported highlighters on the server. The benefit to the first one is that it gives a very explicit check like you are currently using in part 4. This would save from having to bind extra event listeners and spread conditional logic into multiple places. Anywhere you get the highlighter in the rule view you have to protect against null:
this.getTransformHighlighter().then(highlighter => highlighter.show(node));
Becomes
this.getTransformHighlighter().then(highlighter => {
if (highlighter) {
highlighter.show(node);
}
});
After thinking about it for a bit, I don't really have a strong preference. If the traits could be accomplished *without* maintaining a second list I think that would be slightly preferable just b/c it makes the frontend code easier to work with - having an immediate condition to bail out on instead of async. If it was possible to do something like:
traits: {
supportedHighlighters: Object.keys(HIGHLIGHTER_CLASSES)
}
then frontend could check traits.supportedHighlighters && traits.supportedHighlighters["CssTransformHighlighter"]
Comment 36•11 years ago
|
||
Comment on attachment 8433216 [details] [diff] [review]
bug1014547-transform-highlighter-3-revisit_highlighter_api v7.patch
Review of attachment 8433216 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/server/actors/inspector.js
@@ +2570,5 @@
> + *
> + * @param {String} type The type of highlighter to create
> + * @return {Highlighter}
> + */
> + getHighlighterByType: method(function (typeName) {
As mentioned in Comment 34, this should maybe return null if the custom highlighter type isn't known to provide backwards compatibility for the client.
@@ +2571,5 @@
> + * @param {String} type The type of highlighter to create
> + * @return {Highlighter}
> + */
> + getHighlighterByType: method(function (typeName) {
> + return CustomHighlighterActor(this, typeName);
Does this not need to wait for the walker like getHighlighter does?
Attachment #8433216 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 37•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #35)
> If it was possible to do something like:
>
> traits: {
> supportedHighlighters: Object.keys(HIGHLIGHTER_CLASSES)
> }
>
> then frontend could check traits.supportedHighlighters &&
> traits.supportedHighlighters["CssTransformHighlighter"]
I like this a lot. I will probably change the code to work like this.
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #36)
> Comment on attachment 8433216 [details] [diff] [review]
> bug1014547-transform-highlighter-3-revisit_highlighter_api v7.patch
>
> Review of attachment 8433216 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +2570,5 @@
> > + *
> > + * @param {String} type The type of highlighter to create
> > + * @return {Highlighter}
> > + */
> > + getHighlighterByType: method(function (typeName) {
>
> As mentioned in Comment 34, this should maybe return null if the custom
> highlighter type isn't known to provide backwards compatibility for the
> client.
That's right. I made those changes in part 4 though. I will upload the new version soon.
(talking about this, I think the border between part 3 and 4 tends to get blurry as I progress on this bug, so I'll probably fold these 2 patches together before landing).
> @@ +2571,5 @@
> > + * @param {String} type The type of highlighter to create
> > + * @return {Highlighter}
> > + */
> > + getHighlighterByType: method(function (typeName) {
> > + return CustomHighlighterActor(this, typeName);
>
> Does this not need to wait for the walker like getHighlighter does?
In fact it does not. The css transform highlighter doesn't need to access the walker so far. The boxmodel highlighter does need it because that's the one we use to 'pick' nodes from the page, and picking nodes requires to use the walker's attachElement method.
It will be easy enough to add if one day we implement a new highlighter that needs to use the walker, but for now it's not necessary.
Assignee | ||
Comment 39•11 years ago
|
||
part 4 - v8
Should be ready for review.
Main things that changed in this version:
- Fixed the bug reported by bgrins about the BMH not hiding when mouseleaving the markup-view
- The highlighter now isn't shown for pseudo-element transform (see bug 1019457)
- The root actor now has a trait that lists the known highlighter types (this list is in 2 places as it doesn't seem feasible to require highlighter.js from root.js as this also transitively requires inspector.js and fails since the actor doesn't seem to be register. One solution is to require it only in RootActor's sayHello method, when everything's available, but doing so would complexify this otherwise very simple function which shouldn't have to deal with specific actors).
Attachment #8432522 -
Attachment is obsolete: true
Attachment #8433594 -
Flags: review?(bgrinstead)
Comment 40•11 years ago
|
||
Comment on attachment 8433594 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v8.patch
Review of attachment 8433594 [details] [diff] [review]:
-----------------------------------------------------------------
Everything is looking good so far. I haven't reviewed highlighter.js yet, but just sending over my thoughts from the rest of the code.
* The arrows seem to get drawn *starting* from the corners, meaning that they protrude out by the length of the arrow. If you can make it start back that same number of px it would fit in a little nicer.
* Please add a test that checks the coordinates for the boxes drawn by the highlighter in a variety of cases (including iframes, scrolling, etc). Having separate test coverage for getAdjustedBoxQuads could probably make the test for the highlighter coords quite simpler - as it could then just compare the svg with the results of getAdjustedBoxQuads.
::: browser/devtools/framework/toolbox-highlighter-utils.js
@@ +29,5 @@
> * @param {Toolbox} toolbox
> * @return {Object} the highlighterUtils public API
> */
> exports.getHighlighterUtils = function(toolbox) {
> + let target = toolbox.target;
This will throw if toolbox was null, was guarded by the first condition in the if before. I guess it doesn't really matter much, since this is always called by a toolbox instance
@@ +197,5 @@
> if (forceHide && toolbox.highlighter) {
> + let currentPromise = showBoxModelPromise;
> + yield showBoxModelPromise;
> + // At this stage, showBoxModelPromise may be different if a request to
> + // show was made in parallel, in which case wait for the new one
Should we unhighlight in this case or just return and do nothing? The sequence of events here seemed to be
showBoxModelPromise1 = show(el1)
hide() /* Yielding for showBoxModelPromise1.... */
showBoxModelPromise2 = show(el2) /* While hide is still yielding */
showBoxModelPromise1 resolves
The most recent request by the user in this case has been show(), so do we want to immediately call unhighlight again? Seems like this could cause the box model highlighter to not show up on el2 (well, technically it would show up for only a flash)
::: browser/devtools/inspector/test/browser_inspector_invalidate.js
@@ +26,3 @@
> div.style.width = "200px";
> +
> + info("Waiting for the next MozAfterPaint event to let the highlighter update");
I may be misunderstanding the purpose of this, but is this MozAfterPaint call going to be something that will need to be reused throughout tests? If so, could we hide that inside of the highlighter-ready event in the case of gDevTools.testing?
::: browser/devtools/styleinspector/computed-view.js
@@ +547,5 @@
> + this.propertyContainer.addEventListener("mousemove", this._onMouseMove, false);
> + this.propertyContainer.addEventListener("mouseleave", this._onMouseLeave, false);
> + },
> +
> + _onMouseMove: function(event) {
This pattern for listening to mouse move on new targets to show an overlay in the computed view seems like something that could be handled with less coupling to the css transform overlay (similarly to how _onTooltipTargetHover supports multiple tooltip types). Ideally we will have a function like _onHighlightTargetHover that only gets called when there is a new target and _onHighlightTargetLeave that lets all overlays cleanup. Since it's the only one for now we can leave it as-is and do that as a refactor when a new overlay is added.
::: browser/devtools/styleinspector/rule-view.js
@@ +1188,5 @@
> + this.element.addEventListener("mouseleave", this._onMouseLeave, false);
> + },
> +
> + _onMouseMove: function(event) {
> + if (event.target === this._lastHovered) {
Same comment as in computed view - we will want to avoid adding a new mousemove listener for each kind of new highlighter that gets added. This would also be a lot of boilerplate just to add a new highlighter type, so hopefully this can be refactored once a second one gets added.
Assignee | ||
Comment 41•11 years ago
|
||
Thanks for the review. Some comments below:
(In reply to Brian Grinstead [:bgrins] from comment #40)
> Comment on attachment 8433594 [details] [diff] [review]
> bug1014547-transform-highlighter-4-implement_transform_highlighter v8.patch
>
> Review of attachment 8433594 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Everything is looking good so far. I haven't reviewed highlighter.js yet,
> but just sending over my thoughts from the rest of the code.
>
> * The arrows seem to get drawn *starting* from the corners, meaning that
> they protrude out by the length of the arrow. If you can make it start back
> that same number of px it would fit in a little nicer.
I just fixed that and also added a check on the line distance, if it's too short, I'm not displaying the arrow marker at all.
> * Please add a test that checks the coordinates for the boxes drawn by the
> highlighter in a variety of cases (including iframes, scrolling, etc).
> Having separate test coverage for getAdjustedBoxQuads could probably make
> the test for the highlighter coords quite simpler - as it could then just
> compare the svg with the results of getAdjustedBoxQuads.
Will do.
> ::: browser/devtools/framework/toolbox-highlighter-utils.js
> @@ +29,5 @@
> > * @param {Toolbox} toolbox
> > * @return {Object} the highlighterUtils public API
> > */
> > exports.getHighlighterUtils = function(toolbox) {
> > + let target = toolbox.target;
>
> This will throw if toolbox was null, was guarded by the first condition in
> the if before. I guess it doesn't really matter much, since this is always
> called by a toolbox instance
I moved this variable assignment to after the if.
> @@ +197,5 @@
> > if (forceHide && toolbox.highlighter) {
> > + let currentPromise = showBoxModelPromise;
> > + yield showBoxModelPromise;
> > + // At this stage, showBoxModelPromise may be different if a request to
> > + // show was made in parallel, in which case wait for the new one
>
> Should we unhighlight in this case or just return and do nothing? The
> sequence of events here seemed to be
>
> showBoxModelPromise1 = show(el1)
> hide() /* Yielding for showBoxModelPromise1.... */
> showBoxModelPromise2 = show(el2) /* While hide is still yielding */
> showBoxModelPromise1 resolves
>
> The most recent request by the user in this case has been show(), so do we
> want to immediately call unhighlight again? Seems like this could cause the
> box model highlighter to not show up on el2 (well, technically it would show
> up for only a flash)
This makes sure unhighlight is given more importance as it will always be executed, even if a parallel show request is made. I did this specifically to fix the mouseleave bug you reported.
What was happening is that in some rare cases, when you trigger a mouseleave by moving your mouse very quickly over a few nodes in the markup-view and then moved away towards the rule-view, was that a late show request would be sent to the server.
The server has no way of knowing if something went wrong, so the fix for this use case was done here, in the highlighter-utils.
There might be other ways of fixing it though, not sure. But it seems to behave correctly when using the markup-view.
> ::: browser/devtools/inspector/test/browser_inspector_invalidate.js
> @@ +26,3 @@
> > div.style.width = "200px";
> > +
> > + info("Waiting for the next MozAfterPaint event to let the highlighter update");
>
> I may be misunderstanding the purpose of this, but is this MozAfterPaint
> call going to be something that will need to be reused throughout tests? If
> so, could we hide that inside of the highlighter-ready event in the case of
> gDevTools.testing?
It's not ideal indeed. Luckily no other existing tests needed to do this, but it would be a shame to have to copy this for future tests.
As discussed over IRC, we just can't rely anymore on the highlighter-ready event now. It used to be sent at every update, which was nice for tests, but since I fixed the fact that the highlighter wasn't refreshing on MozAfterPaint before, it would cause this event to be sent at every paint, which would be way too much traffic (and not really needed).
I could create a new function in head.js specifically for waiting for the next highlighter update, and one way to do this is to add a mutationobserver on the highlighter to know when the coordinates do change (since the highlighter is SVG and is positioned via attributes).
Will investigate.
> ::: browser/devtools/styleinspector/computed-view.js
> @@ +547,5 @@
> > + this.propertyContainer.addEventListener("mousemove", this._onMouseMove, false);
> > + this.propertyContainer.addEventListener("mouseleave", this._onMouseLeave, false);
> > + },
> > +
> > + _onMouseMove: function(event) {
>
> This pattern for listening to mouse move on new targets to show an overlay
> in the computed view seems like something that could be handled with less
> coupling to the css transform overlay (similarly to how
> _onTooltipTargetHover supports multiple tooltip types). Ideally we will
> have a function like _onHighlightTargetHover that only gets called when
> there is a new target and _onHighlightTargetLeave that lets all overlays
> cleanup. Since it's the only one for now we can leave it as-is and do that
> as a refactor when a new overlay is added.
Yeah, I've been wanting to clean this up for a while, actually moving both the highlighter handling AND the tooltip handling into a separate class (or even a separate module shared with the computed-view, to reduce code duplication). I wanted to tackle this in this bug originally, but it would be better done in its own bug. So I filed bug 1020291.
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1188,5 @@
> > + this.element.addEventListener("mouseleave", this._onMouseLeave, false);
> > + },
> > +
> > + _onMouseMove: function(event) {
> > + if (event.target === this._lastHovered) {
>
> Same comment as in computed view - we will want to avoid adding a new
> mousemove listener for each kind of new highlighter that gets added. This
> would also be a lot of boilerplate just to add a new highlighter type, so
> hopefully this can be refactored once a second one gets added.
Same bug 1020291.
Comment 42•11 years ago
|
||
Comment on attachment 8433594 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v8.patch
Review of attachment 8433594 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/server/actors/highlighter.js
@@ +421,5 @@
> + */
> + _detachPageListeners: function() {
> + if (isNodeValid(this.currentNode)) {
> + let win = this.currentNode.ownerDocument.defaultView;
> + win.removeEventListener("scroll", this.update);
Are scroll/resize needed or will mozafterpaint fire after these events anyway?
@@ +957,5 @@
> + path.setAttribute("fill", "#08C");
> + marker.appendChild(path);
> + this._svgRoot.appendChild(marker);
> +
> + // Create the 2 polygons (transformed and unstransformed)
Typo: "untransformed"
@@ +1035,5 @@
> + // Getting the points for the transformed shape
> + let quad = this.layoutHelpers.getAdjustedQuads(this.currentNode, "border");
> + if (!quad) {
> + return null;
> + }
Since we are early returning here anyway, maybe adding the condition seen further down in this function could save on some thinking/indentation later:
if (!quad || quad.bounds.width <= 0 || quad.bounds.height <= 0) {
this._hideShapes();
return null;
}
This would get rid of the remaining condition down there, and while I don't know the exact situation where quad is falsy here, it probably wouldn't hurt to call hideShapes in that case.
@@ +1063,5 @@
>
> + // For transformed elements, only offsetWidth/Height/Top/Left will give us
> + // the right untransformed coordinates
> +
> + // Find out the offset of the node in its current frame
This function feels like it belongs stowed away in layoutHelpers so we don't have to look at it :). But seriously, I guess that all of the helper methods like getBoundingClientRect are transform-aware so there isn't an easier way.
I think this + getAdjustedQuads is acting weirdly with inline elements. For instance, given this page:
data:text/html,<div style='width:10px;'><span style='transform: translate(100px)'>text that wraps</span></div>
Hovering over the span will draw the initial box as covering the whole element (which seems right), but the final blue region will cover just the first line. Of course this is an edge case, since the transform doesn't do anything anyway. Ideally this would be caught by the same logic that doesn't draw arrows when untransformaed and transformed quads are the same.
Attachment #8433594 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 43•11 years ago
|
||
part 3 - simple rebase
Attachment #8433216 -
Attachment is obsolete: true
Attachment #8434934 -
Flags: review+
Assignee | ||
Comment 44•11 years ago
|
||
I seemed to have been a little bit too ambitious in removing the 'highlightable' trait from the root actor in part 2.
Indeed, our tools must be backwards compatible with at least B2G 1.3, which is 28, which didn't have the highlighter actor at all.
So I need to put this back in part 2.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #44)
> I seemed to have been a little bit too ambitious in removing the
> 'highlightable' trait from the root actor in part 2.
> Indeed, our tools must be backwards compatible with at least B2G 1.3, which
> is 28, which didn't have the highlighter actor at all.
> So I need to put this back in part 2.
Glad you caught this, was about to make a comment... :)
Generally, we'll have to be very careful about removing existing traits to ensure everything is timed correctly (if we decide we need to do that in some future bug).
Assignee | ||
Comment 46•10 years ago
|
||
part 2 - v6 - This part moved the highlighter utils to a separate module, but was removing the backward compatibility with targets that didn't have the highlighter actor.
v6 restores this compatibility.
Brian, I feel bad asking you for all these reviews at the moment :( but since you're the one who reviewed all the other patches on this bug, I guess it makes sense. Don't hesitate transferring R? to someone else if needed.
Attachment #8433215 -
Attachment is obsolete: true
Attachment #8437619 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 47•10 years ago
|
||
rebased part 3
Attachment #8434934 -
Attachment is obsolete: true
Attachment #8437622 -
Flags: review+
Assignee | ||
Comment 48•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #42)
> Comment on attachment 8433594 [details] [diff] [review]
> bug1014547-transform-highlighter-4-implement_transform_highlighter v8.patch
>
> Review of attachment 8433594 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +421,5 @@
> > + */
> > + _detachPageListeners: function() {
> > + if (isNodeValid(this.currentNode)) {
> > + let win = this.currentNode.ownerDocument.defaultView;
> > + win.removeEventListener("scroll", this.update);
>
> Are scroll/resize needed or will mozafterpaint fire after these events
> anyway?
Yes you're right, I've removed these listeners now.
> @@ +1035,5 @@
> > + // Getting the points for the transformed shape
> > + let quad = this.layoutHelpers.getAdjustedQuads(this.currentNode, "border");
> > + if (!quad) {
> > + return null;
> > + }
>
> Since we are early returning here anyway, maybe adding the condition seen
> further down in this function could save on some thinking/indentation later:
>
> if (!quad || quad.bounds.width <= 0 || quad.bounds.height <= 0) {
> this._hideShapes();
> return null;
> }
>
> This would get rid of the remaining condition down there, and while I don't
> know the exact situation where quad is falsy here, it probably wouldn't hurt
> to call hideShapes in that case.
Yep, no harm in calling hideShapes in both cases. Change done.
> @@ +1063,5 @@
> >
> > + // For transformed elements, only offsetWidth/Height/Top/Left will give us
> > + // the right untransformed coordinates
> > +
> > + // Find out the offset of the node in its current frame
>
> This function feels like it belongs stowed away in layoutHelpers so we don't
> have to look at it :). But seriously, I guess that all of the helper
> methods like getBoundingClientRect are transform-aware so there isn't an
> easier way.
You're right, I should probably move this in LayoutHelpers, and yeah, it's not pretty, but I don't know of a better way to get the coordinates of the shape un-transformed.
> I think this + getAdjustedQuads is acting weirdly with inline elements. For
> instance, given this page:
>
> data:text/html,<div style='width:10px;'><span style='transform:
> translate(100px)'>text that wraps</span></div>
>
> Hovering over the span will draw the initial box as covering the whole
> element (which seems right), but the final blue region will cover just the
> first line. Of course this is an edge case, since the transform doesn't do
> anything anyway. Ideally this would be caught by the same logic that
> doesn't draw arrows when untransformaed and transformed quads are the same.
You're right, getAdjustedQuads only takes the first quad returned by getBoxQuads, which is wrong for multi-line inline elements since several line boxes are returned.
Filed bug 1023232 to take care of this issue.
I guess the best to do in this case is to not highlight the transform for inline elements since it doesn't apply anyway.
Assignee | ||
Comment 49•10 years ago
|
||
part 4 - v9
Changes in this version:
- arrows linking the 2 shapes now end at corners
- arrows aren't displayed if the line size is below a certain size
- changed the way we wait for BMH updates in tests (now observing markup mutations)
- added a getAdjustedBoxQuads layoutHelpers test
- added a new highlighterFront test that checks that the coords of the svg elements are in line with those returned by getAdjustedBoxQuads
- removed scroll/resize listeners, not needed because we already listen for paint events
- inline elements aren't highlighted anymore
- moved _getUntransformedQuad to layoutHelpers and cleaned a few functions in that file
- also fixed this function so it takes zoom into account
I still need to work on passing test in try build.
Latest try results (https://tbpl.mozilla.org/?tree=Try&rev=8f98c45da308) still shows one of the new test I added failing.
Attachment #8433594 -
Attachment is obsolete: true
Comment 50•10 years ago
|
||
Comment on attachment 8437619 [details] [diff] [review]
bug1014547-transform-highlighter-2-move_highlighter_utils v6.patch
Review of attachment 8437619 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like a pretty straightforward change with the old v2 patch: https://bugzilla.mozilla.org/attachment.cgi?oldid=8433215&action=interdiff&newid=8437619&headers=1. It would be great to have test coverage against these older servers to make sure we don't accidentally land something that breaks support
Attachment #8437619 -
Flags: review?(bgrinstead) → review+
Comment 51•10 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #49)
> Created attachment 8437686 [details] [diff] [review]
> bug1014547-transform-highlighter-4-implement_transform_highlighter v9.patch
>
> part 4 - v9
Is this ready for review?
Assignee | ||
Comment 52•10 years ago
|
||
Thanks for the review.
(In reply to Brian Grinstead [:bgrins] from comment #50)
> Comment on attachment 8437619 [details] [diff] [review]
> bug1014547-transform-highlighter-2-move_highlighter_utils v6.patch
>
> Review of attachment 8437619 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks like a pretty straightforward change with the old v2 patch:
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8433215&action=interdiff&newid=8437619&headers=1. It would be
> great to have test coverage against these older servers to make sure we
> don't accidentally land something that breaks support
I've been working on a part 5 patch that unit tests the highlighter-utils API by mocking all of its dependencies, so it'll be easy enough to make a backend that doesn't have the highlighter actor or custom highlighters to make sure these cases still work in the future.
Assignee | ||
Comment 53•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #51)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #49)
> > Created attachment 8437686 [details] [diff] [review]
> > bug1014547-transform-highlighter-4-implement_transform_highlighter v9.patch
> >
> > part 4 - v9
>
> Is this ready for review?
Not yet, but should be pretty soon.
Assignee | ||
Comment 54•10 years ago
|
||
Alright, almost there. The last try fixes all the problems I've been seeing in the past: https://tbpl.mozilla.org/?tree=Try&rev=a0d978262be5
but unfortunately brings a new intermittent failure.
It shouldn't be too hard to fix though, a couple of changes to the highlighter landed recently and they're probably not playing well with my changes.
In any case I think this patch is now ready for review, and the changes needed to fix the last intermittent are probably going to be in tests.
Attachment #8437686 -
Attachment is obsolete: true
Attachment #8438460 -
Flags: review?(bgrinstead)
Updated•10 years ago
|
Attachment #8432662 -
Attachment is obsolete: true
Assignee | ||
Comment 55•10 years ago
|
||
As I suspected, I was able to fix the try failures by modifying the test which wasn't waiting for a node to be selected before checking the highlighter. New try build: https://tbpl.mozilla.org/?tree=Try&rev=92405dae9066
Comment 56•10 years ago
|
||
Comment on attachment 8438460 [details] [diff] [review]
bug1014547-transform-highlighter-4-implement_transform_highlighter v10.patch
Review of attachment 8438460 [details] [diff] [review]:
-----------------------------------------------------------------
The css transform highlighting itself works great and I've - I've thrown a bunch of different things at it on this demo page and it correctly highlights all cases I've seen: http://css3.bradshawenterprises.com/transforms/#playground.
Looks like there are still a couple of test changes needed to fix an orange. In addition to this, I'm still bumping into some weirdness with the box model highlighter 'sticking' when I quickly jump from one node to another and leave the markup view (https://i.imgur.com/Q0McbPH.gif and https://i.imgur.com/XIgEGIN.gif). I also see this message in my console quite often a second or two after a highlighter (I think either css or box model) shows up:
A promise chain failed to handle a rejection.
Full Message: false
I can help try to track this down if you'd like, since it seems to not be an issue on your computer.
::: toolkit/devtools/server/actors/highlighter.js
@@ +1174,4 @@
> }
>
> // Is it an element node
> + if (node.nodeType !== Ci.nsIDOMNode.ELEMENT_NODE) {
It won't make much of a difference, but you could move this check above the contains() check since this would be a faster thing to check to be able to quickly bail out on comment nodes, for instance.
Attachment #8438460 -
Flags: review?(bgrinstead)
Comment 57•10 years ago
|
||
Here is a new issue I bumped into when opening the Browser Toolbox to the inspect panel then closing the Browser Toolbox:
Handler function DebuggerTransport.prototype.onInputStreamReady threw an exception: TypeError: this._boxModelHighlighter.off is not a function
Stack: exports.HighlighterActor<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:97:7
Pool<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:775:9
Actor<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:839:5
Pool<.cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:789:5
DSC_onClosed/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1249:40
DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1249:5
DebuggerTransport.prototype.close@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:198:7
DebuggerTransport.prototype.onInputStreamReady<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:341:7
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
Line: 97, column: 6
After this, it doesn't show up in the context menu anymore.
Comment 58•10 years ago
|
||
> After this, it doesn't show up in the context menu anymore.
By context menu, I meant the Tools->Web Developer menu, but now I'm not seeing that again after reproducing. I do see the exception every time, though
Assignee | ||
Comment 59•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #57)
> Here is a new issue I bumped into when opening the Browser Toolbox to the
> inspect panel then closing the Browser Toolbox:
>
> Handler function DebuggerTransport.prototype.onInputStreamReady threw an
> exception: TypeError: this._boxModelHighlighter.off is not a function
> Stack:
> exports.HighlighterActor<.destroy@resource://gre/modules/commonjs/toolkit/
> loader.js ->
> resource://gre/modules/devtools/server/actors/highlighter.js:97:7
> Pool<.destroy@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/protocol.js:775:9
> Actor<.destroy@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/protocol.js:839:5
> Pool<.cleanup@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/protocol.js:789:5
> DSC_onClosed/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1249:40
> DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1249:5
> DebuggerTransport.prototype.close@resource://gre/modules/devtools/dbg-client.
> jsm -> resource://gre/modules/devtools/transport/transport.js:198:7
> DebuggerTransport.prototype.onInputStreamReady<@resource://gre/modules/
> devtools/dbg-client.jsm ->
> resource://gre/modules/devtools/transport/transport.js:341:7
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/DevToolsUtils.js:84:7
> Line: 97, column: 6
>
> After this, it doesn't show up in the context menu anymore.
I think this is bug 1016331, which happens without my patches too.
Assignee | ||
Comment 60•10 years ago
|
||
I think this gets rid of the sticky highlighter, once and for all.
I'm not seeing the rejected promise errors Brian mentioned earlier though ... And I wasn't seeing them before this fix either. I don't know where these came from.
I'm also working on a fix for the remaining intermittent on try. Should be ready soon.
Attachment #8438460 -
Attachment is obsolete: true
Assignee | ||
Comment 61•10 years ago
|
||
Finally, a green try build :)
https://tbpl.mozilla.org/?tree=Try&rev=ef6652ed6424
So, the last remaining failing test is now fixed.
The sticky highlighter too (it was already fixed in v11, but in v12 I removed the showPromise that I had introduced previously to fix this bug).
I also investigated a little bit the random unhandled rejected promises errors and discovered I also have them without the patches applied so I filed bug 1024910 for this.
Attachment #8439262 -
Attachment is obsolete: true
Attachment #8439763 -
Flags: review?(bgrinstead)
Updated•10 years ago
|
Attachment #8439763 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 62•10 years ago
|
||
Thanks for the reviews Brian.
Here's a new patch that folds all 4 R+ parts.
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/141d06692291
Attachment #8430239 -
Attachment is obsolete: true
Attachment #8437619 -
Attachment is obsolete: true
Attachment #8437622 -
Attachment is obsolete: true
Attachment #8439763 -
Attachment is obsolete: true
Attachment #8439917 -
Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment 64•10 years ago
|
||
Added to the release notes with "Developer Tools: CSS transform highlighter to the style-inspector"
relnote-firefox:
--- → 33+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•