Closed Bug 1123125 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools Graveyard :: WebIDE, defect)

37 Branch
x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: abdelrahman, Assigned: jtungul53, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

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
Whiteboard: [lang=js][good first bug]
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,
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
(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
so i just have to remove the hasChild from new.js right?
(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
Are there any tests for this file?
Thanks
(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)
I think I've completed the task. How can I check to see if it is correct and did not break anything?
Attached patch bug1123125_removehasChild.diff (obsolete) — Splinter Review
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)
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.
Attached patch bug1123125_removehasChild.diff (obsolete) — Splinter Review
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)
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)
Attachment #8551904 - Attachment is obsolete: true
Attachment #8552810 - Attachment is obsolete: true
(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 :)
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)
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+
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.
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/
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.