1219544 - Pass in a callback to open source in debugger rather than toolbox

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

In bug 1219073, we pass the toolbox into the heap tree for rendering, but we should pass in a onClick callback, wrapping toolbox in a closure. A small nice refactoring.

Comment 1

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

I want to use the frame component in jit view, so taking care of this

Comment 2

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

Attachment: 1219544-decouple-frame-view.patch

Happy holidays! Here's a huge, confusing patch.

* Frame view component pulled out of memory tool (and onClick function and L10N things are passed in now)
* Previous "shared/source-utils.js" which handles opening a source in different tools now moved to "shared/view-source.js" to be more descriptive
* "shared/source-utils.js" now contains a handful of source utilities from performance's "performance/modules/logic/frame-utils.js".

This paves the way for using this component in jit view, perf call tree and console, etc., which gives us a common component to handle source mapping as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bec32e4d2e9d

Comment 3

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

Also normalized our usage of host vs hostname, etc., was used inconsistently.

Something to look over/think about is our terminology of location, frame, and source, and different representations of each (like call tree for example doesn't used SavedFrames, but generates it out of a string). I think it's consistent for the most part now, but "parseLocation" in frame-utils maybe should be something different, like stackToFrame or something.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8701592 [details] [diff] [review]
1219544-decouple-frame-view.patch

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

Thanks for cleaning this up! r=me with the change of name to fit in line with all of our other event handlers.

::: devtools/client/memory/app.js
@@ +97,5 @@
>            HeapView({
>              snapshot: selectedSnapshot,
>              diffing,
>              onSnapshotClick: () => dispatch(takeSnapshotAndCensus(front, heapWorker)),
> +            linkToDebugger: (frame) => toolbox.viewSourceInDebugger(frame.source, frame.line),

Nit: no need for () around single param.

::: devtools/client/memory/components/heap.js
@@ +90,5 @@
>  
>    propTypes: {
>      onSnapshotClick: PropTypes.func.isRequired,
>      snapshot: snapshotModel,
> +    linkToDebugger: PropTypes.func.isRequired,

All of our event handlers are named "on<action>", and so for consistency this should be named something like "onViewSourceInDebugger".

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0dc1691ab48d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dc1691ab48d

Comment 7

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected