Closed Bug 870309 Opened 7 years ago Closed 3 years ago

Clean up customizableui CSS files

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: Unfocused, Assigned: Towkir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P5] [good first bug][lang=css])

Attachments

(1 file, 4 obsolete files)

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.
No longer blocks: 770135
Whiteboard: [Australis:M6]
Hey Blair - assigning this to you for M6. Let me know if it should be re-assigned.
Assignee: nobody → bmcbride
Whiteboard: [Australis:M6] → [Australis:M7]
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Depends on: 930045
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.
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P5] [feature] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:M?][Australis:P5] [feature] p=0 → [Australis:M?][Australis:P5]
Whiteboard: [Australis:M?][Australis:P5] → [Australis:P5]
Assignee: bmcbride → nobody
Status: ASSIGNED → NEW
Assigning to Aayush as per in-person request at the IITKGP MozSetup event.
Assignee: nobody → aayushbatra01
Attached patch patch v1 (obsolete) — Splinter Review
Can you go through the changes I made?
Attachment #8399083 - Flags: review?(bmcbride)
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.
Attachment #8399083 - Flags: review?(bmcbride) → review-
Attached patch panel.patch (obsolete) — Splinter Review
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.
Attachment #8399083 - Attachment is obsolete: true
Attachment #8399255 - Flags: review?(bmcbride)
Status: NEW → ASSIGNED
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)
Attachment #8399255 - Flags: review?(bmcbride) → feedback+
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.
Flags: needinfo?(aayushbatra01)
Assignee: aayushbatra01 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(aayushbatra01)
Whiteboard: [Australis:P5] → [Australis:P5] [good first bug][lang=css]
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).
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attached patch customizableuimodification.patch (obsolete) — Splinter Review
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
Attachment #8772793 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8772793 - Flags: review?(gijskruitbosch+bugs)
Attached patch customizableUImodification.patch (obsolete) — Splinter Review
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
Attachment #8772793 - Attachment is obsolete: true
Attachment #8772810 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8772810 - Flags: review?(gijskruitbosch+bugs) → review-
Sorry about that, how could I miss this !!!
Assignee: nobody → 3ugzilla
Attachment #8772810 - Attachment is obsolete: true
Attachment #8772812 - Flags: review?(gijskruitbosch+bugs)
Attachment #8772812 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
Attachment #8399255 - Attachment is obsolete: true
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
Keywords: checkin-needed
Status: NEW → ASSIGNED
As per comment 23 ...
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/5f4846ed9f65
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.