urlbar-tab.svg still has the old tab style

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

({good-first-bug})

58 Branch
Firefox 59
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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!
(Reporter)

Comment 2

a year ago
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

a year 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)
(Reporter)

Updated

a year ago
Assignee: nobody → paveless
Status: NEW → ASSIGNED
(Reporter)

Comment 6

a year ago
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

a year 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)
(Reporter)

Comment 9

a year ago
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

a year 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)
(Reporter)

Comment 15

a year ago
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)
(Reporter)

Comment 16

a year ago
Hi Constantin, are you still looking into this?
Flags: needinfo?(paveless)
(Reporter)

Updated

a year ago
Attachment #8935018 - Flags: review+
(Reporter)

Comment 17

a year ago
Let's take Vedant's patch then!
Assignee: paveless → nevetia.vedant
Flags: needinfo?(paveless)
(Reporter)

Updated

a year ago
Attachment #8935006 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8935123 - Attachment is obsolete: true

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac78853b7fc9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.