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

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

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

Comment 1

[John Tungul]

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

[Vikram Jadhav]

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

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

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

Comment 4

[Vikram Jadhav]

so i just have to remove the hasChild from new.js right?

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

(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

[Vikram Jadhav]

Are there any tests for this file?
Thanks

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

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

Comment 8

[John Tungul]

I think I've completed the task. How can I check to see if it is correct and did not break anything?

Comment 9

[John Tungul]

Attachment: 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.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 11

[John Tungul]

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.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 13

[John Tungul]

Attachment: 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)

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

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 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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".

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

(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 :)

Comment 17

[John Tungul]

Attachment: 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

Comment 18

[John Tungul]

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!

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 21

[John Tungul]

Uh oh, looks like I broke Firefox. There are failed test cases. How do we fix these so we can update for checkin-needed?

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 23

[John Tungul]

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).

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 25

[John Tungul]

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?

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 27

[John Tungul]

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!

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/92c38ff324f6

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92c38ff324f6

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92c38ff324f6