Open Bug 1421039 Opened 2 years ago Updated 1 day ago

Remove duplicate extension icons

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(Not tracked)

People

(Reporter: mstriemer, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(5 files, 1 obsolete file)

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
Mentor: mstriemer
Keywords: good-first-bug
Priority: -- → P5
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
(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??
If you have a patch ready, by all means go ahead and upload it!
Assignee: nobody → architjugran
(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
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)
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!
(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!
Attached file Bug1421039
here is my patch file can I get feedback ?
Attachment #8935395 - Flags: review?(mstriemer)
review ping?
Flags: needinfo?(mstriemer)
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.
Flags: needinfo?(mstriemer)
Attachment #8935414 - Flags: review?(mstriemer)
Comment on attachment 8935395 [details]
Bug1421039

This doesn't seem to be what we're looking for.
Attachment #8935395 - Flags: review?(mstriemer) → review-
(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
(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
Flags: needinfo?(mstriemer)
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?
Flags: needinfo?(mstriemer)
Product: Toolkit → WebExtensions
:Archit
are u still working on this?
Flags: needinfo?(architjugran)
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!
Assignee: architjugran → nobody
Hi, I would like to work on this issue.
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.
Assignee: nobody → shivams2799
Flags: needinfo?(architjugran)
Hey Shivam, are you still working on this?
Flags: needinfo?(shivams2799)
Hey Caitlin,

Exams are coming up :/ , will working on it as soon as i get free with the exams :)

Hope you are doing good :)
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? ;)
(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 :(
Flags: needinfo?(shivams2799) → needinfo?(cneiman)
(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.
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!
Assignee: shivams2799 → naman
Flags: needinfo?(cneiman)

Hey Naman, how's it going with this bug?

Flags: needinfo?(naman)

(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.

Flags: needinfo?(naman)

Sorry to hear about that, but we appreciate the update. We'll unassign you from this bug.

Assignee: naman → nobody

Hey, can I take this up?

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

Assignee: nobody → sresthasrivastava.ss

Hey Srestha, how's it going with this bug?

Flags: needinfo?(sresthasrivastava.ss)

Hey, sorry for the late reply, I got busy with my exams, I'll get it done in a day or two.

Flags: needinfo?(sresthasrivastava.ss)

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

Flags: needinfo?(mstriemer)

That sounds reasonable. If you send a PR I can take a look.

Flags: needinfo?(mstriemer)

Hey Srestha, how's it going with this bug? Is there anything we can do to help unblock you?

Flags: needinfo?(sresthasrivastava.ss)

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.

Assignee: sresthasrivastava.ss → nobody

Can I work on this issue? As this is my first contribution I need some helps from someone.

Flags: needinfo?(mstriemer)

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!

Flags: needinfo?(sresthasrivastava.ss)
Flags: needinfo?(mstriemer)

(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?

Hi Travis! Absolutely. Let's give Christkiran until October 9 to respond and then it's all yours. :)

(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.

(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.

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

@mstriemer and @caitmuenster - I submitted my commits in Phabricator for your review. I replaced the specified icons and updated the manifests.

Attachment #9009952 - Attachment is obsolete: true
Blocks: 1599577
Assignee: nobody → travisjvirgil

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: travisjvirgil → nobody
You need to log in before you can comment on or make changes to this bug.