urlbar-tab.svg still has the old tab style

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: nevetia.vedant, Mentored)

Tracking

({good-first-bug})

58 Branch
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The "Switch to tab" icon in the URL bar still has the rounded corners of old:

https://searchfox.org/mozilla-central/source/browser/themes/shared/urlbar-tab.svg

We should probably replace it with something new.
We have a new icon :) You can find it here http://design.firefox.com/icons/viewer/#tab!
Sounds like a good first bug then.
Mentor: jhofmann
Keywords: good-first-bug
Also, jfyi the new icon is currently being used in the "Synced Tabs" view in the sidebar.
Priority: -- → P3
Assignee

Comment 4

2 years ago
I'd like to take up this bug if no one has taken it yet
Hello, 

I changed the icon with the one that is present in the comments above. 

Please let me know if the patch is good. 

I would appreciate timely feedback as this is part of one university module of mine. 

Thank you, 
Regards, 
Constantin
Attachment #8934743 - Flags: review?(jhofmann)
Assignee: nobody → paveless
Status: NEW → ASSIGNED
Comment on attachment 8934743 [details] [diff] [review]
changed tab icon with the new one in the comments above me

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

Hey, thanks for taking this bug!

It seems like you were working on a very outdated branch of mozilla-central (the original urlbar-tab.svg in your patch is from before May 24).

Can you try to pull a newer version and make a new commit on top of that?

Let me know if you have any questions!
Attachment #8934743 - Flags: review?(jhofmann)
Assignee

Comment 7

2 years ago
Posted patch patch file for bug 1421051 (obsolete) — Splinter Review
I've created and attached a patch fixing this issue.
Attachment #8935006 - Flags: review?(jhofmann)
Comment on attachment 8935006 [details] [diff] [review]
patch file for bug 1421051

Hi Vedant,

thank you for your contribution, the patch looks very good. Since Constantin had already started to work on the bug (and is officially assigned to it) I'd like to give him the chance to complete it, especially considering that this seems to be part of a course work. Hence I'm cancelling review on your patch. Please ask the person assigned to a bug before taking over their work in the future.

I'd love if you could take a stab at bug 1422846 instead, which is currently not assigned. If you leave a comment I will happily assign it to you.

Thanks for helping out!
Attachment #8935006 - Flags: review?(jhofmann)
Assignee

Comment 10

2 years ago
Apologies Johann,

I did not see that it had already been assigned. And thank you, I will take a look at it.
Hello, 

Thank you for that! I am surprised to see so much activity and timely responses on here! 

I appreciate that! I will update my codebase and redo the patch shortly. 

Thank you, 
Regards, 
Constantin
Hello, 

As it turned out, I was indeed working on a very outdated version of the codebase. 

I updated it now, so please tell me if this patch is all good. 

Thank you, and thank you for keeping me as the assignee of this bug. 

Regards, 
Constantin
Attachment #8935116 - Flags: review?(jhofmann)
(In reply to Constantin Pavelescu from comment #12)
> Created attachment 8935116 [details] [diff] [review]
> Patch using the updated version of the codebase
> 
> Hello, 
> 
> As it turned out, I was indeed working on a very outdated version of the
> codebase. 
> 
> I updated it now, so please tell me if this patch is all good. 
> 
> Thank you, and thank you for keeping me as the assignee of this bug. 
> 
> Regards, 
> Constantin

Sorry, I just checked it and the patch file seems to be empty. I will upload the good one shortly. Trying to figure out why it wrote an empty patch since I did exactly the same steps as I did before when it wrote a valid patch. 

Sorry for the inconvenience. 
Will be back shortly.
Hello, 

Sorry for the inconvenience, I still don't know why it didn't do a patch that time. 

Hopefully it's going to be fine now. 

Please assist me with the review. 

Thank you, 
Regards, 
Constantin
Attachment #8934743 - Attachment is obsolete: true
Attachment #8935116 - Attachment is obsolete: true
Attachment #8935116 - Flags: review?(jhofmann)
Attachment #8935123 - Flags: review?(jhofmann)
Comment on attachment 8935123 [details] [diff] [review]
Hopefully it's the good file this time

Sorry for the delay, I was out sick. Unfortunately this is basically the same patch as before, the changes can't be applied to a recent version of central.

Are you using Mercurial or Git? Did you run hg pull central and hg up central before applying your changes? You can find more info here:

http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#feature-development

Let me know if you have any questions!
Attachment #8935123 - Flags: review?(jhofmann)
Hi Constantin, are you still looking into this?
Flags: needinfo?(paveless)
Let's take Vedant's patch then!
Assignee: paveless → nevetia.vedant
Flags: needinfo?(paveless)
Attachment #8935006 - Attachment is obsolete: true
Attachment #8935123 - Attachment is obsolete: true

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac78853b7fc9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.