Much of the styling for the #urlbar-zoom-button is duplicated among /browser/themes/linux/browser.css, /browser/themes/osx/browser.css, and /browser/themes/windows/browser.css. If we move the styles to a shared place we can probably remove about 90% of the duplication. Some styles may need to be different for each platform so it will be important that whoever works on this has access to Linux, OSX, and Windows. If you don't have access to all three platforms, you could write a new mozscreenshot API (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots) and use Try server to get screenshots of the changes (this would be great to have anyways!) See /browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm for an example of what would need to be done for the mozscreenshots changes.

Sir, I would like to work on this bug. Would shifting the duplicated lines of code present in the three files that you have mentioned to a new file, also named |browser.css|, in the directory |/browser/themes/shared/| be fine?

Hi Vedant, yeah that sounds fine to me.

Sir, should I limit myself to just |#urlbar-zoom-button|, or should I move ALL the duplicated lines in the three |browser.css| files to a new shared file?

Please limit yourself to just the #urlbar-zoom-button code. Other shared styles can move in the future. In general, smaller changes are better as they are easier to create, review, and test.

I've moved the duplicated lines of code to a new file in |/browser/themes/shared/|. However, I'm having a bit of difficulty with the mozscreenshot API. I have committed the changes locally, and I executed the command mach mochitest --subsuite screenshots --setenv MOZSCREENSHOTS_SETS=DevEdition,TabsInTitlebar,Tabs,WindowSize,Toolbars I have all the screenshots saved on my computer. How do I proceed with the testing part now? I am also uploading the new file that I've created for an initial review.

Shared file eliminating duplicated lines.

I also ran the command ./mach reftest browser/themes/ at the end of which it displayed TEST-PASS and no leaks detected.

Hi Vedant, thanks for your patch. Please follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch to learn how to attach a patch file to this bug.

Comment on attachment 8828694 [details] [diff] [review] Proposed patch for bug 1309260 Review of attachment 8828694 [details] [diff] [review]: ----------------------------------------------------------------- Hi Vedant, were you able to test your changes? You should be able to build these changes with an artifact build: https://developer.mozilla.org/en-US/docs/Artifact_builds I don't think this patch will work, because you added a new browser.css file but it's not referenced by our jar.mn file. Also, the new file should be named browser.inc.css. Nice work so far :)

Terribly sorry for the delay. I was unable to completely fix the first patch as I got hard pressed between college assignments and (now ongoing) exams. I'll try my best to upload the updated patch by Friday :)

I was able to build successfully. However, running > ./mach mochitest --subsuite screenshots --setenv MOZSCREENSHOTS_SETS=DevEdition,TabsInTitlebar,Tabs,WindowSize,Toolbars returns 11 tests passed and 1 test failed, giving the error > 132 INFO TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_screenshots.js | Uncaught exception - [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://mozscreenshots/content/TestRunner.jsm :: loadSets :: line 118" data: no] I couldn't understand why this might occur, as I have another copy of mozilla-central repository and even though I have made no changes in that repository, running the same command over there led to the same result! I apologise for the delay once again though.

moz_screenshots doesn't run too well on local machines. I am sorry I didn't say anything sooner. As long as you run `mach test browser/modules/test/browser_urlBar_zoom.js` and confirm that it passes then you should be covered.

Comment on attachment 8838606 [details] [diff] [review] Changes so far. Review of attachment 8838606 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, and I am really sorry for not reviewing it sooner. ::: browser/themes/linux/browser.css @@ +11,4 @@ > @namespace svg url("http://www.w3.org/2000/svg"); > > %include ../shared/browser.inc > +%include ../shared/browser.inc.css The %include for this file should be moved down some lines so it is declared right before line 20, where `:root {}` is defined. This is important because we want to make sure that %filter linuxShared.inc, %filter substitution, and the two %defines are defined before we include this file. The same changes should be made for the other browser.css files.

I ran |mach test browser/modules/test/browser_urlBar_zoom.js|. The patch passed all tests.

Comment on attachment 8840844 [details] [diff] [review] Proposed patch for Bug 1309260 Review of attachment 8840844 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below line removed. Please update the commit message to read: "Bug 1309260 - Move the urlbar-zoom-button styling to a shared file. r=jaws" Thanks! Nice job :) ::: browser/themes/shared/browser.inc.css @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +@import url("chrome://global/skin/"); I tested your patch with this line removed and it didn't appear necessary.

Thanks for the quick update. I've marked your patch as 'checkin-needed', and it should get checked in to mozilla-inbound sometime today. After being on inbound, it will get merged to mozilla-central, and will likely appear in Firefox Nightly tomorrow or the day after. Thanks!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18) > Thanks for the quick update. I've marked your patch as 'checkin-needed', and > it should get checked in to mozilla-inbound sometime today. After being on > inbound, it will get merged to mozilla-central, and will likely appear in > Firefox Nightly tomorrow or the day after. Thanks! Thank you for guiding me through the silly mistakes I made and helping me complete this patch :)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c214e63d4455 Move the urlbar-zoom-button styling code to a shared place. r=jaws