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

Status: RESOLVED FIXED

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

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: line graph

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: bar graph

Comment 3

[Jordan Santell [:jsantell] [@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 4

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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)

Comment 6

[Victor Porof [:vporof][:vp]]

(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!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8c6b76badad2
https://hg.mozilla.org/integration/fx-team/rev/fc161a706ebf

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/21126200b02f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/96669be6be1f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96669be6be1f