Remove hasChild from new.js in projecteditor and use it from resource.js

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: abdelrhman, Assigned: jtungul53, Mentored)

Tracking

37 Branch
Firefox 38
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Remove the hasChild function from file new.js
http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/devtools/projecteditor/lib/plugins/new/new.js#l79

because it's exist in resource.js
http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/devtools/projecteditor/lib/stores/resource.js#l135

and also modify the calling of hasChild function in new.js to be called from resource.js
(Reporter)

Updated

4 years ago
Whiteboard: [lang=js][good first bug]
(Assignee)

Comment 1

4 years ago
I have some Javascript experience, but am new to open source contributions. This bug sounds easy enough to fix. I would need some help on how to find this file in my local directory, and submitting the fixed version. Would you be able to show me the process? If yes, I can do my best on this.

Comment 2

4 years ago
Hi,
i already build the Mozilla code.
I want to work on this bug,
but i just want know how do i include the resource.js to new.js for using hasChild
(Reporter)

Comment 3

4 years ago
(In reply to John Tungul from comment #1)
> I have some Javascript experience, but am new to open source contributions.
> This bug sounds easy enough to fix. I would need some help on how to find
> this file in my local directory, and submitting the fixed version. Would you
> be able to show me the process? If yes, I can do my best on this.
Hi John,
You can start with developer guide (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide)
and also there are some tutorials on codefirefox (http://codefirefox.com)

(In reply to Vikram Jadhav from comment #2)
> Hi,
> i already build the Mozilla code.
> I want to work on this bug,
> but i just want know how do i include the resource.js to new.js for using
> hasChild
Hi Vikram,
Actually you don't need to include it.
check projecteditor.js file lines 12 and 29 (http://hg.mozilla.org/mozilla-central/file/6446c26b45f9/browser/devtools/projecteditor/lib/projecteditor.js)
Version: 36 Branch → 37 Branch

Comment 4

4 years ago
so i just have to remove the hasChild from new.js right?
(Reporter)

Comment 5

4 years ago
(In reply to Vikram Jadhav from comment #4)
> so i just have to remove the hasChild from new.js right?

Right and you need to change the calling of hasChild this.hasChild(parent, name) to be called from resoures

Comment 6

4 years ago
Are there any tests for this file?
Thanks
(Reporter)

Comment 7

4 years ago
(In reply to Vikram Jadhav from comment #6)
> Are there any tests for this file?
> Thanks

Sure there is test in this file(browser/devtools/projecteditor/test/browser_projecteditor_new_file.js)
(Assignee)

Comment 8

4 years ago
I think I've completed the task. How can I check to see if it is correct and did not break anything?
(Assignee)

Comment 9

4 years ago
Created attachment 8551904 [details] [diff] [review]
bug1123125_removehasChild.diff

in resource.js, i've exported the hasChild function and used a require in the new.js file so the hasChild() function can be called with the existing definition in resource.js. The duplicate hasChild() definition in new.js was deleted.
Attachment #8551904 - Flags: review?(a.ahmed1026)
Mentor: jryans
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → jtungul53
Status: NEW → ASSIGNED
Comment on attachment 8551904 [details] [diff] [review]
bug1123125_removehasChild.diff

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

Abdelrhman, thanks for filing this!  John, thanks for working on it!

If you need additional tips about our development workflow, see the hacking[1] page.

[1]: https://wiki.mozilla.org/DevTools/Hacking

::: browser/devtools/projecteditor/lib/plugins/new/new.js
@@ +51,5 @@
>  
>          // XXX: sanitize bad file names.
>  
>          // If the name is already taken, just add/increment a number.
> +        if (hasChild(parent, name)) {

After updating |hasChild| as I mentioned, this will become

parent.hasChild(name)

@@ +71,5 @@
>      let name;
>      do {
>        name = template.replace("\{1\}", i === 1 ? "" : i);
>        i++;
> +    } while (hasChild(parent, name));

Same as above

::: browser/devtools/projecteditor/lib/stores/resource.js
@@ +398,5 @@
>    }
>  });
>  
>  exports.FileResource = FileResource;
> +exports.hasChild = hasChild;
\ No newline at end of file

We don't need to export this.

Instead we should change the |hasChild| method to act on the current resource instance.  This is something I seem to have missed when reviewing bug 1103346.

So, remove the |resource| argument of |hasChild| and replace its usage with |this|.

You'll also need to update the calls in projecteditor/lib/plugins/rename/rename.js.
Attachment #8551904 - Flags: review?(a.ahmed1026)
(Assignee)

Comment 11

4 years ago
If I change the definition hasChild:function(resource,name) to hasChild:function(name), which variables in the function need to add a this. prefixed to them? Sorry, my Javascript is not too good.
(In reply to John Tungul from comment #11)
> If I change the definition hasChild:function(resource,name) to
> hasChild:function(name), which variables in the function need to add a this.
> prefixed to them? Sorry, my Javascript is not too good.

Replace the parts of |hasChild| function body that used to say |resource| with |this|.  The current object *is* the parent resource, so we don't need to pass it in as an argument.
(Assignee)

Comment 13

4 years ago
Created attachment 8552810 [details] [diff] [review]
bug1123125_removehasChild.diff

Changed this.hasChild(parent,name) function calls on line 55 and 75 in new.js to parent.hasChild(name).

Changed resource.hasChild(parent,name) function calls on line 36 and 59 in rename.js to resource.hasChild(name)

changed hasChild: function(resource, name) function definition on line 135 in resource.js to hasChild: function(name)
Attachment #8552810 - Flags: review?(jryans)
(Reporter)

Comment 14

4 years ago
Comment on attachment 8552810 [details] [diff] [review]
bug1123125_removehasChild.diff

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

Thanks for patch and please update it with these changes
and also add r=jryans to the end of the commit message.

::: browser/devtools/projecteditor/lib/plugins/new/new.js
@@ +6,5 @@
>  
>  const { Class } = require("sdk/core/heritage");
>  const { registerPlugin, Plugin } = require("projecteditor/plugins/core");
>  const { getLocalizedString } = require("projecteditor/helpers/l10n");
> +const { hasChild } = require("projecteditor/lib/stores/resource")

Actually you don't need to do that because resources is already included in projecteditor.js so remove this line from your patch

@@ +76,3 @@
>  
>      return name;
>    },

remove the comma after } at the end of suggestName and also the next empty line

@@ +80,1 @@
>  })

also add semicolon after })

