Closed Bug 1175686 Opened 5 years ago Closed 5 years ago

Pull line and bar graph widgets outside of Graphs.js into their own files

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: vporof, Assigned: vporof)

Details

Attachments

(2 files)

No description provided.
Attached patch line graphSplinter Review
Attachment #8623882 - Flags: review?(jsantell)
Attached patch bar graphSplinter Review
Attachment #8623883 - Flags: review?(jsantell)
Comment on attachment 8623883 [details] [diff] [review]
bar graph

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

I don't see where this creates BarGraphWidget.js
Comment on attachment 8623882 [details] [diff] [review]
line graph

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

r+, but I prefer line-bar-graph over LineBarGraph w/r/t file names to be more in line with node/commonJS modules, rather than seemingly more JSM style of capitalized file names
Attachment #8623882 - Flags: review?(jsantell) → review+
Comment on attachment 8623883 [details] [diff] [review]
bar graph

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

R+ if just moving bar graph into a new file (but include it in patch)
Attachment #8623883 - Flags: review?(jsantell) → review+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Comment on attachment 8623882 [details] [diff] [review]
> line graph
> 
> Review of attachment 8623882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, but I prefer line-bar-graph over LineBarGraph w/r/t file names to be
> more in line with node/commonJS modules, rather than seemingly more JSM
> style of capitalized file names

We're super inconsistent in that dir at the moment. Should do proper naming in a followup for all files.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> Comment on attachment 8623883 [details] [diff] [review]
> bar graph
> 
> Review of attachment 8623883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+ if just moving bar graph into a new file (but include it in patch)

Whoops!
Whiteboard: [fixed-in-fx-team]
Backed out for ongoing failures. Please verify that this is fully green on Try before pushing again.
https://treeherder.mozilla.org/logviewer.html#?job_id=3515723&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/288cd0b9c9a3
https://hg.mozilla.org/mozilla-central/rev/96669be6be1f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.