Closed Bug 1198569 Opened 7 years ago Closed 7 years ago

Multi-line comments wrap in inspector view

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: darktrojan, Assigned: rohan1395)

Details

(Whiteboard: [good first bug][lang=js][lang=css][polish-backlog])

Attachments

(2 files, 4 obsolete files)

Attached image This looks silly.
Text nodes have newlines replaced with spaces so that they only take up one line in the inspector. Comments should too.
Whiteboard: [good first bug][lang=js][lang=css][polish-backlog]
I would like to work on this. Where is the relevant code located? I tried browser/devtools/inspector/breadcrumbs.js but didn't find anything.
Flags: needinfo?(geoff)
The code that generates the DOM tree preview in the inspector is browser/devtools/markupview/markup-view.js
There are various MarkupContainer implementations in this file, one for each type of node shown in the inspector, and each of them create the necessary markup using some HTML template in browser/devtools/markupview/markup-view.xhtml.
For comments, see the <span id="template-comment"> in that file. This is the piece of HTML that is used to generate comment nodes in the inspector.

I hope that's enough to dig around by yourself a little more. If not, let me know, I can give more hints about how to solve this bug.
Flags: needinfo?(geoff)
Attached patch fix.patch (obsolete) — Splinter Review
Attachment #8654818 - Flags: review?(pbrosset)
Comment on attachment 8654818 [details] [diff] [review]
fix.patch

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

This essentially makes comment nodes look like text nodes. I think that's good.
Can you push to TRY to make sure no tests fail because of this (I highly doubt we have a test that checks this kind things, but you never know).
Attachment #8654818 - Flags: review?(pbrosset) → review+
I have filed a request for level 1 commit access here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1200723

Would you be able to vouch for me ?
Flags: needinfo?(pbrosset)
(In reply to Rohan Rawat from comment #5)
> I have filed a request for level 1 commit access here:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1200723
> 
> Would you be able to vouch for me ?
Don't worry I'll push to try for you for now. Let's keep this request bug open and I'll vouch for you after we've worked a little bit more together. Are you interested in working on other bugs? Feel free to take a look at http://firefox-dev.tools , it should help you find good bugs to work on (bugs that are either mentored or marked as easy). Also, feel free to join our #devtools IRC channel to ask for bugs.
Try build URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e5c0c3a9db
Assignee: nobody → rohan1395
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Ah one more thing, the patch isn't formatted correctly I think. It's just a diff, it has no commit message nor an author.
This should help: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
and this: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8655965 - Flags: review?(pbrosset)
Comment on attachment 8655965 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template

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

This looks good. Thanks for creating this new patch.
Just one last thing needed: can you add the suffix ", r=<reviewer-username>" to the commit message?
So:
Bug 1198569 - Added the 'white-space: normal' rule to the comments template, r=pbro
Once done, you don't need to re-ask for review, you can simply upload your new patch, mark the old one as obsolete, and then add the keyword "checkin-needed" at the top of the bug. The try build isn't done, but the tests are green on one platform already, and knowing the type of change you made, I'm not expecting anything to go badly.
So just set the flag after you've uploaded the new patch, that will put the bug on the sheriffs' radar and they will check it in the repository.
Attachment #8655965 - Flags: review?(pbrosset) → review+
Attachment #8655977 - Flags: review?(pbrosset)
Attachment #8655965 - Attachment is obsolete: true
Attachment #8655977 - Flags: review?(pbrosset)
Keywords: checkin-needed
You might have attached the wrong patch, the new one is identical to the previous one. Also you forgot to set the review flag to R+ when attaching it (in the drop-down where you can choose R? to ask someone to review, you can also choose R+).
Keywords: checkin-needed
Attachment #8654818 - Attachment is obsolete: true
Sorry about that. I am still trying to figure out bzexport.
Attachment #8655977 - Attachment is obsolete: true
Attachment #8656020 - Attachment is obsolete: true
Attachment #8656021 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.