Closed Bug 1435373 Opened 6 years ago Closed 6 years ago

Editing a shape using the shape path editor should edit the property the tool was started from

Categories

(DevTools :: Inspector: Rules, enhancement, P2)

Product:
DevTools
Component:
Inspector: Rules
Type:
enhancement

Tracking

(firefox61 verified, firefox62 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: pbro, Assigned: rcaliman)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 10 obsolete files)

Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. r=gl
11.95 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule. r=pbro
54.44 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
Bug 1435373 - Shapes editor: update tests for new implementation. r=pbro
60.51 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review 
Here are my steps:

- open this URL in a browser tab:
data:text/html,<style>div {width:100px;height:100px;background:red;clip-path:polygon(0 0, 100% 0, 100% 100%);}</style><div>

- open the inspector, select the <div>

- in the Rules view, click on the shapes icon next to clip-path

- start modifying the shape in the page by dragging points

==> A new clip-path property appears on the inline style of the element.
I would expect the original property where I clicked to be updated instead.
So, technically, the difficulty here is that updates happen on the server-side, in the highlighter (/devtools/server/actors/highlighters/shapes.js). And at this level, we have no knowledge of CSS rules.

So, somehow, we should give the highlighter a reference to the CSS rule that should be updated instead. We should be able to do this when calling the highlighter's show method.

So, back on the client, we handle clicks on the shape icon in devtools/client/inspector/shared/highlighters-overlay.js (onClick method). This then calls the toggleShapesHighlighter function.
At this point, it should be possible to know which CSS Rule was the icon clicked in. We have access to the inspector, to the rule-view, and to the DOM node that was clicked on, so it shouldn't be too hard getting the reference to the actual Rule object from this. The Rule object we want is the one which class is in devtools/client/inspector/rules/models/rule.js
This object has a reference to the CSSRuleActor (via its actorID anyway, or front).

The idea is we would send this actorID along with the highlighter's show method (as part of the options object).
Then, back on the server, the highlighter would just check if this information is present. If it is, it would try to get the actor itself from it.
If either the actor can't be found or the info isn't here, then it would still make shapes edits to the inline style.
But if we can find the right actor, then that means we could make edits there directly. Find the right property, and edit that instead.

Feel free to get in touch with me for more info if needed, I can probably investigate in more details.
Assignee: nobody → rcaliman
Depends on: 1438127
No longer depends on: 1438127
Comment on attachment 8952208 [details]
Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision.

https://reviewboard.mozilla.org/r/221460/#review227982
Attachment #8952208 - Flags: review?(gl) → review+
Attachment #8952207 - Flags: review?(gl)
Attachment #8952207 - Flags: review?(gl)
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review228674


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/rules/views/text-property-editor.js:531
(Diff revision 2)
> +        onCommit: this._onSwatchCommit,
> +        onRevert: this._onSwatchRevert
> +      };
> +
> +      if (shapesEditor.activeSwatch) {
> +        shapesEditor.replaceSwatch(shapesEditor.activeSwatch, this.shapeToggle, callbacks);

Error: Line 531 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

This is still a work in progress, but I'd like some feedback on the approach.

The basic use case works now. Run this in the URL bar:
data:text/html;charset=utf-8,<style>.polygon {background: red; width:300px; height: 300px; clip-path: polygon(0 0 , 100% 0, 100% 100%);}</style><div class="polygon"></div>

Work still in progress:
- correctly mapping changes back to inline styles when that's the origin of the shape
- correctly selecting the rule in the rule view when multiple properties compete via cascade / media queries
- fix tests (none have been updated and most will fail because changes are now applied async instead of sync via inline style)
- consolidate point hover logic & remove obsolete code
Attachment #8952207 - Flags: feedback?(gl)
Attachment #8952207 - Flags: feedback?(pbrosset)
Attachment #8952207 - Flags: feedback?(gl)
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review229608


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/rules/views/text-property-editor.js:531
(Diff revision 3)
> +        onCommit: this._onSwatchCommit,
> +        onRevert: this._onSwatchRevert
> +      };
> +
> +      if (shapesEditor.activeSwatch) {
> +        shapesEditor.replaceSwatch(shapesEditor.activeSwatch, this.shapeToggle, callbacks);

Error: Line 531 exceeds the maximum line length of 90. [eslint: max-len]
Attachment #8954425 - Attachment is obsolete: true
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review229868

Hi Razvan. Here is a first set of review comments which I hope will be useful.
I have tested the patch locally and found it to work really well. Of course, I haven't done extensive manual tests, but I did try to break it a little bit and couldn't.
I made a bunch of comments in the code, and asked a few questions to open up the discussion.
Obviously, this is a somewhat important change, so I want to make sure I understand the overal interactions of things before signing off on it.
Btw, as we discussed, it would be very helpful to have a simple diagram of how things interact with eachother.

::: commit-message-d0d36:8
(Diff revision 4)
> +- Wrap shape highlighter with in-context shape editor singleton
> +- Link text properties from Rule view into in-context shapes editor
> +- Write changes to corresponding rule instead of inline style
> +- Hide shape highlighter on node change or TextProperty disabled/overwritten
> +- Disable functionality to restore shape editor on refresh
> +- Refactor and consolidate shape point hover behavirour (still some loose ends)

nit: behavirour -> behaviour
Also, could you describe what the loose ends are in the commit message?

::: devtools/client/inspector/rules/views/text-property-editor.js:518
(Diff revision 4)
>          return s[0].toUpperCase() + s.slice(1);
>        }).join("");
>        shapeToggle.setAttribute("data-mode", mode);
>  
> -      let { highlighters, inspector } = this.ruleView;
> -      if (highlighters.shapesHighlighterShown === inspector.selection.nodeFront &&
> +      let shapesEditor =
> +        await this.ruleView.highlighters.getInContextEditor("shapesEditor");

Now that this is async, we need to think of race conditions.
Can you think of anything that can change while we're waiting for the promise to resolve? If so, then right after the await statement, we should check for it and act appropriately.

It's unlikely to happen in real life, but in tests, it's frequent to start some async action and then end the test immediately, which fails.
So, maybe the right thing to do here is to check if the ruleView hasn't been destroyed. And if it has, just return silently.

::: devtools/client/inspector/rules/views/text-property-editor.js:520
(Diff revision 4)
> -        shapeToggle.classList.add("active");
> -        highlighters.highlightRuleViewShapePoint(highlighters.state.shapes.hoverPoint);
> -      }
> +      let callbacks = {
> +        onPreview: this._onInContextEditorPreview,
> +        onCommit: this._onInContextEditorCommit
> +      };

