Our CSS files are a bit of a mess.... with most stuff defined in panelUIOverlay.css, which should be very minimal (it should only have styles for the button, as that's the only thing added in the overlay). Everything to do with the panel should be in panelUI.css.

Hey Blair - assigning this to you for M6. Let me know if it should be re-assigned.

Removing the items from M7 that do not block landing on m-c.

Blair, I think this has come down to just renaming panelUIOverlay.(inc.)css to just panelUI.(inc.)css. Or do you see more areas for improvement?

In any case, I would like to come up with a list of possible improvements so we may take this bug on in a structured manner.

(In reply to Mike de Boer [:mikedeboer] from comment #3) > Blair, I think this has come down to just renaming panelUIOverlay.(inc.)css > to just panelUI.(inc.)css. Or do you see more areas for improvement? Yea, since we got rid of the overlay to add the button to in-content pages, that's enough. Though, you'll also want to remove the rules for #app-extension-point-end, which should have been removed when we removed the overlay.

Assigning to Aayush as per in-person request at the IITKGP MozSetup event.

Can you go through the changes I made?

Comment on attachment 8399083 [details] [diff] [review] patch v1 Review of attachment 8399083 [details] [diff] [review]: ----------------------------------------------------------------- Wrong patch :) This is the patch for bug 982735.

Uh oh, this is my fault. I was helping Aayush upload the patch from my laptop (I had scp'd it across from the build machine), and I guess I picked Sukanta's patch while uploading. Whoops. Uploading it myself since I'm not sure if Aayush has the original patch with him. The metadata is associated with him, though.

Comment on attachment 8399255 [details] [diff] [review] panel.patch Review of attachment 8399255 [details] [diff] [review]: ----------------------------------------------------------------- This is the right direction, but: 1) this misses the OS X include change 2) this misses the removal of the app-extension point rules here: https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#138 (all of the app-extension-point things are dead)

Aayush, are you still working on this? Sorry about the delay in getting review, if necessary I can help you through putting up an improved patch.

Hello, I would like to help with this, if I can.

(In reply to :Gijs Kruitbosch from comment #10) > Comment on attachment 8399255 [details] [diff] [review] > panel.patch > > Review of attachment 8399255 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is the right direction, but: > > 1) this misses the OS X include change > 2) this misses the removal of the app-extension point rules here: > https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > customizableui/panelUIOverlay.inc.css#138 > > (all of the app-extension-point things are dead) Hi, I would like to work with this one, Looks like the extension point matter is already out of way, now what left is just to rename those files, If I am not wrong, may be I can submit a patch, and If I am wrong, what exactly should be done here ?

beside, did you mention this line https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#138 on comment 10 ? (as mxr is now broken).

(In reply to [:Towkir] Ahmed from comment #14) > beside, did you mention this line > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > customizableui/panelUIOverlay.inc.css#138 on comment 10 ? (as mxr is now > broken). No, the code that I referenced in comment #10 is already gone. We just want to rename the files, AIUI.

Just to make sure before submitting patch, https://dxr.mozilla.org/mozilla-central/search?q=panelUIOverlay&redirect=false should be done according to comment 5 right ?

(In reply to [:Towkir] Ahmed from comment #16) > Just to make sure before submitting patch, > https://dxr.mozilla.org/mozilla-central/ > search?q=panelUIOverlay&redirect=false > should be done according to comment 5 right ? Yes.

Looks like the files in the "obj-x86_64-pc-linux-gnu" don't get added in the patch. Please assign this to me if this looks good. Thanks

Comment on attachment 8772793 [details] [diff] [review] customizableuimodification.patch Review of attachment 8772793 [details] [diff] [review]: ----------------------------------------------------------------- You should use "hg move" instead of copying and add/removing the file.

Hope this looks good :) And thanks for the hg rename tips, I was looking for something like this but discovered it a late, earlier google searches were of no use :p

Comment on attachment 8772810 [details] [diff] [review] customizableUImodification.patch Review of attachment 8772810 [details] [diff] [review]: ----------------------------------------------------------------- Please provide a better commit message that describes what the patch does. You've also updated all the references to panelUIOverlay.inc.css to point to panelUI.inc.css but not renamed that file.

Sorry about that, how could I miss this !!!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/5f4846ed9f65 Shift to panelUI.css from panelUIOverlay.css in customizableui CSS files. r=gijs

As per comment 23 ...

(In reply to [:Towkir] Ahmed from comment #24) > As per comment 23 ... No, this is only fixed when it lands on central. It will (semi-)automatically get marked fixed at that point.

It could also have been merged/included into browser.css? That would save loading a separate style sheet on startup.

(In reply to Alfred Kayser from comment #27) > It could also have been merged/included into browser.css? > That would save loading a separate style sheet on startup. and it would have made browser.css even bigger / more of a dumping ground. If anything, more files rather than fewer seems the sensible approach here, to try and keep separate rules separate and have it obvious where particular bits belong. If there are perf gains in concatenating the files (seems doubtful as it's all chrome:// and preloaded anyway, and IIRC we tried this when looking for australis perf impact), by all means file a followup for concatenating at build-time - but it should probably wait for the resolution of bug 1288747. We're done in this bug.