Bug 725235 adds an inspection popup when hovering a variable. It would be a good addition to add a "Watch" button (or link) that would add the currently inspected variable to the variable watch list.

I want to work on this bug. Can anyone tell me how to proceed?

This should get you started: https://wiki.mozilla.org/DevTools/Hacking I'll let Victor give the specifics with regards to this bug.

Hi Lalit, thanks for the interest! This bug is about enhancing the popup displayed when hovering an identifier in the debugger. We call it the "variables view popup" and it displays an object's properties. Say you're hovering the "foo" in a foo.bar declaration. It'd be helpful to have a button in the displayed popup that creates a "foo.bar" watch expression. First thing first, let me know if you have any experience in building Firefox from source. The link Nick gave you should be a good introduction on how to get started. Secondly, you'll need to think about where exactly in the popup it will make sense to add the "Watch" button that creates the watch expression. Third thing is implementation :) This [0] is the code in the debugger that creates a variables view inspection popup after hovering an identifier in the source editor. It's part of the "VariableBubbleView" object in the frontend, dealing with this view. The actual contents (properties etc.) are displayed in a VariablesView [1] instance. The code dealing with the popup container is here: [2], and the method responsible with creating a VariablesView inside the Tooltip is here: [3]. I think the easiest way to fix this bug is to enhance `setVariableContent` in Tooltip.js, making it have an option of creating buttons inside the popup, beneath the variables view. An "extraButtons" argument after "relayEvents" is the best approach. Consumers of this API will probably have to use it like this: tooltip.setVariableContent(objectActor, { // view options }, { // controller options }, { // relay events }, [ // here are the extra buttons { label: "Watch", command: function() {} } ]); [0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1857 [1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm [2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js [3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#465

Hi Victor, thanks for replying back! Yes, I have successfully built Firefox from source. I am not exactly sure where to put the watch button. And I think your approach allows API consumer to add watch button for complete object and not it's individual properties. Below I have described the problem in detail:- Suppose there is an object foo in some script.js file. // Object foo var foo = { 'bar': '0', 'bar_1': '1', 'bar_2': '2' }; If I hover over foo object, I see a popup with 'foo' value('bar':'0', 'bar_1':'1' etc.) in it. Now, there are two possible cases: 1. User wants to add watch expression for complete object 'foo' on hovering. This looks easy as I have to add only one watch button, but I am not sure of the position of watch button. 2. User wants to add watch expression for some of the properties(say 'bar') of foo object on hovering. This may require adding more than one watch button either on left or right of object property. Am I thinking about it the right way?

I don't think we want to complicate ourselves with adding buttons on each property in the variables view, which will clutter things and not provide *that* much benefit. A single "Watch" button is more than enough, especially since comment 0 also specifies that the "currently inspected variable" should be added to the watch expressions list.

(In reply to Lalit Khattar from comment #4) > > 1. User wants to add watch expression for complete object 'foo' on hovering. > This looks easy as I have to add only one watch button, but I am not sure of > the position of watch button. The "Watch" button should be placed below the variables view, as explained in comment 3. It will be inside the popup, but underneath all the properties, close to the mouse cursor when the popup opens.

I agree fully with comment 5 :) Watching an object obviously also implicitly watches its properties: if the user unfolds the object in the watch panel, he'll see the properties. Then the object stays unfolded so the use case is fulfilled :)

@Victor: Can you please elaborate on the implementation part as I am not familiar with the code base much. For eg. I don't know if there is method to create button.

The UI is built in XUL, so I suggest you read a bit about it at [0] and [1]. A XUL button is described in detail here: [2]. You'll have to create the button similarly to <html:input> elements via createElement("button") and add the necessary event listener and attributes, like the label, to the button. The element should be added to the .devtools-tooltip-variables-view-box most likely, or one of the nodes inside the tooltip so that the button appears underneath the variables view. Be careful, there's no top-level "document" in javascript modules like Tooltip.js. In the particular `setVariableContent` method, the document is available via this.doc. Make sure the button label is also localized, by adding a string to the debugger.properties file and accessing it via L10N.getStr in the debugger frontend. [0] https://developer.mozilla.org/en/docs/XUL [1] https://developer.mozilla.org/en-US/docs/XUL/Tutorial [2] https://developer.mozilla.org/en-US/docs/XUL/button

