Remove duplicate extension icons
Categories
(WebExtensions :: Frontend, enhancement, P5)
Tracking
(firefox76 fixed)
| Tracking | Status | |
|---|---|---|
| firefox76 | --- | fixed |
People
(Reporter: mstriemer, Assigned: aji.yash13, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 7 obsolete files)
There are some duplicate versions of the extension icon. Some are green, some use context fill and some are a slightly different shape. We should try to reduce the number of icons to 1, but at most 2. One of the icons is the default extension icon so having one for extensions and one for Firefox might make sense. browser/themes/shared/addons/addon-install-confirm.svg browser/themes/shared/controlcenter/extension.svg toolkit/themes/shared/extensions/extensionGeneric-16.svg browser/components/extensions/extension.svg
| Reporter | ||
Updated•4 years ago
|
Comment 1•4 years ago
|
||
I would like to take up this bug as my first ever bug. I got a decent idea that i will search for the files used in code in https://searchfox.org/ and then replace the exactly same files with one path in code and delete redundant files
Comment 2•4 years ago
|
||
(In reply to Archit Jugran from comment #1) > I would like to take up this bug as my first ever bug. I got a decent idea > that i will search for the files used in code in https://searchfox.org/ and > then replace the exactly same files with one path in code and delete > redundant files Can i upload patch or do i have to wait for the bug to get assigned??
Comment 3•4 years ago
|
||
If you have a patch ready, by all means go ahead and upload it!
Comment 4•4 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #3) > If you have a patch ready, by all means go ahead and upload it! I just have one doubt . When i search in mozilla central for addon-confirm file, it shows a weird occurence https://dxr.mozilla.org/mozilla-central/search?q=addon-install-confirm.svg&redirect=true The weird occurence is in file allowed-dupes.mn where there is a non existent file path to the very file that i want to remove i.e. addon-install-confirm.svg since it is same as the fourth file in the above list and has less occurences than the fourth. Also in CSS file the url ,i cannot understand eg. url(chrome://browser/skin/controlcenter/extension.svg); ..where did this come from when the file it is pointing to has a different path : browser/themes/shared/controlcenter/extension.svg
Comment 5•4 years ago
|
||
I guess i can replace the common url for a file with the common url for another duplicate file in CSS.. but i would be grateful if someone could explain the URL format to me (like my doubt in the above comment)
| Reporter | ||
Comment 6•4 years ago
|
||
There are two icons that are green with a gradient and two files that are grey. There is probably a way to get us down to one file but due to how we're using the icons it might be tricky. The allowed-dupes.mn file entry is because browser/themes/shared/addons/addon-install-confirm.svg and browser/components/extensions/extension.svg are identical. I'd say we go with the browser/components/extensions/extension.svg version. You should be able to remove references to both files from allowed-dupes.mn when that's done. For the other two I think browser/themes/shared/controlcenter/extension.svg is a better file to standardize on. It probably makes sense to move the file to toolkit/themes/shared/extensions/extension.svg though. As far as where the chrome:// URL comes from, it should depend on the manifest files. If you change the extensionGeneric-16.svg manifest references to extension.svg you should be able to reference the image at chrome://mozapps/skin/extensions/extension.svg.
Comment 7•4 years ago
|
||
PLease can you elaborate about the manifest files ?I did not understand that part. Thank you!
Comment 8•4 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #6) > There are two icons that are green with a gradient and two files that are > grey. There is probably a way to get us down to one file but due to how > we're using the icons it might be tricky. > > The allowed-dupes.mn file entry is because > browser/themes/shared/addons/addon-install-confirm.svg and > browser/components/extensions/extension.svg are identical. I'd say we go > with the browser/components/extensions/extension.svg version. You should be > able to remove references to both files from allowed-dupes.mn when that's > done. > > For the other two I think browser/themes/shared/controlcenter/extension.svg > is a better file to standardize on. It probably makes sense to move the file > to toolkit/themes/shared/extensions/extension.svg though. > > As far as where the chrome:// URL comes from, it should depend on the > manifest files. If you change the extensionGeneric-16.svg manifest > references to extension.svg you should be able to reference the image at > chrome://mozapps/skin/extensions/extension.svg. PLease can you elaborate about the manifest files ?I did not understand that part. Thank you!
Comment 9•4 years ago
|
||
here is my patch file can I get feedback ?
Comment 10•4 years ago
|
||
| Reporter | ||
Comment 12•4 years ago
|
||
Comment on attachment 8935414 [details] [diff] [review] I removed all instances of browser/themes/shared/addons/addon-install-confirm.svg and changed them to instances of browser/components/extensions/extension.svg and similarly for the other pair Thanks for the patch Archit, sorry for the late reply. I had to make a few changes to get this to build. The controlcenter styles should be updated to use the regular extension.svg. diff --git a/browser/themes/shared/controlcenter/panel.inc.css b/browser/themes/shared/controlcenter/panel.inc.css --- a/browser/themes/shared/controlcenter/panel.inc.css +++ b/browser/themes/shared/controlcenter/panel.inc.css @@ -207,7 +207,7 @@ } #identity-popup[connection=extension] .identity-popup-security-content { - background-image: url(chrome://browser/skin/controlcenter/extension.svg); + background-image: url(chrome://mozapps/skin/extensions/extension.svg); -moz-context-properties: fill; fill: #60bf4c; } The manifest files need to be updated to point to the new file location. diff --git a/browser/themes/shared/jar.inc.mn b/browser/themes/shared/jar.inc.mn --- a/browser/themes/shared/jar.inc.mn +++ b/browser/themes/shared/jar.inc.mn @@ -23,7 +23,6 @@ * skin/classic/browser/controlcenter/conn-not-secure.svg (../shared/controlcenter/conn-not-secure.svg) skin/classic/browser/controlcenter/connection.svg (../shared/controlcenter/connection.svg) skin/classic/browser/controlcenter/mcb-disabled.svg (../shared/controlcenter/mcb-disabled.svg) - skin/classic/browser/controlcenter/extension.svg (../shared/controlcenter/extension.svg) * skin/classic/browser/controlcenter/permissions.svg (../shared/controlcenter/permissions.svg) * skin/classic/browser/controlcenter/tracking-protection.svg (../shared/controlcenter/tracking-protection.svg) skin/classic/browser/controlcenter/warning-gray.svg (../shared/controlcenter/warning-gray.svg) diff --git a/toolkit/themes/shared/mozapps.inc.mn b/toolkit/themes/shared/mozapps.inc.mn --- a/toolkit/themes/shared/mozapps.inc.mn +++ b/toolkit/themes/shared/mozapps.inc.mn @@ -17,7 +17,7 @@ skin/classic/mozapps/extensions/category-recent.svg (../../shared/extensions/category-recent.svg) skin/classic/mozapps/extensions/category-available.svg (../../shared/extensions/category-available.svg) - skin/classic/mozapps/extensions/extension.svg (../../shared/controlcenter/extension.svg) + skin/classic/mozapps/extensions/extension.svg (../../shared/extensions/extension.svg) skin/classic/mozapps/extensions/utilities.svg (../../shared/extensions/utilities.svg) skin/classic/mozapps/extensions/alerticon-warning.svg (../../shared/extensions/alerticon-warning.svg) skin/classic/mozapps/extensions/alerticon-error.svg (../../shared/extensions/alerticon-error.svg) And the browser/themes/shared/controlcenter/extension.svg icon should move to toolkit/themes/shared/extensions/extension.svg. If you search for the other file names locally after making these changes it looks like there might still be a few references to them leftover.
| Reporter | ||
Comment 13•4 years ago
|
||
Comment on attachment 8935395 [details] Bug1421039 This doesn't seem to be what we're looking for.
Comment 14•4 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #12) > Comment on attachment 8935414 [details] [diff] [review] > I removed all instances of > browser/themes/shared/addons/addon-install-confirm.svg and changed them to > instances of browser/components/extensions/extension.svg and similarly for > the other pair > > Thanks for the patch Archit, sorry for the late reply. > > I had to make a few changes to get this to build. > > The controlcenter styles should be updated to use the regular extension.svg. > > diff --git a/browser/themes/shared/controlcenter/panel.inc.css > b/browser/themes/shared/controlcenter/panel.inc.css > --- a/browser/themes/shared/controlcenter/panel.inc.css > +++ b/browser/themes/shared/controlcenter/panel.inc.css > @@ -207,7 +207,7 @@ > } > > #identity-popup[connection=extension] .identity-popup-security-content { > - background-image: url(chrome://browser/skin/controlcenter/extension.svg); > + background-image: url(chrome://mozapps/skin/extensions/extension.svg); > -moz-context-properties: fill; > fill: #60bf4c; > } > > The manifest files need to be updated to point to the new file location. > > diff --git a/browser/themes/shared/jar.inc.mn > b/browser/themes/shared/jar.inc.mn > --- a/browser/themes/shared/jar.inc.mn > +++ b/browser/themes/shared/jar.inc.mn > @@ -23,7 +23,6 @@ > * skin/classic/browser/controlcenter/conn-not-secure.svg > (../shared/controlcenter/conn-not-secure.svg) > skin/classic/browser/controlcenter/connection.svg > (../shared/controlcenter/connection.svg) > skin/classic/browser/controlcenter/mcb-disabled.svg > (../shared/controlcenter/mcb-disabled.svg) > - skin/classic/browser/controlcenter/extension.svg > (../shared/controlcenter/extension.svg) > * skin/classic/browser/controlcenter/permissions.svg > (../shared/controlcenter/permissions.svg) > * skin/classic/browser/controlcenter/tracking-protection.svg > (../shared/controlcenter/tracking-protection.svg) > skin/classic/browser/controlcenter/warning-gray.svg > (../shared/controlcenter/warning-gray.svg) > diff --git a/toolkit/themes/shared/mozapps.inc.mn > b/toolkit/themes/shared/mozapps.inc.mn > --- a/toolkit/themes/shared/mozapps.inc.mn > +++ b/toolkit/themes/shared/mozapps.inc.mn > @@ -17,7 +17,7 @@ > skin/classic/mozapps/extensions/category-recent.svg > (../../shared/extensions/category-recent.svg) > skin/classic/mozapps/extensions/category-available.svg > (../../shared/extensions/category-available.svg) > > - skin/classic/mozapps/extensions/extension.svg > (../../shared/controlcenter/extension.svg) > + skin/classic/mozapps/extensions/extension.svg > (../../shared/extensions/extension.svg) > skin/classic/mozapps/extensions/utilities.svg > (../../shared/extensions/utilities.svg) > skin/classic/mozapps/extensions/alerticon-warning.svg > (../../shared/extensions/alerticon-warning.svg) > skin/classic/mozapps/extensions/alerticon-error.svg > (../../shared/extensions/alerticon-error.svg) > > And the browser/themes/shared/controlcenter/extension.svg icon should move > to toolkit/themes/shared/extensions/extension.svg. > > If you search for the other file names locally after making these changes it > looks like there might still be a few references to them leftover. But those are references to other files having same name extension.svg , not the extension.svg i deleted
Comment 15•4 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #12) > Comment on attachment 8935414 [details] [diff] [review] > I removed all instances of > browser/themes/shared/addons/addon-install-confirm.svg and changed them to > instances of browser/components/extensions/extension.svg and similarly for > the other pair > > Thanks for the patch Archit, sorry for the late reply. > > I had to make a few changes to get this to build. > > The controlcenter styles should be updated to use the regular extension.svg. > > diff --git a/browser/themes/shared/controlcenter/panel.inc.css > b/browser/themes/shared/controlcenter/panel.inc.css > --- a/browser/themes/shared/controlcenter/panel.inc.css > +++ b/browser/themes/shared/controlcenter/panel.inc.css > @@ -207,7 +207,7 @@ > } > > #identity-popup[connection=extension] .identity-popup-security-content { > - background-image: url(chrome://browser/skin/controlcenter/extension.svg); > + background-image: url(chrome://mozapps/skin/extensions/extension.svg); > -moz-context-properties: fill; > fill: #60bf4c; > } > > The manifest files need to be updated to point to the new file location. > > diff --git a/browser/themes/shared/jar.inc.mn > b/browser/themes/shared/jar.inc.mn > --- a/browser/themes/shared/jar.inc.mn > +++ b/browser/themes/shared/jar.inc.mn > @@ -23,7 +23,6 @@ > * skin/classic/browser/controlcenter/conn-not-secure.svg > (../shared/controlcenter/conn-not-secure.svg) > skin/classic/browser/controlcenter/connection.svg > (../shared/controlcenter/connection.svg) > skin/classic/browser/controlcenter/mcb-disabled.svg > (../shared/controlcenter/mcb-disabled.svg) > - skin/classic/browser/controlcenter/extension.svg > (../shared/controlcenter/extension.svg) > * skin/classic/browser/controlcenter/permissions.svg > (../shared/controlcenter/permissions.svg) > * skin/classic/browser/controlcenter/tracking-protection.svg > (../shared/controlcenter/tracking-protection.svg) > skin/classic/browser/controlcenter/warning-gray.svg > (../shared/controlcenter/warning-gray.svg) > diff --git a/toolkit/themes/shared/mozapps.inc.mn > b/toolkit/themes/shared/mozapps.inc.mn > --- a/toolkit/themes/shared/mozapps.inc.mn > +++ b/toolkit/themes/shared/mozapps.inc.mn > @@ -17,7 +17,7 @@ > skin/classic/mozapps/extensions/category-recent.svg > (../../shared/extensions/category-recent.svg) > skin/classic/mozapps/extensions/category-available.svg > (../../shared/extensions/category-available.svg) > > - skin/classic/mozapps/extensions/extension.svg > (../../shared/controlcenter/extension.svg) > + skin/classic/mozapps/extensions/extension.svg > (../../shared/extensions/extension.svg) > skin/classic/mozapps/extensions/utilities.svg > (../../shared/extensions/utilities.svg) > skin/classic/mozapps/extensions/alerticon-warning.svg > (../../shared/extensions/alerticon-warning.svg) > skin/classic/mozapps/extensions/alerticon-error.svg > (../../shared/extensions/alerticon-error.svg) > > And the browser/themes/shared/controlcenter/extension.svg icon should move > to toolkit/themes/shared/extensions/extension.svg. > > If you search for the other file names locally after making these changes it > looks like there might still be a few references to them leftover. But those are references to other files having same name , not the file i deleted
Comment 16•3 years ago
|
||
Archit, sorry for the long delay in replying. If you're still interested in working on this, I think Mark's point was that we shouldn't have an extension.svg in the controlcenter/ directory at all. We should just have extension.svg under extensions/ and update the styles for controlcenter to refer to that copy. Does that make sense?
Updated•3 years ago
|
Comment 18•3 years ago
|
||
Hey Archit! Since we haven't heard from you in awhile, we're going to unassign you from this bug and open it up to other contributors. If you would like to come back and finish the patch, please feel free to re-assign yourself!
Comment 19•3 years ago
|
||
Hi, I would like to work on this issue.
Comment 20•3 years ago
|
||
Go for it, Shivam! If you haven't done so already, please set up your development environment and try to find the code using searchfox.org.
Comment 21•3 years ago
|
||
removing dups icons
Comment 23•3 years ago
|
||
Hey Caitlin, Exams are coming up :/ , will working on it as soon as i get free with the exams :) Hope you are doing good :)
Comment 24•3 years ago
|
||
Hey Shivam! Just checking in because it's been awhile since the last update on this bug. It looks like you're headed in the right direction on this bug -- do you want to give it another go and finish it off? ;)
Comment 25•3 years ago
|
||
(In reply to Caitlin Neiman [:caitmuenster] from comment #24) > Hey Shivam! Just checking in because it's been awhile since the last update > on this bug. It looks like you're headed in the right direction on this bug > -- do you want to give it another go and finish it off? ;) Hi Caitlin, Really sorry for the delay, I have this bug in my mind and will sure take a look on it after my last exam for this semester on 15th Dec. Actually everytime I need to update any Gecko related bug I have to shift to another OS apart from my main OS (Debian 9) on which there are some issues on Debian related to LLVM/Clang. Also I need to connect through LAN as in Fedora OS , Wifi Card of my laptop does not work at all :(
Comment 26•2 years ago
|
||
(In reply to Shivam Singhal [ :championshuttler ] from comment #25) > (In reply to Caitlin Neiman [:caitmuenster] from comment #24) > > Hey Shivam! Just checking in because it's been awhile since the last update > > on this bug. It looks like you're headed in the right direction on this bug > > -- do you want to give it another go and finish it off? ;) > > Hi Caitlin, Really sorry for the delay, I have this bug in my mind and will > sure take a look on it after my last exam for this semester on 15th Dec. > Actually everytime I need to update any Gecko related bug I have to shift to > another OS apart from my main OS (Debian 9) on which there are some issues > on Debian related to LLVM/Clang. Also I need to connect through LAN as in > Fedora OS , Wifi Card of my laptop does not work at all :( Hi Shivam, are you working on this bug? If not, then I would like to take this up.
Comment 27•2 years ago
|
||
Hey Naman! I talked with Shivam via email earlier and given the problems he's having with his machine, we can go ahead and re-assign this bug to you. If you haven't already done so, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for instructions on how to get started. Happy bug fixing!
Comment 29•2 years ago
|
||
(In reply to Caitlin Neiman [:caitmuenster] from comment #28)
Hey Naman, how's it going with this bug?
Hey Caitlin, actually I am unable to run web extension project on my system. There are performance issues.
Comment 30•2 years ago
|
||
Sorry to hear about that, but we appreciate the update. We'll unassign you from this bug.
Comment 31•2 years ago
|
||
Hey, can I take this up?
Comment 32•2 years ago
|
||
Hey Srestha, go for it! If you haven't done so already, you might want to read our onboarding guide here: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
Comment 33•2 years ago
|
||
Hey Srestha, how's it going with this bug?
Comment 34•2 years ago
|
||
Hey, sorry for the late reply, I got busy with my exams, I'll get it done in a day or two.
Comment 35•2 years ago
|
||
Hey,@mstriemer, so as the [1] and [2] are similar,I've decided to replace [1] with [2] and [4] with [3](as you said in https://phabricator.services.mozilla.com/D6153 that "using the smaller icon for the bigger one looks fine though, so maybe we should keep the small icon around instead"). This way, we will have only two icons. Please let me know if that works, I'll create the PR.
1.mozilla-central/browser/themes/shared/addons/addon-install-confirm.svg
2.mozilla-central/browser/components/extensions/extension.svg
3.mozilla-central/toolkit/themes/shared/extensions/extensionGeneric-16.svg
4.mozilla-central/browser/themes/shared/controlcenter/extension.svg
Updated•2 years ago
|
| Reporter | ||
Comment 36•2 years ago
|
||
That sounds reasonable. If you send a PR I can take a look.
Comment 37•2 years ago
|
||
Hey Srestha, how's it going with this bug? Is there anything we can do to help unblock you?
Comment 38•2 years ago
|
||
There hasn't been much activity on this, so we are re-opening this for everyone to work on. We are happy to assign a contributor after they submitted a patch.
Comment 39•2 years ago
|
||
Can I work on this issue? As this is my first contribution I need some helps from someone.
Comment 40•2 years ago
|
||
Hey Christkiran, so sorry for the delayed response! Yes, you can absolutely work on this. To get started, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp.
It's best to needinfo Mark (the mentor) once you have specific questions. :) Once you have a patch for this on Phabricator, we will assign this issue to you.
Happy bug fixing!
Comment 41•2 years ago
|
||
(In reply to Caitlin Neiman [:caitmuenster] from comment #40)
Hey Christkiran, so sorry for the delayed response! Yes, you can absolutely work on this. To get started, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp.
It's best to needinfo Mark (the mentor) once you have specific questions. :) Once you have a patch for this on Phabricator, we will assign this issue to you.
Happy bug fixing!
Hi Caitlin, if Christkiran does not get back to you about this bug can I work on it?
Comment 42•2 years ago
|
||
Hi Travis! Absolutely. Let's give Christkiran until October 9 to respond and then it's all yours. :)
Comment 43•2 years ago
|
||
(In reply to Caitlin Neiman [:caitmuenster] from comment #42)
Hi Travis! Absolutely. Let's give Christkiran until October 9 to respond and then it's all yours. :)
Going to assume I can start working on this since we have not heard anything.
Comment 44•2 years ago
|
||
(In reply to Caitlin Neiman [:caitmuenster] from comment #42)
Hi Travis! Absolutely. Let's give Christkiran until October 9 to respond and then it's all yours. :)
(In reply to Travis Virgil from comment #43)
(In reply to Caitlin Neiman [:caitmuenster] from comment #42)
Hi Travis! Absolutely. Let's give Christkiran until October 9 to respond and then it's all yours. :)
Going to assume I can start working on this since we have not heard anything.
I finished making the corrections for the extension files. I just need to submit to Phabricator for a review.
Comment 45•2 years ago
|
||
Comment 46•2 years ago
|
||
Replace browser/themes/shared/addons/addon-install-confirm.svg with browser/components/extensions/extension.svg
Replace toolkit/themes/shared/extensions/extensionGeneric-16.svg with browser/themes/shared/controlcenter/extension.svg
Relocate browser/themes/shared/controlcenter/extension.svg to toolkit/themes/shared/extensions/extension.svg
Depends on D53914
Comment 47•2 years ago
|
||
@mstriemer and @caitmuenster - I submitted my commits in Phabricator for your review. I replaced the specified icons and updated the manifests.
Updated•2 years ago
|
Comment 48•2 years ago
|
||
Updated•2 years ago
|
Comment 49•1 year ago
|
||
Hey Travis, since we haven't heard from you in awhile, we're going to unassign this bug. If you'd like to continue working on it, please let us know and we'll reassign it!
| Assignee | ||
Comment 50•1 year ago
|
||
Hello, I would like to submit a patch for this bug, but there is a previous patch with [Needs Revision ] status. So do i need to submit a complete new patch or enhancing the previously submitted patch would be appropriate ?
| Assignee | ||
Comment 51•1 year ago
|
||
I am going to upload a new patch.
| Assignee | ||
Comment 52•1 year ago
|
||
Updated•1 year ago
|
| Assignee | ||
Comment 53•1 year ago
|
||
Depends on D63288
Any update on this? :)
| Assignee | ||
Comment 55•1 year ago
|
||
Hello Mark, can you please have a look over the patch i have submitted ?
| Reporter | ||
Updated•1 year ago
|
| Reporter | ||
Comment 56•1 year ago
|
||
I'll take a look at the patches tomorrow.
| Assignee | ||
Comment 57•1 year ago
|
||
ok thanks
Comment 58•1 year ago
|
||
Pushed by mstriemer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/18a09d7d8422 Remove duplicate extension icons r=mstriemer
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 59•1 year ago
|
||
| bugherder | ||
| Reporter | ||
Updated•1 year ago
|
Description
•