Closed
Bug 986060
Opened 10 years ago
Closed 10 years ago
Bubble the reflow "interruptible" flag in the developer-hud-update event
Categories
(DevTools Graveyard :: WebIDE, defect)
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.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → janx
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #1) > Any news? :) Here are some news! :)
Status: NEW → ASSIGNED
Flags: needinfo?(janx)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8406050 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/26a034a63189
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/26a034a63189
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•