I have added the code for create button but the button is only appearing for non primitive variable value e.g. an object. I think it's because tooltip.setVariableContent is called for only non-primitive value. See code [0] Should I pass "extraButtons" argument in tooltip.setTextContent? [0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1868

I have one more question. The 'Watch' button I just created, how should it look? Is there any design specification?

(In reply to Lalit Khattar from comment #10) > > Should I pass "extraButtons" argument in tooltip.setTextContent? Sure, that seems reasonable to me. (In reply to Lalit Khattar from comment #11) > > The 'Watch' button I just created, how should it look? Is there any design > specification? Making it look like all the other buttons sounds like a good idea to me. We have some styles that we generally use with buttons, in toolbars.inc.css[0]. You should use those classes too. [0] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#18

In my thought I was thinking of more a "link" style to make the popup look less cluttered. Just my 2 cents :)

(In reply to Julien Wajsberg [:julienw] from comment #13) > In my thought I was thinking of more a "link" style to make the popup look > less cluttered. Just my 2 cents :) Sure, that would work too!

Hey Victor, There is a small problem with addExpression method. It only fills the textbox with 'aExpression' value and doesn't hit enter key automatically(which I think is required in this case so that user doesn't need to hit enter after clicking 'Watch' button). Let me know which one is desirable(with enter or without enter) and I will be uploading my first patch in bugzilla soon :)

(In reply to Lalit Khattar from comment #15) > > There is a small problem with addExpression method. It only fills the > textbox with 'aExpression' value and doesn't hit enter key > automatically(which I think is required in this case so that user doesn't > need to hit enter after clicking 'Watch' button). You could just restore focus to the editor via DebuggerView.editor.focus(); for it to have the same effect.

This is the first version of patch. I will make further changes to it, if required :)