::: browser/devtools/projecteditor/lib/plugins/rename/rename.js
@@ +32,5 @@
>        tree.promptEdit(oldName, resource).then(name => {
>          if (name === oldName) {
>            return resource;
>          }
> +        if (resource.hasChild(name)) {

this needs to use parent object not resource so change it to parent.hasChild(name)

@@ +55,5 @@
>      let parent = resource.parent;
>      do {
>        name = template.replace("\{1\}", i === 1 ? "" : i);
>        i++;
> +    } while (resource.hasChild(name));

this also needs to be changed to parent.hasChild(name)

::: browser/devtools/projecteditor/lib/stores/resource.js
@@ +131,5 @@
>     *
>     * @param Resource resource
>     * @param string name
>     */
> +  hasChild: function(name) {

Edit the comment before this line to describe parameters well
Comment on attachment 8552810 [details] [diff] [review]
bug1123125_removehasChild.diff

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

It seems Abdelrhman has given many comments, so I'll await your next revision. :)

Also, when attaching a new version, please mark the old attachments as "obsolete".
Attachment #8552810 - Flags: review?(jryans)
(Assignee)

Updated

4 years ago
Attachment #8551904 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8552810 - Attachment is obsolete: true
(Reporter)

Comment 16

4 years ago
(In reply to J. Ryan Stinnett [:jryans] from comment #15)
> Comment on attachment 8552810 [details] [diff] [review]
> bug1123125_removehasChild.diff
> It seems Abdelrhman has given many comments, so I'll await your next
> revision. :)

I thought you would be late for reviewing it, so I reviewed it till you get back to make patch ready for check-in :)
(Assignee)

Comment 17

4 years ago
Created attachment 8553287 [details] [diff] [review]
bug1123125_removehasChild.diff

removed require statement in new.js
removed comma on line 77 in new.js
semicolon added at end of line 78 in new.js
replaced resource.hasChild(name) with parent.hasChild(name),line 36 and 59 in rename.js
changed the params and description comment of function: hasChild(name) declaration, line 130 in resource.js

r=jryans
Attachment #8553287 - Flags: review?(jryans)
(Assignee)

Comment 18

4 years ago
Got it. I've made the requested changes. Thanks a lot for walking me through my first bug and being so patient with me, guys. I can't thank you enough!
(In reply to John Tungul from comment #18)
> Got it. I've made the requested changes. Thanks a lot for walking me through
> my first bug and being so patient with me, guys. I can't thank you enough!

Don't worry about it, our processes are quite crazy, so it's a lot to take in at first! :) I'll take a look soon.
Comment on attachment 8553287 [details] [diff] [review]
bug1123125_removehasChild.diff

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

Great, looks good to me!

I've pushed this to our try environment to run tests.  Assuming it looks good, we can mark this bug "checkin-needed".

Feel free to look around for other bugs you'd like to work on.  Thanks again!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9bd75e4f7a
Attachment #8553287 - Flags: review?(jryans) → review+
(Assignee)

Comment 21

4 years ago
Uh oh, looks like I broke Firefox. There are failed test cases. How do we fix these so we can update for checkin-needed?
(In reply to John Tungul from comment #21)
> Uh oh, looks like I broke Firefox. There are failed test cases. How do we
> fix these so we can update for checkin-needed?

At the moment, I believe these are not related to your patch and are just artifacts of our testing environment.  I've triggered that bad tests to re-run, so we'll see what happens.
(Assignee)

Comment 23

4 years ago
Alright, hopefully all goes well! While we are waiting, how would I get my contributions on my github? I've watched the video on http://codefirefox.com/video/marketing-yourself, and I've forked the repository already. He says that any push that I do will show up, but I've only submitted the patches for review through bugzilla comments. How would github know?

If this is too much to explain, then just let me know. I can always do without it and just link a search with my name and completed bugs(when I have many contributions).
(In reply to John Tungul from comment #23)
> Alright, hopefully all goes well! While we are waiting, how would I get my
> contributions on my github? I've watched the video on
> http://codefirefox.com/video/marketing-yourself, and I've forked the
> repository already. He says that any push that I do will show up, but I've
> only submitted the patches for review through bugzilla comments. How would
> github know?

Once the tests look good, we'll set "checkin-needed".  A sheriff will land the patch to the fx-team repo[1].  Assuming it doesn't blow up there, it will be merged ~1 day later into mozilla-central[2] and this bug will be marked FIXED.  gecko-dev[3] on GitHub is a mirror of these repos, so it will show up there once the patch has landed.

[1]: http://hg.mozilla.org/integration/fx-team/
[2]: http://hg.mozilla.org/mozilla-central/
[3]: https://github.com/mozilla/gecko-dev/
(Assignee)

Comment 25

4 years ago
Did the retests clear? It still shows the red test cases but I think that was the previous iteration. Are we clear for checkin-needed status?
(In reply to John Tungul from comment #25)
> Did the retests clear? It still shows the red test cases but I think that
> was the previous iteration. Are we clear for checkin-needed status?

Yes, everything looks good now!
Keywords: checkin-needed
(Assignee)

Comment 27

4 years ago
Ahhh yeahhhh! We did it! Thanks for all the help. Until the next time we cross paths! I'm relearning Javascript and which ever other frameworks are used for Firefox, so I wont overwork you mentors for next time!
https://hg.mozilla.org/integration/fx-team/rev/92c38ff324f6
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/92c38ff324f6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 38

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.