Closed Bug 885926 Opened 8 years ago Closed 7 years ago

Switch out querySelector(idToSelector(id)) for getElementsByAttribute(id) for palette queries

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: mconley, Assigned: esajic)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M-][good first bug][mentor=mconley][lang=js][qa-])

Attachments

(1 file, 1 obsolete file)

There are a few times where we query the hidden toolbox palette for an immediate child using querySelector(idToSelector(id)). It turns out that idToSelector is pretty expensive, performance-wise, and we do lots of this querying before we even do first paint (mounting evidence suggests it was a major contributor to the talos regression in bug 880611).

We seem to get far better performance if we just use getElementsByAttribute("id", id) on the palette to query for the children.
Blocks: australis-ts
Hardware: x86 → All
So due to a foul-up on my part, this was erroneously evaluated as an excellent way to recover performance on first paint.

See bug 880611 comment 15 for more details.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
(In reply to Mike Conley (:mconley) from comment #15)
> I may have broken out the champagne too soon - my patch had a bug where I
> was using element.getElementsByAttribute("id", aId), for a node instead of
> element.getElementsByAttribute("id", aId)[0]. Kinda makes all the
> difference, and now the regression is back. :/

So did that make no difference, or did it have a negative impact? If it made no difference, I think we should consider fixing this bug anyway - getElementsByAttribute is more robust, and will likely benefit from future performance improvements.
Flags: needinfo?(mconley)
I actually do think this would be a cleaner way of doing things, instead of mucking about with regex's. Certainly not a blocker though.
Assignee: mconley → nobody
Status: RESOLVED → REOPENED
Flags: needinfo?(mconley)
Resolution: WONTFIX → ---
Whiteboard: [Australis:M8] → [good first bug][mentor=mconley][lang=js]
Whiteboard: [good first bug][mentor=mconley][lang=js] → [Australis:M-][good first bug][mentor=mconley][lang=js]
Hi Mike,

I would like to take this bug. Please assign it to me. 

I am new to mozilla, will you please suggest me where to start for solving this bug? 

Thanks
Assignee: nobody → sumit4iit
Status: REOPENED → ASSIGNED
Thanks Jared,

Can you help me with how to start with this bug? I am not sure which files do I need to look into. 

Thanks.
Sure, you'll need to clone the UX repository which is located at https://hg.mozilla.org/projects/ux. The file in question is located at browser\components\customizableui\src\CustomizableUI.jsm. You'll find idToSelector at the bottom of the file, and there are numerous places within the file where the result of idToSelector is passed to querySelector.
Jared's definitely got you on the right track here. Thanks for jumping on this, sumit4iit! Let us know if you need more pointers.
Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I see that most of the code of m-c and UX is similar.
Do I need to download entire UX repository or is there a way to reuse the code from m-c?
I checked wiki for UX branch and did not get much information.
(In reply to sumit4iit from comment #8)
> Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I
> see that most of the code of m-c and UX is similar.
> Do I need to download entire UX repository or is there a way to reuse the
> code from m-c?
> I checked wiki for UX branch and did not get much information.

Hey sumit4iit, you're definitely going to want the UX branch. While the vast majority of it is similar to mozilla-central, the part you'll be working on is unique to UX.

You can clone it from here:  http://hg.mozilla.org/projects/ux
(In reply to sumit4iit from comment #8)
> Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I
> see that most of the code of m-c and UX is similar.
> Do I need to download entire UX repository or is there a way to reuse the
> code from m-c?
> I checked wiki for UX branch and did not get much information.

I updated the UX branch wiki page to hopefully make this more clear. Please email me if you think there are still some things that could be explained better.
Thanks Jared, I guess that should help newcomers. I have cloned the repo.
The other issue that I noticed is that (I am not sure that if this is the correct place to report, please let me know), 
While cloning I noticed that many times there was a network reset issue and 'hg' used to rollback the transaction. So I used axel to get source in zip format, and I noticed that mozilla server does not support resume capability. I am not sure if this is actually supposed to happen or if this is a bug. 
And this was the reason why I went for the mozilla-central.
No longer blocks: australis-ts
I'm a software developer new to Mozilla but I have done bug patches for Google Android before. I was just wondering whether Sumit was still working on this bug or whether you still need a patch for it? If yes then I would like to have a go. I have cloned (using Mercurial) and built the latest UX branch and got it working on my Mac ok.
Emma, I am not working on this bug presently. 
You can start on this bug.

Thanks :)
Thanks Sumit - re-assigning to Emma.

Emma - I've received your email and will respond shortly.
Assignee: sumit4iit → esajic
Attached patch bug_885926_patch_1.patch (obsolete) — Splinter Review
Thanks Sumit :-)

Mike thanks for your very helpful email about this and for getting back to me so quickly.

I decided to start again using the mozilla-central repository instead of the UX one because the Australis changes got merged into the main Nightly repository (mozilla-central) recently as you mentioned in your email.

My first attempt at a patch is attached here!

Here is info on what I did based on your very handy advice:
-------------------
1. Use Mercurial hg to check out the latest mozilla-central branch
hg clone http://hg.mozilla.org/mozilla-central/ src
cd src
2. Follow build process for Mac OS X using .mozconfig and ./mach build to build Firefox
3. Use ./mach run to run Firefox
4. In address bar type about:config and then set devtools.chrome.enabled to true
5. Tools --> Web Developer --> Scratchpad to launch Scratchpad, then from Scratchpad's Environment Menu select BROWSER rather than CONTENT to run in the Browser context.
6. Execute --> Run to run Scratchpad script (this script's use of CustomizableUI will only work on the Firefox you built yourself using ./mach build)
let widgetGroup = CustomizableUI.getWidget("sync-button");
let node = widgetGroup.forWindow(window).node;
node
node.id
7. If successful the script won't give any output. You need to highlight and use the DISPLAY button to print out the value of "node"
It should have the value [object XULElement] (it's a XULElement object).
8. Highlight and use the DISPLAY button to print out the value of "node.id" -- it should have the value "sync-button".

Additional buttons you can test in the same way as sync-button are:
 feed-button
 home-button
 downloads-button
 developer-button

9. Quit Firefox and run the customizableui test suite using ./mach mochitest-browser browser/components/customizableui/test  
This gives a benchmark of how the tests behave on the machine you're using (as no changes have been made to code yet).

10. Make bug fix edits to src/browser/components/customizableui/src/CustomizableUI.jsm and rebuild.

11. Re-run scratchpad tests and customizableui test suite to verify everything works ok.

12. Use Mercurial to generate a patch.
 hg qnew <patch file name>.patch 
To update the patch file with the latest changes you've made in the repository run hg qref 
Patch will be created in <repository>/.hg/patches
----------------

Feedback very welcome.

Cheers and thanks,
Emma
Comment on attachment 8349584 [details] [diff] [review]
bug_885926_patch_1.patch

Putting into my review queue.
Attachment #8349584 - Flags: review?(mconley)
Comment on attachment 8349584 [details] [diff] [review]
bug_885926_patch_1.patch

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

This looks good - but we should also remove the definition of idToSelector.

Thanks Emma!
Attachment #8349584 - Flags: review?(mconley) → review-
Cheers Mike :-)
Yes good point. I've made an improved patch with the obsolete idToSelector function removed (it was only being used within CustomizableUI.jsm before and I've taken out all references to it now in this fix anyway).
I've re-tested using the scratchpad and the customizableui test suite.
Thanks!
Emma
Attachment #8349584 - Attachment is obsolete: true
Attachment #8349613 - Flags: review?(mconley)
Comment on attachment 8349613 [details] [diff] [review]
bug_885926_patch_1.patch

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

I like it! Thanks Emma!
Attachment #8349613 - Flags: review?(mconley) → review+
Cheers Mike thanks very much for all your help on this :-)
(In reply to Emma Sajic from comment #21)
> Cheers Mike thanks very much for all your help on this :-)

Thank you for your thorough and thoughtful approach! Let me know if you want any help finding another bug to hack on.
https://hg.mozilla.org/mozilla-central/rev/a97cb4ad5ba1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [Australis:M-][good first bug][mentor=mconley][lang=js] → [Australis:M-][good first bug][mentor=mconley][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.