Closed Bug 1284844 Opened 4 years ago Closed 4 years ago

Reps: use quotes around text in ObjectWithText

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: harry7)

References

Details

(Whiteboard: [reserve-html][good first bug])

Attachments

(2 files, 3 obsolete files)

The console currently outputs CSSStyleRule "body", but reps would output CSSStyleRule body
Blocks: 1283847
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Summary: Reps: use quotes around test in ObjectWithText → Reps: use quotes around text in ObjectWithText
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Flags: needinfo?(odvarko)
Whiteboard: [reserve-html] → [reserve-html][good first bug]
I want to work on this task. I am new to this. I downloaded source code using hg and built it. Now I want to know how to start with searching the files what needs to be changed
Flags: needinfo?(lclark)
Great!

In the test for ObjectWithText, you'll change ".Shadow" to "\".Shadow\"". That will make the test check for quotes around the text value. You can see the line of code here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/test/mochitest/test_reps_object-with-text.html#42

This will make the test fail. You can see this by running:

> ./mach test test_reps_object-with-text.html

Then, to make it pass again the ObjectWithText component you'll change the line in getDescription to include quotes. Here's where to make that fix: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/reps/object-with-text.js#35
Flags: needinfo?(odvarko)
Flags: needinfo?(lclark)
Assignee: nobody → hems.india1997
Status: NEW → ASSIGNED
I modified the tests and changed the necessary file  and I ran the test by running this
./mach test test_reps_object-with-text.html
it passed the test. 
Attatched a Patch
Attachment #8769709 - Flags: review?(lclark)
(In reply to Hemanth Kumar Veeranki from comment #3)
> Created attachment 8769709 [details] [diff] [review]
> The Patch file created for Bug 124844
> 
> I modified the tests and changed the necessary file  and I ran the test by
> running this
> ./mach test test_reps_object-with-text.html
> it passed the test. 
> Attatched a Patch

Typo Patch file is for Bug 1284844
Attachment #8769709 - Attachment description: The Patch file created for Bug 124844 → The Patch file created for Bug 1284844
Comment on attachment 8769709 [details] [diff] [review]
The Patch file created for Bug 1284844

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

Overall, looks good! It will need to pass eslint before we commit it, but other than that we should be good to go.

In your next patch, you'll want the commit message to be the name of the issue and the reviewers name, so:

> Bug 1284844 - Reps: use quotes around text in ObjectWithText. r=linclark

You can mark me for review again once you've made the changes

::: devtools/client/shared/components/reps/object-with-text.js
@@ +32,4 @@
>      },
>  
>      getDescription: function (grip) {
> +      return (grip.preview.kind == "ObjectWithText") ? "\""+grip.preview.text +"\"" : "";

There needs to be a space around either side of the plus sign according to our code style rules.

You can check this file for code style issues using:

> ./mach eslint --no-ignore <filename>
Attachment #8769709 - Flags: review?(lclark)
Attachment #8769709 - Flags: review?(lclark)
Comment on attachment 8769709 [details] [diff] [review]
The Patch file created for Bug 1284844

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

it looks like this is the same patch. Maybe there was a mistake in uploading?
Attachment #8769709 - Flags: review?(lclark) → review-
(In reply to Lin Clark [:linclark] from comment #6)
> Comment on attachment 8769709 [details] [diff] [review]
> The Patch file created for Bug 1284844
> 
> Review of attachment 8769709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it looks like this is the same patch. Maybe there was a mistake in uploading?

Yes I am sorry. I was confused. I ll upload the new one
Comment on attachment 8769751 [details] [diff] [review]
Bug 1284844 - Reps: use quotes around text in ObjectWithText. r=linclark

Ah, I didn't realize that it would be split across multiple lines like this if there were spaces.

Now that I look closer, the check for grip.preview.kind in the existing code is actually unnecessary here. We are checking for that in supportsObject already anyway, so there's no way to reach this code (grip.preview.kind == "ObjectWithText") isn't true. Because of this, we can remove that and just return the grip.preview.text string with quotes around it. That will clean up the code a little bit. Thanks!
Attachment #8769751 - Flags: review?(lclark)
Attachment #8769709 - Attachment is obsolete: true
(In reply to Lin Clark [:linclark] from comment #9)
> Comment on attachment 8769751 [details] [diff] [review]
> Bug 1284844 - Reps: use quotes around text in ObjectWithText. r=linclark
> 
> Ah, I didn't realize that it would be split across multiple lines like this
> if there were spaces.
> 
> Now that I look closer, the check for grip.preview.kind in the existing code
> is actually unnecessary here. We are checking for that in supportsObject
> already anyway, so there's no way to reach this code (grip.preview.kind ==
> "ObjectWithText") isn't true. Because of this, we can remove that and just
> return the grip.preview.text string with quotes around it. That will clean
> up the code a little bit. Thanks!

Now should I create a patch with recently pulled remote version or my recent updated version. Because If you were to integrate the patch then we ll need to merge all commits right. Shall I update next patch by integrating all commits ?
You should combine all commits together to just have one commit.
Integrated all commits into one. Made necessary changes.
Attachment #8769788 - Flags: review?(lclark)
Comment on attachment 8769788 [details] [diff] [review]
Bug 1284844 - Reps: use quotes around text in ObjectWithText. r=linclark

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

This looks good to me. I've pushed it up to our system called MozReview to do a automated run of the test suite. Once that passes, I think this should be ready to go. Thanks!
Attachment #8769788 - Flags: review?(lclark) → review+
Comment on attachment 8769794 [details]
Bug 1284844 - Reps: use quotes around text in ObjectWithText.

https://reviewboard.mozilla.org/r/63504/#review60374
Attachment #8769794 - Flags: review?(lclark) → review+
Attachment #8769751 - Attachment is obsolete: true
Attachment #8769788 - Attachment is obsolete: true
Keywords: checkin-needed
This is a general question. What do reps mean in this context? I see this word in some other bugs as well. So I would like to know what exactly are these reps?
Flags: needinfo?(lclark)
seems this has conflicts like applying 5775fc1df401
patching file devtools/client/shared/components/reps/object-with-text.js
Hunk #1 FAILED at 26
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/object-with-text.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(hems.india1997)
(In reply to Hemanth Kumar Veeranki from comment #16)
> This is a general question. What do reps mean in this context? I see this
> word in some other bugs as well. So I would like to know what exactly are
> these reps?

Reps (Representation) is set of React components that are responsible for rendering JS primitive (string, number, etc) as well as complex (arrays, objects, etc) data types. The plan is using these reps everywhere in the DevTools UI (Console panel, DOM panel, Debugger panel) so, JS object rendering is consistent across the entire UI

You can see existing reps in devtools/client/shared/components/reps dir.

Honza
(In reply to Carsten Book [:Tomcat] from comment #17)
> seems this has conflicts like applying 5775fc1df401
> patching file devtools/client/shared/components/reps/object-with-text.js
> Hunk #1 FAILED at 26
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/shared/components/reps/object-with-text.js.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue

Can I see what went wrong? like the contents of .rej file generated
Flags: needinfo?(hems.india1997)
It looks like Honza provided the info requested from me. But there was a request for :Tomcat that wasn't flagged in Comment #19.
Flags: needinfo?(lclark) → needinfo?(cbook)
Attached file rej.file content
Hey, yeah sure attached the content from the rej file here
Flags: needinfo?(cbook)
(In reply to Carsten Book [:Tomcat] from comment #21)
> Created attachment 8770137 [details]
> rej.file content
> 
> Hey, yeah sure attached the content from the rej file here

But I dont get anything. That was the line which I should change for this bug to be resolved. Is there something I should do ?
Flags: needinfo?(lclark)
Comment on attachment 8769794 [details]
Bug 1284844 - Reps: use quotes around text in ObjectWithText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63504/diff/1-2/
https://reviewboard.mozilla.org/r/63504/#review61274

This just needed to be rebased. I went ahead and did that and should be able to land it now.
Flags: needinfo?(lclark)
Pushed by lclark@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d4c09eae59c
Reps: use quotes around text in ObjectWithText. r=me
https://hg.mozilla.org/mozilla-central/rev/4d4c09eae59c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.3 - Jul 18
Priority: P3 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.