1284844 - Reps: use quotes around text in ObjectWithText

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

The console currently outputs CSSStyleRule "body", but reps would output CSSStyleRule body

Comment 1

[Hemanth Kumar Veeranki [:harry7]]

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

Comment 2

[Lin Clark [:linclark]]

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

Comment 3

[Hemanth Kumar Veeranki [:harry7]]

Attachment: The Patch file created for Bug 1284844

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

Comment 4

[Hemanth Kumar Veeranki [:harry7]]

(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

Comment 5

[Lin Clark [:linclark]]

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>

Comment 6

[Lin Clark [:linclark]]

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?

Comment 7

[Hemanth Kumar Veeranki [:harry7]]

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

[Hemanth Kumar Veeranki [:harry7]]

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

Comment 9

[Lin Clark [:linclark]]

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!

Comment 10

[Hemanth Kumar Veeranki [:harry7]]

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

Comment 11

[Lin Clark [:linclark]]

You should combine all commits together to just have one commit.

Comment 12

[Hemanth Kumar Veeranki [:harry7]]

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

Integrated all commits into one. Made necessary changes.

Comment 13

[Lin Clark [:linclark]]

Attachment: Bug 1284844 - Reps: use quotes around text in ObjectWithText.

Review commit: https://reviewboard.mozilla.org/r/63504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63504/

Comment 14

[Lin Clark [:linclark]]

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!

Comment 15

[Lin Clark [:linclark]]

Comment on attachment 8769794 [details]
Bug 1284844 - Reps: use quotes around text in ObjectWithText.

https://reviewboard.mozilla.org/r/63504/#review60374

Comment 16

[Hemanth Kumar Veeranki [:harry7]]

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?

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 19

[Hemanth Kumar Veeranki [:harry7]]

(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

Comment 20

[Lin Clark [:linclark]]

It looks like Honza provided the info requested from me. But there was a request for :Tomcat that wasn't flagged in Comment #19.

Comment 21

[Carsten Book [:Tomcat]]

Attachment: rej.file content

Hey, yeah sure attached the content from the rej file here

Comment 22

[Hemanth Kumar Veeranki [:harry7]]

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

Comment 23

[Lin Clark [:linclark]]

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/

Comment 24

[Lin Clark [:linclark]]

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.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d4c09eae59c
Reps: use quotes around text in ObjectWithText. r=me

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d4c09eae59c