Closed Bug 986060 Opened 6 years ago Closed 5 years ago

Bubble the reflow "interruptible" flag in the developer-hud-update event

Categories

(DevTools :: WebIDE, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: etienne, Assigned: janx)

References

Details

Attachments

(1 file, 1 obsolete file)

We'd like to use it from the ReflowHelper in gaia integrations tests.
Assignee: nobody → janx
Blocks: 986062
Any news? :)
Flags: needinfo?(janx)
(In reply to Etienne Segonzac (:etienne) from comment #1)
> Any news? :)

Here are some news! :)
Status: NEW → ASSIGNED
Flags: needinfo?(janx)
Comment on attachment 8406050 [details] [diff] [review]
Bubble the reflow interruptible flag in the developer-hud-update event. r=ochameau

Alex, could you please have a look? I slightly changed the event API, sending an object instead of a string.

Caveat: This will break https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/reflow.js so the test will need to be patched before this can go forward.
Attachment #8406050 - Flags: review?(poirot.alex)
Depends on: 995946
Comment on attachment 8406050 [details] [diff] [review]
Bubble the reflow interruptible flag in the developer-hud-update event. r=ochameau

Review of attachment 8406050 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/devtools.js
@@ -209,5 @@
>    /**
>     * Modify one of a target's metrics, and send out an event to notify relevant
>     * parties (e.g. the developer HUD, automated tests, etc).
>     */
> -  update: function target_update(metric, value = 0, message) {

That was really weird to have a default value in second argument and not on the 3rd!

@@ +216,5 @@
> +    if (!metric.value) {
> +      metric.value = 0;
> +    }
> +
> +    metrics.set(metric.name, metric.value);

What about adding an explicit check against metric.name,
to ensure it is set?
if (!metric.name) {
  throw new Error("Missing metric.name");
}

@@ +228,5 @@
>  
>      // FIXME(Bug 982066) Remove this loop.
>      if (metrics && metrics.size > 0) {
>        for (let name of metrics.keys()) {
>          data.metrics.push({name: name, value: metrics.get(name)});

Don't you also need to update data.metrics to contain the new interuptible field?

@@ +243,5 @@
>     * Nicer way to call update() when the metric value is a number that needs
>     * to be incremented.
>     */
>    bump: function target_bump(metric, message) {
> +    metric.value = this.metrics.get(metric.name) + 1;

Shouldn't we check for this.metrics.get() before doing the sum?
Otherwise the first time we will call this method, metric.value will be undefined instead of 1.
Attachment #8406050 - Flags: review?(poirot.alex) → review+
Thanks for the r+!

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> Comment on attachment 8406050 [details] [diff] [review]
> Bubble the reflow interruptible flag in the developer-hud-update event.
> r=ochameau
> 
> Review of attachment 8406050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/devtools.js
> @@ -209,5 @@
> >    /**
> >     * Modify one of a target's metrics, and send out an event to notify relevant
> >     * parties (e.g. the developer HUD, automated tests, etc).
> >     */
> > -  update: function target_update(metric, value = 0, message) {
> 
> That was really weird to have a default value in second argument and not on
> the 3rd!

Technically, the default value of `message` was undefined, which worked well with the `if (message)` on line 232.

> @@ +216,5 @@
> > +    if (!metric.value) {
> > +      metric.value = 0;
> > +    }
> > +
> > +    metrics.set(metric.name, metric.value);
> 
> What about adding an explicit check against metric.name,
> to ensure it is set?
> if (!metric.name) {
>   throw new Error("Missing metric.name");
> }

Maybe that could help catch potential new bugs faster. I'll add a check.

> @@ +228,5 @@
> >  
> >      // FIXME(Bug 982066) Remove this loop.
> >      if (metrics && metrics.size > 0) {
> >        for (let name of metrics.keys()) {
> >          data.metrics.push({name: name, value: metrics.get(name)});
> 
> Don't you also need to update data.metrics to contain the new interuptible
> field?

No need to do that: `data.metrics` is only used to update the Developer HUD widgets and will be deprecated once the HUD updates on a per-widget basis (bug 982066).

> @@ +243,5 @@
> >     * Nicer way to call update() when the metric value is a number that needs
> >     * to be incremented.
> >     */
> >    bump: function target_bump(metric, message) {
> > +    metric.value = this.metrics.get(metric.name) + 1;
> 
> Shouldn't we check for this.metrics.get() before doing the sum?
> Otherwise the first time we will call this method, metric.value will be
> undefined instead of 1.

The way things are set up, metrics are first "registered" by being affected a value of 0 in `this.metrics`. However, I'm willing to add a check just to make things more flexible for future watcher variants.
Comment on attachment 8407584 [details] [diff] [review]
Bubble the reflow interruptible flag in the developer-hud-update event. r=ochameau

Review of attachment 8407584 [details] [diff] [review]:
-----------------------------------------------------------------

Alex's r+
Attachment #8407584 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26a034a63189
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.