980335 - Ensure browser/devtools/app-manager/content/index.xul is free of inline script

Status: RESOLVED DUPLICATE of bug 1007061

(DevTools Graveyard :: WebIDE, defect)

Description

[Mark Goodwin [:mgoodwin]]

I think there's just a single oncommand attribute.

Comment 1

[Abhishek Potnis [:avp]]

Hi,

I am interested in working on this bug. I am new to XUL, so will need some guidance. I have figured out that the call |UI.selectTabFromButton(event.target)| is the inline script that needs to be moved to index.js. Am I going in the right direction?

Thanks.

Comment 2

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

Hi Abhishek!

Yes, that sounds right to me!  For this particular change, working with XUL should be about the same as HTML, if you are familiar with that.  In any case, let me know if you have any questions.

If you need some help getting started with our code base, check out the Dev Tools hacking[1] guide.

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

Comment 3

[Abhishek Potnis [:avp]]

Attachment: bug-980335-fix.diff

Comment 4

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

Comment on attachment 8387717 [details] [diff] [review]
bug-980335-fix.diff

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

This is close, but not quite right.  You can test it out yourself by opening the App Manager (Tools -> Web Developer -> App Manager).

Also, your comment message should start with "Bug 980335 - " and end with "r=jryans".  See the guide[1] for more info.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/app-manager/content/index.js
@@ +22,5 @@
>      this.onUnload = this.onUnload.bind(this);
>      this.onMessage = this.onMessage.bind(this);
>      this.onConnected = this.onConnected.bind(this);
>      this.onDisconnected = this.onDisconnected.bind(this);
> +    let e1 = document.getElementById("tabs");

Use meaningful names for variables.  So for this one, probably "tabs".

@@ +27,2 @@
>  
> +    e1.addEventListener("click", selectTabFromButton(e1));

The Apps and Device buttons on the left should switch between different panels, but that's not working anymore with this change.

Here you are calling the |selectTabFromButton| function and passing it's result to |addEventListener|, but that's not what we want.  Instead, |selectTabFromButton| should be called when the click happens.

Also, try using the same pattern as other on* event handlers here.  You don't need to change the name to start with "on" or anything, but you probably need to use bind and also remove the listener on unload.

Comment 5

[Abhishek Potnis [:avp]]

Attachment: bug-fix-980335.diff

I have made the suggested changes, but I did not find other on* Event Handlers in index.xul.

Comment 6

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

Comment on attachment 8390387 [details] [diff] [review]
bug-fix-980335.diff

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

The buttons are still broken.  Once again, please test locally with each patch.  If you are unsure about something, ask specific questions when attaching your patch.

Also, as I said last time: you probably need to use bind and also remove the listener on unload.

Comment 7

[Abhishek Potnis [:avp]]

Attachment: bug-980335-fix.diff

I have tested the patch and it works. I needed to use bind as you pointed out.

Initially, I  built with the previous patch and got a Reference Error saying "selectTabFromButton is not defined". I read about bind() on the MDN, but am not clear with how 
|this.selectTabFromButton = this.selectTabFromButton.bind(this);| 
line resolved that Reference Error.

My understanding is the |this| in the above line refers to a UI Class Object, which should be used (as the |this|) in selectTabFromButton function, but without the bind(as in the previous patch), the |this| in selectTabFromButton was being set to something else(may be the window object). Is this correct?

Also, I did not understand why I need to remove the Event Listener on unload.

Thanks.

Comment 8

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

Thanks for your efforts to fix this bug!  I appreciate your drive to complete this work and the dedication you have shown so far.

However, our code is complex, and I think you need to improve your knowledge of JavaScript concepts and event handling before continuing to help with a task like this.

We'd love to help you get involved in a different manner where your skills can be used optimally, such as new bug triage, QA, or regression range bisection.  Please take a look at our contribution page[1] to see other ways you can help.

[1]: http://www.mozilla.org/contribute/

Comment 9

[Biraj Karmakar [:biraj]]

Hi Ryan,

I have just started bug solving and this will be my bug for DevTools and I am interested on this ? Can you guide me to solve this?

Comment 10

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

Biraj,

Please take a look through the earlier comments on this bug report.  I think they give a good description of what's needed here.

If you need some help getting started with our code base, check out the Dev Tools hacking[1] guide.  If you have trouble getting Firefox to build, you can stop by IRC in the #introduction room for help with that.

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

Comment 11

[Paul Rouget [:paul]]

Know that index.xul will disappear in a couple of weeks once we land the new UI. I don't think it's worth it to try to fix this bug.

Comment 12

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

Ah, Paul's right, I didn't think about that.  Biraj, you're probably better off picking a different bug to get started.  You can find more of them here[1].

[1]: http://mzl.la/1kVITbY

Comment 13

[Biraj Karmakar [:biraj]]

ok Thanks

Comment 14

[Anup Kumar [:reznord]]

Hi,

I am interested in working on this bug. so please can you assign this bug to me?

Thanks in advance,

Regards,
Anup

Comment 15

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

Anup, see my comment 12 above.  I don't really think this is a good bug for mentoring anymore, since the file involved is going to be replaced soon.  I've now removed the whiteboard tags to reflect that.

However, if you are still interested in it anyway, I'll assign it once you have a first patch attached.