1147281 - Unify popup notification icon CSS in browser/themes/shared/ to reduce duplication

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

There is a bunch of duplication in browser.css across the 3 platforms that could be shared in browser/themes/shared/ to make it easier to maintain and add new notification anchors.

See https://mxr.mozilla.org/mozilla-central/search?string=-notification-icon&find=\.css

A new file can be created e.g. browser/themes/shared/notification-icons.inc.css that contains the notification icon CSS that are identical in all three themes and it can then be #include into the 3 browser.css files.

This will be easiest to review and best for blame if it's done as an `hg copy` of one of the browser.css files with all the stuff not relevant to notification icons deleted.

Comment 1

[Manraj Singh [:manrajsingh]]

Hi sir,

So basically we need to shift CSS code from all three files and put it in a common file and then include that file in all three CSS files right? I would like to take this one up. Could someone assign it to me?

Comment 2

[Matthew N. [:MattN]]

(In reply to Manraj Singh [:manrajsingh] from comment #1)
> So basically we need to shift CSS code from all three files and put it in a
> common file and then include that file in all three CSS files right? I would
> like to take this one up. Could someone assign it to me?

Hi Manraj! That's the right idea.

Comment 3

[Manraj Singh [:manrajsingh]]

Hi sir,

Sir I have fixed the issue but there is a problem in creating the patch file. I have been trying to do it since the past one day. Basically I did a `hg pull -u` before fixing the issue. It had a merge conflict. So I fixed it manually. I did a full build after that and now whenever I tried to create patch it gave a "abort: working directory revision is not qtip" error. I searched and found http://stackoverflow.com/questions/10949876/mercurial-how-to-recover-from-an-interrupted-qpop-a and tried this solution. Now it is stuck in every hg command I am trying apart from "qapplied" and "hg qseries". Please help me with this.

Comment 4

[Manraj Singh [:manrajsingh]]

Attachment: Unifies popup notification icon CSS in browser/themes/shared/ to reduce duplication

Hi Sir,

Please ignore my previous comment. There was some issue. I have fixed it and am attaching the patch file.Please review it and let me know the changes. Thank you.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8585073 [details] [diff] [review]
Unifies popup notification icon CSS in browser/themes/shared/ to reduce duplication

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

Hi Manraj, you're on the right track. I have some questions below:

::: browser/themes/windows/browser.css
@@ +2198,2 @@
>  #webapps-notification-icon {
>    list-style-image: url(chrome://global/skin/icons/webapps-16.png);

What's the reason for leaving this one out of the new file?

@@ +2209,3 @@
>  .bad-content-blocked-notification-icon,
>  #bad-content-blocked-notification-icon {
>    list-style-image: url(chrome://browser/skin/bad-content-blocked-16.png);

Same question for this one and the one below

@@ +2219,2 @@
>  #pointerLock-notification-icon {
>    list-style-image: url(chrome://browser/skin/pointerLock-16.png);

This one too

Comment 6

[Manraj Singh [:manrajsingh]]

Hi sir,

I felt these were not common as one had Class along with Id while in Windows or Linux didn't have it. Please let me know should I include it in shared/notification-icons.inc.css or not.

Comment 7

[Matthew N. [:MattN]]

(In reply to Manraj Singh [:manrajsingh] from comment #6)
> I felt these were not common as one had Class along with Id while in Windows
> or Linux didn't have it. Please let me know should I include it in
> shared/notification-icons.inc.css or not.

Ok, I see that now. I think to be safe you should copy over the ruleset with both rules (class and ID) in those cases since I think the inconsistency is an oversight in the past and we can cleanup the duplication later in bug 1147981.

Comment 8

[Manraj Singh [:manrajsingh]]

Attachment: rev2 - Unifies popup notification icon CSS in browser/themes/shared/ to reduce duplication

Hi sir,

Please review the changes.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8587237 [details] [diff] [review]
rev2 - Unifies popup notification icon CSS in browser/themes/shared/ to reduce duplication

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

Thanks!

I converted the new file to by an hg copy of the Windows version to preserve version control blame, consolidated a few more rulesets and then pushed this.

https://hg.mozilla.org/integration/fx-team/rev/053673640c0d

Comment 10

[Matthew N. [:MattN]]

Moved the @2x rules to the same file: https://hg.mozilla.org/integration/fx-team/rev/3d4fb6a3ab9e

Comment 11

[Manraj Singh [:manrajsingh]]

Yeps sir. Even I thought of moving them to shared file as it would be easier for making changes in future.

Should we land this fix now?

Comment 12

[Matthew N. [:MattN]]

(In reply to Manraj Singh [:manrajsingh] from comment #11)
> Should we land this fix now?

Everything is landed already so this will get merged to mozilla-central within the next day.

Do you have another bug you want to work on? Perhaps you want to finish bug 728813 for someone else who stopped working on it?

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/053673640c0d
https://hg.mozilla.org/mozilla-central/rev/3d4fb6a3ab9e

Comment 14

[Manraj Singh [:manrajsingh]]

Sorry for late reply. Sure sir. I am currently out. I will go through the bug page and comment there.