Comment on attachment 8366128 [details] [diff] [review] Proposed patch version 0. Review of attachment 8366128 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, great progress! Thank you! Julien is right though, a button this large is a little too much "in your face", we should try something more subtle, that sort of looks like a link. Let me know if you need help with building a test for this. Try browsing some of the debugger tests, clone one and modify it to suit your needs. ::: browser/devtools/debugger/debugger-panes.js @@ +1872,5 @@ > messages: [textContent], > messagesClass: className, > containerClass: "plain" > + }, [ > + { Nit: please format this like so: }, [{ label: ... command: ... }]); @@ +1875,5 @@ > + }, [ > + { > + label: L10N.getStr('addWatchExpressionButton'), > + command: () => { > + let aExpression = this._markedText.anchor.textContent; This won't always contain the all necessary code used in evaluation. Use evalPrefix instead. Nit: no need to name the variable aExpression since it's not an argument in a function. @@ +1902,5 @@ > if (aType == "properties") { > window.emit(EVENTS.FETCHED_BUBBLE_PROPERTIES); > } > } > + }, [ Ditto on formatting. ::: browser/devtools/shared/widgets/Tooltip.js @@ +436,5 @@ > description.textContent = text; > vbox.appendChild(description); > } > > + for(let i=0; i<extraButtons.length; i++) { You could (and should) iterate this array more elegantly, since you don't really need a counter. for (let { label, command } of extraButtons) { ... } @@ +437,5 @@ > vbox.appendChild(description); > } > > + for(let i=0; i<extraButtons.length; i++) { > + let button = this.doc.createElement("button") Add a semicolon at the end of the line. @@ +438,5 @@ > } > > + for(let i=0; i<extraButtons.length; i++) { > + let button = this.doc.createElement("button") > + button.className = 'devtools-toolbarbutton' Semicolon! :) As written earlier, the toolbarbutton class doesn't seem to work well, it makes the popup a bit bloated. Make the class name an option as well, like the label and the command, and define the style in debugger.inc.css. There, remove all button styling, leaving only the label (no border or background etc.), and color it blue. Here's the default colors: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors "Hightlight blue" should work nicely. @@ +440,5 @@ > + for(let i=0; i<extraButtons.length; i++) { > + let button = this.doc.createElement("button") > + button.className = 'devtools-toolbarbutton' > + button.setAttribute("label", extraButtons[i].label); > + button.addEventListener('click', extraButtons[i].command); If it works, use the "command" event instead of click, to allow other input methods accessibility. @@ +496,5 @@ > innerbox.className = "devtools-tooltip-variables-view-innerbox"; > innerbox.setAttribute("flex", "1"); > vbox.appendChild(innerbox); > > + for(let i=0; i<extraButtons.length; i++) { Ditton on for..of iteration. @@ +498,5 @@ > vbox.appendChild(innerbox); > > + for(let i=0; i<extraButtons.length; i++) { > + let button = this.doc.createElement("button"); > + button.className = 'devtools-toolbarbutton' Ditto on the class name and styling.

(In reply to Victor Porof [:vp] from comment #18) > Make the class name an option as well, like the label and the command, and > define the style in debugger.inc.css. There, remove all button styling, > leaving only the label (no border or background etc.), and color it blue. I added the following code for button styling. I don't know what else to add to it to give it "link" style look. Should I add style for underlining the text? .dbg-expression-button { -moz-appearance: none; border: none; background: none; color: rgba(29, 79, 115, 1); min-width: 20px; // doesn't work } And other changes which you told are done. > Let me know if you need help with building a test for this. Try browsing some of the debugger tests, > clone one and modify it to suit your needs. Sure, I would like to add test for this, though I haven't looked at any tests yet.

I'd underline the text, so that the user knows this is clickable.

(In reply to Julien Wajsberg [:julienw] from comment #20) > I'd underline the text, so that the user knows this is clickable. Yeah, that seemed appropriate to me too. Anything else, like a hand mouse pointer.

This patch contains the changes you told. I am not still confident about the button styling. Let me know, if you still want me to do changes in it.

Comment on attachment 8366762 [details] [diff] [review] Proposed patch version 1. Review of attachment 8366762 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-panes.js @@ +1905,5 @@ > + label: L10N.getStr("addWatchExpressionButton"), > + className: "dbg-expression-button", > + command: () => { > + DebuggerView.WatchExpressions.addExpression(evalPrefix); > + DebuggerView.editor.focus(); There's a slight annoying delay when calling .focus(); We should avoid doing that by adding a new param to addExpression, which tells it to avoid waiting for additional user input. I'll attach a diff to show you how. ::: browser/devtools/shared/widgets/Tooltip.js @@ +436,5 @@ > description.textContent = text; > vbox.appendChild(description); > } > > + for(let { label, className, command } of extraButtons) { Nit: add a space between for and ( @@ +496,5 @@ > innerbox.className = "devtools-tooltip-variables-view-innerbox"; > innerbox.setAttribute("flex", "1"); > vbox.appendChild(innerbox); > > + for(let { label, className, command } of extraButtons) { Nit: add a space between for and ( @@ +500,5 @@ > + for(let { label, className, command } of extraButtons) { > + let button = this.doc.createElement("button"); > + button.className = className; > + button.setAttribute("label", label); > + button.addEventListener("command", command); Unfortunately, you won't be able to add more than one watch expression, because the buttons are cached and command event listener is only added once, and it will be the function that adds the first expression string. My recommendation is to ditch the reuseCachedWidget option and rebuild the contents every time. `setTextContent` doesn't suffer from this problem because it creates new buttons every time. ::: browser/themes/shared/devtools/debugger.inc.css @@ +283,5 @@ > +.dbg-expression-button { > + -moz-appearance: none; > + border: none; > + background: none; > + color: rgba(70, 175, 227, 1); This looks good to me! You should use different colors based on the theme, and identify them with comments: .theme-dark .dbg-expression-button { color: #46afe3; /* Blue highlight color */ } .theme-light .dbg-expression-button { color: #0088cc; /* Blue highlight color */ } @@ +284,5 @@ > + -moz-appearance: none; > + border: none; > + background: none; > + color: rgba(70, 175, 227, 1); > + text-decoration: underline; Add a cursor: pointer; here for a nicer feel :)

Merged diff https://bugzilla.mozilla.org/attachment.cgi?id=8367187&action=edit and did other changes you told.

Comment on attachment 8367231 [details] [diff] [review] Proposed patch version 2. Review of attachment 8367231 [details] [diff] [review]: ----------------------------------------------------------------- Please don't ask me for review until you believe this patch is ready to land. Ask for feedback instead :) This still needs a test. ::: browser/devtools/shared/widgets/Tooltip.js @@ +502,5 @@ > + button.className = className; > + button.setAttribute("label", label); > + button.addEventListener("command", command); > + vbox.appendChild(button); > + } I mentioned something about adding more than one expression not working because of the buttons being cached. See previous review.

(In reply to Victor Porof [:vp] from comment #26) > Please don't ask me for review until you believe this patch is ready to > land. Ask for feedback instead :) > This still needs a test. Sorry, will keep in mind from now on :) And will also add the test in patch itself. > I mentioned something about adding more than one expression not working > because of the buttons being cached. See previous review. I did pass argument 'reuseCacheWidget' value 'false' in tooltip.setVariable function call. Is it not working? Or, did I misinterpret what you told?

(In reply to Lalit Khattar from comment #27) > (In reply to Victor Porof [:vp] from comment #26) > > I did pass argument 'reuseCacheWidget' value 'false' in tooltip.setVariable > function call. Is it not working? Or, did I misinterpret what you told? Oh, I meant removing that argument altogether from the `setVariable` method! No need to keep complicating our lives.

Hey Victor, I have added the test but the test is giving timeout error, don't know why. Can you please look into it?

Comment on attachment 8367932 [details] [diff] [review] Proposed patch version 3. Review of attachment 8367932 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comments below addressed. I'll push to try once you update the patch. This looks ready to land if all's good ;) ::: browser/devtools/debugger/test/browser.ini @@ +246,5 @@ > [browser_dbg_variables-view-reexpand-02.js] > [browser_dbg_variables-view-webidl.js] > [browser_dbg_watch-expressions-01.js] > [browser_dbg_watch-expressions-02.js] > +[browser_dbg_expression-button-01.js] Please name this test browser_dbg_variables-view-popup-11.js and keep those files sorted alphabetically. ::: browser/devtools/debugger/test/browser_dbg_expression-button-01.js @@ +22,5 @@ > + "The button available is labeled correctly."); > + is(tooltip.querySelector("button").className, className, > + "The button available is styled correctly."); > + > + tooltip.querySelector("button").click(); The logs tell you why this test is failing :) > Waiting for event: 'popuphiding' on [object XULElement] This means the popup hides before it's expected. The act of clicking the button closes the popup, as implemented in debugger-panes.js. See below for fix. @@ +39,5 @@ > + executeSoon(() => debuggee.start()); > + yield waitForSourceAndCaretAndScopes(panel, ".html", 19); > + > + // Inspect primitive value variable. > + yield openVarPopup(panel, { line: 15, ch: 12 }); Instead of calling reopen immediately, you should wait for the popup to close and watch expressions to be evaluated. Also, whenever the popup opens and there are watch expressions present, they're re-evaluated, so we need to be careful about that too. Even more, you'll have to pass true for the aWaitForFetchedProperties when opening a popup containing a complex object with properties. See below: // Inspect primitive value variable. yield openVarPopup(panel, { line: 15, ch: 12 }); let popupHiding = once(tooltip, "popuphiding"); let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); testExpressionButton("Watch", "dbg-expression-button", "a"); yield promise.all([popupHiding, expressionsEvaluated]); ok(true, "The new watch expressions were re-evaluated and the panel got hidden (1)."); // Inspect non primitive value variable. let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); yield openVarPopup(panel, { line: 16, ch: 12 }, true); yield expressionsEvaluated; ok(true, "The watch expressions were re-evaluated when a new panel opened (1)."); let popupHiding = once(tooltip, "popuphiding"); let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); testExpressionButton("Watch", "dbg-expression-button", "b"); yield promise.all([popupHiding, expressionsEvaluated]); ok(true, "The new watch expressions were re-evaluated and the panel got hidden (2)."); // Inspect property of an object. let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); yield openVarPopup(panel, { line: 17, ch: 10 }); yield expressionsEvaluated; ok(true, "The watch expressions were re-evaluated when a new panel opened (2)."); let popupHiding = once(tooltip, "popuphiding"); let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); testExpressionButton("Watch", "dbg-expression-button", "b.a"); yield promise.all([popupHiding, expressionsEvaluated]); ok(true, "The new watch expressions were re-evaluated and the panel got hidden (3)."); ::: browser/devtools/shared/widgets/Tooltip.js @@ +486,2 @@ > > + var vbox = this.doc.createElement("vbox"); Should use let here, since there's no reason to use var anymore. @@ +503,2 @@ > > + var widget = new VariablesView(innerbox, viewOptions); Ditto, use let.

Oh, and another thing: remove the watch.removeAt(0); call in testExpressionButton, it's not needed. It's also good to remove it to test whether multiple watch expressions can be added properly.

I just thought about yet another thing: it'd be great to have a test that makes sure adding the same watch expression again doesn't duplicate it (so that clicking "Watch" twice, for the same expression, only adds it once).

(In reply to Victor Porof [:vp] from comment #32) > I just thought about yet another thing: it'd be great to have a test that > makes sure adding the same watch expression again doesn't duplicate it (so > that clicking "Watch" twice, for the same expression, only adds it once). Here is how I plan to do it: 1. Re-inspect a previously inspected variable. 2. call testExpressionButton with 3rd arg: a previously inspected variable expression. // Re-inspect primitive value variable. let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); // opens previously opened variable 'a' yield openVarPopup(panel, { line: 15, ch: 12 }); yield expressionsEvaluated; ok(true, "The watch expressions were re-evaluated when a new panel opened (3)."); let popupHiding = once(tooltip, "popuphiding"); let expressionsEvaluated = waitForDebuggerEvents(panel, events.FETCHED_WATCH_EXPRESSIONS); // pass previously inspected expression as 3rd arg. testExpressionButton(label, className, "b.a"); yield promise.all([popupHiding, expressionsEvaluated]); ok(true, "The new watch expressions were re-evaluated and the panel got hidden (4)."); Let me know, if there is a better way to do it.

(In reply to Lalit Khattar from comment #33) > (In reply to Victor Porof [:vp] from comment #32) Sounds very good! You should create a new test just for this.

(In reply to Victor Porof [:vp] from comment #34) > > Sounds very good! You should create a new test just for this. I don't understand why a separate test is needed :) I think it will be easy to put it in same test otherwise I have to copy most of the previous test which seems pointless to me :) Even if I would, should this task repeat the task of previous test like checking for label, styling etc?

It's nice keeping tests as simple as possible. Even though you'd end up copying some of the code in a previous test, that's better than dealing with potentially complex tests that will end up harder to debug when dealing with intermittent failures. No need to repeat the same checks in the second test. Just make sure no expressions are duplicated (besides checking the expression at index 0, verify the itemCount property, which should be 1 after the second popup too).

Added 'duplicate-expression' test. Assuming there is nothing to add/remove in this patch, I would like to know when will this patch be merged in main repository?

Comment on attachment 8368492 [details] [diff] [review] Proposed patch version 4. Review of attachment 8368492 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I'll push to try and if everything looks good, I'll land the patch.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ea624d60e20b

Try is green! Let's land it.

Thanks for all the help, Victor :)

Thank *you*!