This variable isn't used anywhere else than on the next line. Maybe no need to declare it, and just use:

```
shapesEditor.link(this.prop, shapeToggle, {
  onPreview: this._onInContextEditorPreview,
  onCommit: this._onInContextEditorCommit
});
```

::: devtools/client/inspector/shared/highlighters-overlay.js:26
(Diff revision 4)
>    /**
>     * @param  {Inspector} inspector
>     *         Inspector toolbox panel.
>     */
>    constructor(inspector) {
> +    this.editors = {};

`this.highlighters` doesn't need any introduction in this file, because this is called `HighlightersOverlay`, and we generally what highlighters means in devtools. However here you're introducing a new notion: `editors`, and it's not immediately obvious to the person readin the code what this is.

So I would suggest moving both `this.editors` and `this.highlighters` to be first in the contructor, and preceding both of them with some self-explanatory comments.

::: devtools/client/inspector/shared/highlighters-overlay.js:158
(Diff revision 4)
> +  async presistShapesHighlighterState(node, options) {
>      try {
>        // Save shapes highlighter state.
>        let { url } = this.inspector.target;
>        let selector = await node.getUniqueSelector();
>        this.state.shapes = { selector, options, url };
> -      this.shapesHighlighterShown = node;
> -      this.emit("shapes-highlighter-shown", node, options);
>      } catch (e) {
>        this._handleRejection(e);
>      }
>    }

I seem to remember you talking about how this was now hard to do with your new approach. That when trying to restore the state, it would be hard to know which rule to tie it to.
Is this the case or did I misunderstand something?

::: devtools/client/inspector/shared/highlighters-overlay.js:454
(Diff revision 4)
> -    try {
> -      await this.restoreState("shapes", this.state.shapes, this.showShapesHighlighter);
> -    } catch (e) {
> -      this._handleRejection(e);
> -    }
> +    // try {
> +    //   await this.restoreState("shapes", this.state.shapes, this.showShapesHighlighter);
> +    // } catch (e) {
> +    //   this._handleRejection(e);
> +    // }

If this code is commented out, we don't need it anymore, so the whole function should be removed.

::: devtools/client/inspector/shared/highlighters-overlay.js:500
(Diff revision 4)
>      }
>  
>      this.emit(`${name}-state-restored`, { restored: false });
>    }
>  
> +  async getInContextEditor(type) {

We're introducing a new sort of component in this file: in-context editors. It would be good to describe what they are, how they are expected to work, what is their high-level usage/API.
So, either as a jsdoc comment here, or maybe as an introduction comment block at the top of the file.

::: devtools/client/inspector/shared/highlighters-overlay.js:509
(Diff revision 4)
> +        let highlighter = await this._getHighlighter("ShapesHighlighter");
> +        if (!highlighter) {
> +          return null;
> +        }

I wonder if ShapesInContextEditor shouldn't be the one responsible for getting its own highlighter.
What are your thoughts on this?
ShapesInContextEditor is a special thing that happens to work with a highlighter. But other types of editors might need other things. So maybe best if it creates its own required instances of things by itself.

::: devtools/client/inspector/shared/highlighters-overlay.js:515
(Diff revision 4)
> +        if (!highlighter) {
> +          return null;
> +        }
> +        const ShapesInContextEditor = require("devtools/client/shared/widgets/ShapesInContextEditor");
> +
> +        editor = new ShapesInContextEditor(highlighter, this.inspector, this.state);

Do we really need this to be a class? There's only ever one HighlightersOverlay, and for one given type, it always returns the same instance. So, as you said in the commit message, ShapesInContextEditor is a singleton. Therefore, wouldn't it be simpler to have it as a simple object rather than a class that needs to be instantiated (btw, HighlightersOverlay also is a class for some reason, this isn't related to your patch, but it seems like we could change it to not be one).

::: devtools/client/inspector/shared/highlighters-overlay.js:855
(Diff revision 4)
> +      this.editors[type].off("show");
> +      this.editors[type].off("hide");

I don't think the event listener will be removed if you don't pass the reference to the same callback than you did when you added the listener with `on`.
Having said that, I realize I don't know for sure...

::: devtools/client/inspector/shared/highlighters-overlay.js:870
(Diff revision 4)
>      for (let type in this.highlighters) {
>        if (this.highlighters[type]) {
> -        if (this.highlighters[type].off) {
> -          this.highlighters[type].off("highlighter-event", this._onHighlighterEvent);
> -        }
>          this.highlighters[type].finalize();
>          this.highlighters[type] = null;
>        }
>      }

Now that you introduced a new `drestroyEditors` function, it might be good to also wrap this in a `destroyHighlighters` function, so it's clear there are 2 sorts of similar things at play here.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:10
(Diff revision 4)
> +"use strict";
> +
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const { debounce } = require("devtools/shared/debounce");
> +
> +class ShapesInContextEditor {

This needs some jsdoc comment, to at least give a high-level description of what this class does, how it's used, etc.

Also, can you explain what your thoughts are regarding potential similar editors we might need in the future.
It looks to me like this type of in-context editor will be very useful for other things, such as fonts. But these other editors will have partly the same needs (swatch click/link to prop), and other very different needs (maybe not a highlighter).
So, doing a very generic/reusable thing right now isn't a good idea, because we don't know when/if similarm editors will be needed. But I'd like to have your thoughts on this.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:26
(Diff revision 4)
> +    this._onSwatchClick = this._onSwatchClick.bind(this);
> +
> +    this.activeSwatch = null;
> +    this.activeProperty = null;
> +
> +    // Commit triggers expensive changes so we debounce it.

Maybe explain a little bit more what is expensive about it. Maybe just give a reference to the function that one would need to read to understand the expensive part of this.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:50
(Diff revision 4)
> +  * Using TextProperty instances from Rule view, store into a Map references to the
> +  * swatch element (toggle icon) and callbacks from TextPropertyEditor necessary to
> +  * preview and update the selected element's shape values in the Rule view.

I am finding this comment a bit hard to follow. I understand by reading the code and having a good knowledge of the inspector in general. But for people who don't have this knowledge, I think it would be good to try to rephrase a bit.
What does link mean here? What are we linking, and to what? Which direction does the link work? This kinds of things would be useful.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:86
(Diff revision 4)
> +    swatch.addEventListener("click", this._onSwatchClick);
> +    this.links.set(prop, { swatch, callbacks });
> +  }
> +
> +  /**
> +  * Remove references to swatch and callbacks from |this.links| for the given key.

A little bit of the same thing here. Although not as important, because if you understand what link means, then unlink is obvious.
But I do think that instead of documenting what the function does internally, it would be better to document the intent, the meaning.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:115
(Diff revision 4)
> +  * If the given TextProperty exists as a key in |this.links|, replace the reference it
> +  * has to the swatch element with a new node reference.

Please add a sentence here that explains when that happens, and why do we ever need to replace a swatch.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:161
(Diff revision 4)
> +    event.stopPropagation();
> +    for (let [prop, data] of this.links) {
> +      if (data.swatch == event.target) {
> +        this.activeSwatch = data.swatch;
> +        this.activeSwatch.classList.add("active");
> +        this.activeProperty = prop;

Don't we need to `break` out of the loop here? There can only be one link per swatch, right? In which case we don't really need to loop through the remaining items.
Attachment #8952207 - Flags: review?(pbrosset)
Attachment #8952207 - Flags: review?(gl)
Comment on attachment 8954523 [details]
Bug 1435373 - Rename and reorder methods and properties per review feedback.

I would just squash this.
Attachment #8954523 - Flags: review?(gl)
Blocks: 1443151
Attachment #8954523 - Attachment is obsolete: true
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review230166

::: commit-message-d0d36:8
(Diff revision 4)
> +- Wrap shape highlighter with in-context shape editor singleton
> +- Link text properties from Rule view into in-context shapes editor
> +- Write changes to corresponding rule instead of inline style
> +- Hide shape highlighter on node change or TextProperty disabled/overwritten
> +- Disable functionality to restore shape editor on refresh
> +- Refactor and consolidate shape point hover behavirour (still some loose ends)

> Also, could you describe what the loose ends are in the commit message?

There still is leftover logic related to synchronizing the highlighting the hovered coordinate points between the highlighter and rule view. Ideally, it should all be internal to the InContextShapeEditor. But this commit already turned into a beast and those are less important changes for now.

Loose ends are methods like:
- getShapeToggleActive() https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1574
- getShapePoint() https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1595
- isRuleViewShapePoint() https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/highlighters-overlay.js#753

There's too much logic spread out in many files for achieveing this behaviour. Some of it was dead code.

::: devtools/client/inspector/shared/highlighters-overlay.js:158
(Diff revision 4)
> +  async presistShapesHighlighterState(node, options) {
>      try {
>        // Save shapes highlighter state.
>        let { url } = this.inspector.target;
>        let selector = await node.getUniqueSelector();
>        this.state.shapes = { selector, options, url };
> -      this.shapesHighlighterShown = node;
> -      this.emit("shapes-highlighter-shown", node, options);
>      } catch (e) {
>        this._handleRejection(e);
>      }
>    }

I was still looking for a solution to keep the "restore shapes highlighter after refresh" workflow. But the more I think about it, the less it makes sense to do this. It adds needless complexity right now. The main UX snag is that temporary shape changes aren't persisted after refresh, so even if restoring worked, the user would be back editing the initial shape. 

Since *in-context editors* differ in behaviour from *highlighters*, I propose removing the restore logic for shapes for now and revisit if/when there's a demonstrated need for it. What do you think?

::: devtools/client/inspector/shared/highlighters-overlay.js:509
(Diff revision 4)
> +        let highlighter = await this._getHighlighter("ShapesHighlighter");
> +        if (!highlighter) {
> +          return null;
> +        }

I agree, the in-contex shapes editor should request its own shapes highlighter internally.

However, the shape highlighter instance stored in this.highlighters is expected and used in multiple places in the test files.

Even if requesting it internally to the in-context editor, it should still be exposed back to HighlighterOverlay.highlighters for tests to pass.

I'd prefer to handle this and updating that aspect of the tests when merging ShapesHighlighter with ShapesInContextEditor which requires a broader change.

::: devtools/client/inspector/shared/highlighters-overlay.js:515
(Diff revision 4)
> +        if (!highlighter) {
> +          return null;
> +        }
> +        const ShapesInContextEditor = require("devtools/client/shared/widgets/ShapesInContextEditor");
> +
> +        editor = new ShapesInContextEditor(highlighter, this.inspector, this.state);

It _should_ behave as a singleton and it _should_ be written accordingly. I was just following the existing examples in this file with regards to highlighters. 

I will handle this in a different patch for this bug I opened: https://bugzilla.mozilla.org/show_bug.cgi?id=1443151

::: devtools/client/inspector/shared/highlighters-overlay.js:855
(Diff revision 4)
> +      this.editors[type].off("show");
> +      this.editors[type].off("hide");

For the new EventListener, which the InContextShapesEditor uses, passing only the event name to "off" will remove all associated handlers.

See: https://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#60

::: devtools/client/shared/widgets/ShapesInContextEditor.js:10
(Diff revision 4)
> +"use strict";
> +
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const { debounce } = require("devtools/shared/debounce");
> +
> +class ShapesInContextEditor {

> Also, can you explain what your thoughts are regarding potential similar editors we might need in the future.

Yes, I thought about extracting the logic for an abstract in-context editor class for future reuse. Originally, I tried to extract that from [SwatchBasedEditorTooltip.js](https://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/tooltip/), but it turns out the swatch element being destroyed and rebuilt every time the TextPropertyEditor updates diverges behaviour of in-context editor from tooltips. (This is the reason for the additional .link() functionality in the current in-context editor). Ideally, we'd want a `SwatchBasedEditor` base class that can satisfy both tootips and long-lived editors (in-context or other panels).

> But these other editors will have partly the same needs (swatch click/link to prop), and other very different needs (maybe not a highlighter).

The main reason I didn't abstract just yet is because I'm not confident I have the right solution (with regards to .link() for mitigating the swatch destruction/reconstruction) and that I can't clearly foresee needs of other subclasses. I want to get more detail on this as I build the fonts editor which has overlapping needs, but isn't an in-context editor. 

Having at least two editors that need to communicate changes to the rule view will give better insight into the common functionality.

::: devtools/client/shared/widgets/ShapesInContextEditor.js:115
(Diff revision 4)
> +  * If the given TextProperty exists as a key in |this.links|, replace the reference it
> +  * has to the swatch element with a new node reference.

Addressed in jsdoc for .link() where this method is called.
(In reply to Razvan Caliman [:rcaliman][@rcaliman] from comment #20)
> I was still looking for a solution to keep the "restore shapes highlighter
> after refresh" workflow. But the more I think about it, the less it makes
> sense to do this. It adds needless complexity right now. The main UX snag is
> that temporary shape changes aren't persisted after refresh, so even if
> restoring worked, the user would be back editing the initial shape. 
> 
> Since *in-context editors* differ in behaviour from *highlighters*, I
> propose removing the restore logic for shapes for now and revisit if/when
> there's a demonstrated need for it. What do you think?
Works for me
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review231230

::: devtools/client/shared/widgets/ShapesInContextEditor.js:12
(Diff revisions 4 - 5)
>  const EventEmitter = require("devtools/shared/event-emitter");
>  const { debounce } = require("devtools/shared/debounce");
>  
> +/**
> + * The ShapesInContextEditor:
> + * - currently wraps the ShapesHighlighter actor (in the future it should replace it);

Maybe some rephrasing needed here. The ShapesHighlighter lives on the server side, while this ShapesInContextEditor live son the client side. So it will never replace it, right?

::: devtools/client/shared/widgets/ShapesInContextEditor.js:67
(Diff revisions 4 - 5)
>    /**
> -  * Using TextProperty instances from Rule view, store into a Map references to the
> -  * swatch element (toggle icon) and callbacks from TextPropertyEditor necessary to
> -  * preview and update the selected element's shape values in the Rule view.
> +  * The shapes in-context editor works by listening to shape value changes from the shapes
> +  * highlighter and mapping them to the correct CSS property in the Rule view.
> +  *
> +  * In order to know where to map changes, the TextPropertyEditor instances register
> +  * themseles in a map object internal to the ShapesInContextEditor.

s/themseles/themselves

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_01.js:41
(Diff revision 5)
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield onHighlighterArmed;

You can make this a one liner:
`yield highlighters.once("shapes-highlighter-armed");`

However, this works because we start listening right after having selected a node with `selectNode`, and because the event is sent asynchronously after the node was selected.

So there's some implicit time-sensitive test code here.
If, for some reason, we later add another async action after having selected a node, but before listening to this event, then the test would timeout, because the event would have already been fired by the time we started to listening to it.

So, I think that in each and every test you changed here to use this event, you should instead do something like this:

```
info("Selecting a node that has a shape css property");
let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
yield selectNode("#shape", inspector);
info("Waiting for the shapes in-context editor to be ready");
yield onHighlighterArmed;

```

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:36
(Diff revision 5)
> +/*
> +* Test that points hovered in the rule view will highlight corresponding points
> +* in the shapes highlighter on the page.
> +*/

I prefer writing comments in tests using `info(...)` statements, because this way the comments also serve as a way to read test logs (this is useful when tests  start to fail and you want to know where in the test is the failure).
It doesn't mean that this jsdoc shouldn't be here, but if you felt the need to add a comment here, I'm fairly sure it would also be useful as an `info(...)` in the code (maybe shorter).

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:61
(Diff revision 5)
> -  ok(pointSpan.classList.contains("active"), "Hovered span is active");
> -  is(highlighters.state.shapes.options.hoverPoint, "0",
> -     "Hovered point is saved to state");
> +  // No longer checking for `active` class on point node in rule view because
> +  // hover behaviour there is handled by CSS :hover pseudo-class, not JS.
> +  // Only check that corresponding point in shapes highlighter is marked.

I don't think this is needed. This comment assumes that the person reading the code knows that before your change, we used to check other things, and used to do hover with JS, and now we don't.
I don't think this is useful. You can just document what we do now, not imply what we used to do before.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:83
(Diff revision 5)
>  }
>  
> -function* highlightFromHighlighter(view, highlighters, testActor, helper) {
> +/*
> +* Test that points hovered in the shapes highlighter on the page will highlight
> +* corresponding points in the rule view.
> +* .

nit: empty line can be removed.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js:13
(Diff revision 5)
> -  let inspector = yield openInspectorForURL(TEST_URL);
> -  let helper = yield getHighlighterHelperFor(HIGHLIGHTER_TYPE)(inspector);
> -  let {testActor} = inspector;
> +  // let inspector = yield openInspectorForURL(TEST_URL);
> +  // let helper = yield getHighlighterHelperFor(HIGHLIGHTER_TYPE)(inspector);
> +  // let {testActor} = inspector;

Should be removed.

::: devtools/client/inspector/test/head.js:836
(Diff revision 5)
> +  const onHighlighterArmed = new Promise((resolve) => {
> +    highlighters.once("shapes-highlighter-armed", resolve);
> +  });

`once` already returns a promise. No need to wrap this into another level of promise.

::: devtools/client/inspector/test/head.js:836
(Diff revision 5)
> +  const onHighlighterArmed = new Promise((resolve) => {
> +    highlighters.once("shapes-highlighter-armed", resolve);
> +  });
> +  const hasSwatch = new Promise((resolve) => {
> +    if (highlighters &&
> +      highlighters.editors &&
> +      highlighters.editors[SHAPES_IN_CONTEXT_EDITOR] &&
> +      highlighters.editors[SHAPES_IN_CONTEXT_EDITOR].hasSwatch(shapesToggle)) {
> +      resolve();
> +    }
> +  });
> +
> +  // On first call, a shape highlighter instance may exist, but no swatches that trigger
> +  // it may be associated yet. Any swatch that gets associated with it will "arm" the
> +  // highlighter so that clicks on them will trigger the highlighter.
> +  // On subsequent calls, the swatch may exist so the arming event will not be triggered.
> +  // Here we race both conditions to proceed as soon as one is satisfied.
> +  info("Wait for shapes highlighter swatch to be ready");
> +  yield Promise.race([onHighlighterArmed, hasSwatch]);

The `hasSwatch` promise looks a bit odd to me, because it doesn't have any async code in it.
I'm wondering if we couldn't rewrite this in a bit of a simpler way:

```
if (!highlighters.editors[SHAPES_IN_CONTEXT_EDITOR] ||
    !highlighters.editors[SHAPES_IN_CONTEXT_EDITOR].hasSwatch(shapestoggle)) {
  yield highlighters.once("shapes-highlighters-armed");
}
```

::: devtools/client/inspector/test/head.js:861
(Diff revision 5)
> +  let metaKey = options.transformMode;
> +  let ctrlKey = options.transformMode;
> +
>    if (show) {
>      let onHighlighterShown = highlighters.once("shapes-highlighter-shown");
> -    shapesToggle.click();
> +    // shapesToggle.click();

Please remove this commented-out code.

::: devtools/client/inspector/test/head.js:867
(Diff revision 5)
> +    EventUtils.sendMouseEvent({type: "click", metaKey, ctrlKey },
> +      shapesToggle, view.styleWindow);
>      yield onHighlighterShown;
>    } else {
>      let onHighlighterHidden = highlighters.once("shapes-highlighter-hidden");
>      shapesToggle.click();

Should this `click` be removed since you now use the `EventUtils` lib instead?
Attachment #8952207 - Flags: review?(pbrosset)
Attachment #8952207 - Flags: review?(gl)
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review231610
Attachment #8952207 - Flags: review?(pbrosset) → review+
Try link after latest changes to reduce intermittent test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72fd05682e28d530b74d976e30b50cf53a6a8e06
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review232290

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js
(Diff revisions 7 - 8)
> -  let onRuleViewChanged = view.once("ruleview-changed");
>    info("Moving ellipse rx");
>    yield mouse.down(rxPixel, cyPixel, selector);
>    yield mouse.move(rxPixel + dx, cyPixel, selector);
>    yield mouse.up(rxPixel + dx, cyPixel, selector);
>    yield testActor.reflow();
> -  yield onRuleViewChanged;

Don't we also need to listen for shapes-highlighter-changes-applied for this block of code?
Technically, both should work. The "shapes-highlighter-changes-applied" is fired _after_ "ruleview-changed". 

In high-frequency changes, like in a for loop, I've observed that sometimes "ruleview-changed" may be debounced. That's why I added the "shapes-highlighter-changes-applied" event to isolate the sequence of events. 

In this code block, I've not seen the issue.
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review232836
Keywords: checkin-needed
There were some devtools code refactorings yesterday and the patches don't apply anymore. Please fix those conflicts and submit updated patches. A Try push would also be a good idea because e.g. the linting rules changed. Thank you.
Flags: needinfo?(rcaliman)
Keywords: checkin-needed
Fixed merge conflicts and adapt to new linter rules.

Latest try run: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6df3b59118e9cf3f5e0cb61596b294cf6fb19f8f
Flags: needinfo?(rcaliman)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a22276dc53
Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/195379cf14f0
Map changes from shape highlighter to correct CSS rules. r=pbro
Keywords: checkin-needed
Backed out for devtools failures at devtools/client/shared/test/browser_telemetry_button_eyedropper.js and devtools/client/inspector/rules/test/browser_rules_eyedropper.js and devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=195379cf14f053d716d904b75ce4c00d8046e32a

Failure logs:

browser_telemetry_button_eyedropper.js: https://treeherder.mozilla.org/logviewer.html#?job_id=168039651&repo=mozilla-inbound&lineNumber=11079

devtools/client/inspector/rules/test/browser_rules_eyedropper.js: https://treeherder.mozilla.org/logviewer.html#?job_id=168046083&repo=mozilla-inbound&lineNumber=13253

devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js: https://treeherder.mozilla.org/logviewer.html#?job_id=168057054&repo=mozilla-inbound&lineNumber=2201



Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/41df1e7a38d533b52624c5529ec387828a34cb22
Flags: needinfo?(rcaliman)
(In reply to Andreea Pavel [:apavel] from comment #42)

> devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:
> https://treeherder.mozilla.org/logviewer.html#?job_id=168057054&repo=mozilla-
> inbound&lineNumber=2201

It looks to me like this test is timing out because of a failure to
> yield testActor.reflow()
before
> yield onShapeChangeApplied;
in testCircleMoveCenter().

I'll see if I can discern anything with the other failing tests.
The failure logs show that this patch is triggering some code paths that are not adequately protected with asserts and early exits. I've posted two parts that will cover up some of problem areas, hopefully revealing the true problems. Razvan, I encourage you to rebase your patches and apply these two parts on top, then trigger another multi-platform try run. If you do a "./mach try fuzzy" then the query "devtools debug" will get just the tests that failed on your attempted landing. Maybe we can learn more with those results.
For now, I'll do my own try push with my rebasing of the original patches.
Comment on attachment 8961974 [details]
Bug 1435373 Part 5: Update browser_inspector_highlighter-cssshape_04.js to wait on reflow after mouse moves, before waiting on shape changes.

https://reviewboard.mozilla.org/r/230820/#review236312


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:204
(Diff revision 1)
>    info("Moving circle center");
>    let { mouse } = helper;
>    yield mouse.down(cxPixel, cyPixel, selector);
>    yield mouse.move(cxPixel + dx, cyPixel + dy, selector);
>    yield mouse.up(cxPixel + dx, cyPixel + dy, selector);
> +  yield testActor.reflow()

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8961972 [details]
Bug 1435373 Part 2: Map changes from shape highlighter to correct CSS rules.

The issue that this change addresses has been split out to Bug 1451098.
Attachment #8961972 - Attachment is obsolete: true
Comment on attachment 8961971 [details]
Bug 1435373 Part 1: Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision.

The issues that this patch addresses has been split out to Bug 1451115.
Attachment #8961971 - Attachment is obsolete: true
Attachment #8961974 - Attachment is obsolete: true
Comment on attachment 8961972 [details]
Bug 1435373 Part 2: Map changes from shape highlighter to correct CSS rules.

https://reviewboard.mozilla.org/r/230814/#review239444


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/shared/highlighters-overlay.js:31
(Diff revision 2)
>    constructor(inspector) {
> -    this.inspector = inspector;
> +     /*
> +      * Collection of instantiated highlighter actors like FlexboxHighlighter,
> +      * CssGridHighlighter, ShapesHighlighter and GeometryEditorHighlighter.
> +      */
> -    this.highlighters = {};
> +     this.highlighters = {};

Error: Expected indentation of 4 spaces but found 5. [eslint: indent-legacy]

::: devtools/client/inspector/shared/highlighters-overlay.js:34
(Diff revision 2)
> +      * CssGridHighlighter, ShapesHighlighter and GeometryEditorHighlighter.
> +      */
> -    this.highlighters = {};
> +     this.highlighters = {};
> +     /*
> +      * Collection of instantiated in-context editors, like ShapesInContextEditor, which
> +      * behave like highlighters but with added editing capabilities that need to map value

Error: Line 34 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:144
(Diff revision 2)
> -async function testPolygonRemovePoint(testActor, helper) {
> -  await helper.show("#polygon", {mode: "cssClipPath"});
> -  let { highlightedNode } = helper;
> +async function testPolygonRemovePoint(config) {
> +  const {inspector, highlighters, testActor, helper} = config;
> +  const selector = "#polygon";
> +  const property = "clip-path";
> +
> +  yield setup({selector, property, ...config});

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:75
(Diff revision 2)
> -
>    markerHidden = await testActor.getHighlighterNodeAttribute(
>      "shapes-marker-hover", "hidden", highlighterFront);
>    ok(markerHidden, "Marker on highlighter is not visible");
>  
> -  info("Hide shapes highlighter");
> +  yield teardown({selector, property, ...config});

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_06.js:37
(Diff revision 2)
> +  const { testActor, helper, highlighters } = config;
> +  const options = { transformMode: true };
> +  const property = "clip-path";
> +
> +  for (let selector of SHAPE_SELECTORS) {
> +    yield setup({selector, property, options, ...config});

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_07.js:27
(Diff revision 2)
>  });
>  
> -async function testOneDimScale(testActor, helper) {
> -  for (let shape of SHAPE_IDS) {
> -    info(`Displaying ${shape}`);
> -    await helper.show(shape, {mode: "cssClipPath", transformMode: true});
> +async function setup(config) {
> +  const { inspector, view, selector, property, options } = config;
> +  await selectNode(selector, inspector);
> +  return await toggleShapesHighlighter(view, selector, property, true, options);

Error: Redundant use of `await` on a return value. [eslint: no-return-await]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js:45
(Diff revision 2)
>    await mouse.move(20, 20);
>    await mouse.up();
>    await testActor.reflow();
> +  await onRuleViewChanged;
>  
> -  let computedStyle = await highlightedNode.getComputedStyle();
> +  let computedStyle = yield inspector.pageStyle.getComputed(highlightedNode);

Error: Parsing error: the keyword 'yield' is reserved [eslint]
Comment on attachment 8952208 [details]
Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision.

Obsoleted by attachment 8961971 [details].
Attachment #8952208 - Attachment is obsolete: true
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

Obsoleted by attachment 8961972 [details].
Attachment #8952207 - Attachment is obsolete: true
Razvan, I took the liberty of attempting a rebasing of your patches. It was pretty messy and I'm not sure the results are correct. I'm going to use these patches as the basis for a test of Bug 1451098.
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review239448


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_01.js:30
(Diff revision 12)
>    let {inspector, view} = await openRuleView();
>    let highlighters = view.highlighters;
>  
> -  await selectNode("#shape", inspector);
> +  info("Select a node with a shape value");
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_03.js:30
(Diff revision 12)
>    let {inspector, view} = await openRuleView();
>    let highlighters = view.highlighters;
>  
>    info("Selecting the first shape container.");
> -  await selectNode("#shape1", inspector);
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape1", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_04.js:28
(Diff revision 12)
>    let {inspector, view} = await openRuleView();
>    let highlighters = view.highlighters;
>  
> -  await selectNode("#shape", inspector);
> +  info("Select a node with a shape value");
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_05.js:28
(Diff revision 12)
>    let {inspector, view, testActor} = await openRuleView();
>    let highlighters = view.highlighters;
>  
> -  await selectNode("#shape", inspector);
> +  info("Select a node with a shape value");
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_06.js:30
(Diff revision 12)
>    let {inspector, view} = await openRuleView();
>    let highlighters = view.highlighters;
>  
> -  await selectNode("#shape1", inspector);
> +  info("Selecting the first shapes container.");
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape1", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/rules/test/browser_rules_shapes-toggle_07.js:29
(Diff revision 12)
>    let {inspector, view} = await openRuleView();
>    let highlighters = view.highlighters;
>  
> -  await selectNode("#shape", inspector);
> +  info("Select a node with a shape value");
> +  let onHighlighterArmed = highlighters.once("shapes-highlighter-armed");
> +  yield selectNode("#shape", inspector);

Error: Parsing error: the keyword 'yield' is reserved [eslint]

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:51
(Diff revision 12)
> +
> +  let container = getRuleViewProperty(view, selector, property).valueSpan;
>    let shapesToggle = container.querySelector(".ruleview-shapeswatch");
>  
>    let highlighterFront = highlighters.highlighters[HIGHLIGHTER_TYPE];
>    let markerHidden = await testActor.getHighlighterNodeAttribute(

Error: Parsing error: unexpected token testactor [eslint]
Attachment #8961971 - Attachment is obsolete: true
Attachment #8961972 - Attachment is obsolete: true
Attachment #8961972 - Flags: review?(pbrosset)
Attachment #8965092 - Flags: review?(pbrosset)
Attachment #8965093 - Attachment is obsolete: true
Attachment #8965093 - Flags: review?(pbrosset)
Attachment #8965094 - Attachment is obsolete: true
Attachment #8965094 - Flags: review?(pbrosset)
Blocks: 1424939
Comment on attachment 8965092 [details]
Bug 1435373 - Shapes editor: update tests for new implementation.

https://reviewboard.mozilla.org/r/233788/#review240058

I have one main comment about this: it looks like you rebased an old commit from before the big generator/yield -> async/await conversion, and in the process brought back a lot of the old code unfortunately. 
We should fix the rebase so that the newer async/await changes come back.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:57
(Diff revision 2)
> +  let points = yield testActor.getHighlighterNodeAttribute(
> +    "shapes-polygon", "points", highlighters.highlighters[HIGHLIGHTER_TYPE]);

Can you explain to me why we're not using the simpler form `helper.getElementAttribute` here (and in other parts of this commit)?

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:13
(Diff revision 2)
> -
>  const HIGHLIGHTER_TYPE = "ShapesHighlighter";
> -const CSS_SHAPES_ENABLED_PREF = "devtools.inspector.shapesHighlighter.enabled";
>  
> -add_task(async function() {
> -  await pushPref(CSS_SHAPES_ENABLED_PREF, true);
> +add_task(function* () {
> +  let env = yield openInspectorForURL(TEST_URL);

Is this already done in head.js perhaps?
Attachment #8965092 - Flags: review?(pbrosset)
Razvan said he found the root cause for the leak detected on try, so I'll wait for him to push the fix here before reviewing the other commit.
Comment on attachment 8965092 [details]
Bug 1435373 - Shapes editor: update tests for new implementation.

https://reviewboard.mozilla.org/r/233788/#review240058

> Can you explain to me why we're not using the simpler form `helper.getElementAttribute` here (and in other parts of this commit)?

We can't use `getHighlighterHelperFor()` to instrument the shape highlighters tests. The shapes highighter show/hide methods have additional side effects like selecting the textProperty for where changes will map. This is neceessary in order to test if property values are correctly updated. 

The nodeFront is set internally in the helper as a result of the show() call (which we can't rely on for the shapes highlighter).

One way to fix this is to extend `getHighlighterHelperFor()` with methods that can set the highlighter and nodeFront instances from the oustide so we can delegate methods like `getElementAttribute()` to the helper, but circumvent its the show() / hide() methods. 

It may be confusing when future people will come to these tests see the helper being used, not understand it's used just partially, and try to use show() / hide() from the helper and have tests fail because the side-effects don't happen anymore.
Comment on attachment 8965092 [details]
Bug 1435373 - Shapes editor: update tests for new implementation.

https://reviewboard.mozilla.org/r/233788/#review240112
Attachment #8965092 - Flags: review?(pbrosset) → review+
See Also: → 1450980
See Also: → 1450671
See Also: → 1443151
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review240118

I didn't spend too much time on this one, since I had R+'d a previous iteration of this patch, and because Razvan and I have talked about the approach several times in the past already.
Just one thing before R+'ing this, see the comment below. If this try turns out green consistently, then we should do the suggested change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6bfbf9411eebfc5876c24536e0690150ccf54aa&group_state=expanded

::: devtools/client/inspector/shared/highlighters-overlay.js:886
(Diff revision 14)
>    }
>  
>    /**
> -   * Destroy this overlay instance, removing it from the view and destroying
> -   * all initialized highlighters.
> +  * Destroy and clean-up all instances of in-context editors.
> +  */
> +  async destroyEditors() {

As discussed, this isn't called from destroy anymore because this was causing the leak.
I think we should add it back, but inside the editor, don't await on `this.hide` in destroy because that may never resolve if the server has been stopped already when the test ended.
Attachment #8952207 - Flags: review+
Comment on attachment 8952207 [details]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule.

https://reviewboard.mozilla.org/r/221458/#review240580
Attachment #8952207 - Flags: review?(pbrosset) → review+
Re-work of the patch is done. Implemented Patrick's recommendations. Leaks and test failures are now resolved.
Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7427f67002b0a28cf7cf443e300b7a7a1c6b5f
Flags: needinfo?(rcaliman)
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3b0f6497bb7
Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision. r=gl
https://hg.mozilla.org/integration/autoland/rev/0ea578dacf23
Shapes editor: update tests for new implementation. r=pbro
Keywords: checkin-needed
This got backed out because one of the patches failed to apply.
We imported them, hg log showed all of them, however after the push was done only 2 of them reached autoland. 
:rcaliman can you take a look?
Flags: needinfo?(rcaliman)
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc0d5bbaed8b
Backed out 2 changesets for patch failing to be applied on a CLOSED TREE
Attachment #8966539 - Attachment description: Minor refactor for shape output string guarding against empty this.geometryBox. → Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox.
Comment on attachment 8966539 [details] [diff] [review]
Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. r=gl

Re-upload of same patch. Forwarding r+ from previous.
Attachment #8966539 - Attachment description: Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. → Bug 1435373 - Minor refactor for shape output string guarding against empty this.geometryBox. r=gl
Attachment #8966539 - Flags: review+
Comment on attachment 8966540 [details] [diff] [review]
Bug 1435373 - Shapes editor: implementation to map shape value changes to rule. r=pbro

Re-upload of same patch. Forwarding r+ from previous.
Attachment #8966540 - Flags: review+
Comment on attachment 8966541 [details] [diff] [review]
Bug 1435373 - Shapes editor: update tests for new implementation. r=pbro

Re-upload of same patch. Forwarding r+ from previous.
Attachment #8966541 - Attachment description: Bug 1435373 - Shapes editor: update tests for new implementation. → Bug 1435373 - Shapes editor: update tests for new implementation. r=pbro
Attachment #8966541 - Flags: review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dac79a5a4a5
Minor refactor for shape output string guarding against empty this.geometryBox. Round values in getDistance() util function to avoid verbose precision.r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc52ed730824
Shapes editor: implementation to map shape value changes to rule. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f53df7f400
Shapes editor: update tests for new implementation.r=pbro
I did a clean branch from the latest master and cherry-picked each one of the three commits onto it, then uploaded here as patches again. 

I suspect the failure occurred because the previous branch was rebased from a master that had another change backed-out in the mean time.
https://hg.mozilla.org/mozilla-central/rev/4dac79a5a4a5
https://hg.mozilla.org/mozilla-central/rev/bc52ed730824
https://hg.mozilla.org/mozilla-central/rev/b9f53df7f400
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1452986
Build ID: 20180528091514
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox Nightly 62.0a1 and Firefox 61.0b9  on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
status-firefox62: --- → verified
Depends on: 1466225
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: