Closed Bug 1185606 Opened 5 years ago Closed 4 years ago

Use windows 10 urlbar icons on Windows 8

Categories

(Firefox :: Theme, defect)

All
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: ntim, Assigned: chowdhuryshaif, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(2 files, 4 obsolete files)

They fit better into Windows 8 than the current Aero style ones.
Would you like to take this bug, Tim?
(In reply to Dão Gottwald [:dao] from comment #1)
> Would you like to take this bug, Tim?
Probably not now, but I can mentor this bug.
Mentor: ntim.bugs
Whiteboard: [good first bug][lang=css]
All of this is done in browser/themes/windows.
Here's what needs to be done : 
- perform an hg rename (replacing preWin10 with XPVista7) on :
urlbar-history-dropmarker-preWin10.png
urlbar-history-dropmarker-preWin10@2x.png
reload-stop-go-preWin10.png
reload-stop-go-preWin10@2x.png

- In the jar.mn file, update the references to these files as well. In the override part (0), replace 6.3 with 6.2

(0) : https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/jar.mn#702
(In reply to Tim Nguyen [:ntim] from comment #3)
> - In the jar.mn file, update the references to these files as well. In the
> override part (0), replace 6.3 with 6.2

Whoops, meant 6.1 not 6.2
I would like to work on this bug.
I have to replace 6.3 with 6.1 for those lines @Tim for the prior os support? am I rite?
Flags: needinfo?(ntim.bugs)
(In reply to Bhuvanesh from comment #6)
> I have to replace 6.3 with 6.1 for those lines @Tim for the prior os
> support? am I rite?

Changing this means the old aero style images will be used for versions 6.1 (Windows 7) and lower instead of version 6.3 (Windows 8.1) and lower. By doing so, the newer modern style images will also be used on Windows 8.
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen [:ntim] from comment #7)
> (In reply to Bhuvanesh from comment #6)
> > I have to replace 6.3 with 6.1 for those lines @Tim for the prior os
> > support? am I rite?
> 
> Changing this means the old aero style images will be used for versions 6.1
> (Windows 7) and lower instead of version 6.3 (Windows 8.1) and lower. By
> doing so, the newer modern style images will also be used on Windows 8.

ok I would like to do it.Thank you @tim.
(In reply to Bhuvanesh from comment #8)
> ok I would like to do it.Thank you @tim.

Assigned it to you. Did you set up your build environment yet ?
Assignee: nobody → bhuvanesh0712
Status: NEW → ASSIGNED
(In reply to Tim Nguyen [:ntim] from comment #9)
> (In reply to Bhuvanesh from comment #8)
> > ok I would like to do it.Thank you @tim.
> 
> Assigned it to you. Did you set up your build environment yet ?

I installed mercurial ,have cloned the repo mozilla-central and created a branch

The problem is how to test my changes @tim since I use mac
(In reply to Bhuvanesh from comment #10)
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > (In reply to Bhuvanesh from comment #8)
> > > ok I would like to do it.Thank you @tim.
> > 
> > Assigned it to you. Did you set up your build environment yet ?
> 
> I installed mercurial ,have cloned the repo mozilla-central and created a
> branch
> 
> The problem is how to test my changes @tim since I use mac

Just create a patch and post it here, and I'll test it for you.
You can also use a Windows VM, and install the build prerequisites, but this takes quite some time to get done.
Bhuvanesh, are you currently working on this ?
Flags: needinfo?(bhuvanesh0712)
(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #13)
> Bhuvanesh, are you currently working on this ?

sorry tim was stuck with an accident will put the patch here this weekend once I get home
Flags: needinfo?(bhuvanesh0712)
Resetting because of inactivity. Bhuvanesh, if you'd still like to take this, feel free to ask so I can assign this bug to you again.
Assignee: bhuvanesh0712 → nobody
Status: ASSIGNED → NEW
I will work on this...
Assignee: nobody → chowdhuryshaif
Status: NEW → ASSIGNED
Attached patch bug1185606 (obsolete) — Splinter Review
Could you please test it?
Attachment #8672460 - Flags: review?(ntim.bugs)
Comment on attachment 8672460 [details] [diff] [review]
bug1185606

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

Thanks for the patch ! Unfortunately, the build will fail because you haven't renamed the actual files in the tree.

You can run this to rename the files:
cd browser/themes/windows 
hg rename oldname.png newname.png
Attachment #8672460 - Flags: review?(ntim.bugs) → feedback+
I think I did hg rename before submitting the patch..

>diff --git a/browser/themes/windows/reload-stop-go-preWin10.png b/browser/themes/windows/reload-stop-go-XPVista7.png
>rename from browser/themes/windows/reload-stop-go-preWin10.png
>rename to browser/themes/windows/reload-stop-go-XPVista7.png
>diff --git a/browser/themes/windows/reload-stop-go-preWin10@2x.png b/browser/themes/windows/reload-stop-go-XPVista7@2x.png
>rename from browser/themes/windows/reload-stop-go-preWin10@2x.png
>rename to browser/themes/windows/reload-stop-go-XPVista7@2x.png
>diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
>rename from browser/themes/windows/urlbar-history-dropmarker-preWin10.png
>rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
>diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png
>rename from browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png
>rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png
(In reply to Shaif from comment #19)
> I think I did hg rename before submitting the patch..
> 
> >diff --git a/browser/themes/windows/reload-stop-go-preWin10.png b/browser/themes/windows/reload-stop-go-XPVista7.png
> >rename from browser/themes/windows/reload-stop-go-preWin10.png
> >rename to browser/themes/windows/reload-stop-go-XPVista7.png
> >diff --git a/browser/themes/windows/reload-stop-go-preWin10@2x.png b/browser/themes/windows/reload-stop-go-XPVista7@2x.png
> >rename from browser/themes/windows/reload-stop-go-preWin10@2x.png
> >rename to browser/themes/windows/reload-stop-go-XPVista7@2x.png
> >diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
> >rename from browser/themes/windows/urlbar-history-dropmarker-preWin10.png
> >rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
> >diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png
> >rename from browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png
> >rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png

Oh sorry, it seems that the review system actually removes the hg renames from the displayed diffs. I'll test this patch later today.
(In reply to Tim Nguyen [:ntim] from comment #20)
> (In reply to Shaif from comment #19)
> > I think I did hg rename before submitting the patch..
> > 
> > >diff --git a/browser/themes/windows/reload-stop-go-preWin10.png b/browser/themes/windows/reload-stop-go-XPVista7.png
> > >rename from browser/themes/windows/reload-stop-go-preWin10.png
> > >rename to browser/themes/windows/reload-stop-go-XPVista7.png
> > >diff --git a/browser/themes/windows/reload-stop-go-preWin10@2x.png b/browser/themes/windows/reload-stop-go-XPVista7@2x.png
> > >rename from browser/themes/windows/reload-stop-go-preWin10@2x.png
> > >rename to browser/themes/windows/reload-stop-go-XPVista7@2x.png
> > >diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
> > >rename from browser/themes/windows/urlbar-history-dropmarker-preWin10.png
> > >rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7.png
> > >diff --git a/browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png b/browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png
> > >rename from browser/themes/windows/urlbar-history-dropmarker-preWin10@2x.png
> > >rename to browser/themes/windows/urlbar-history-dropmarker-XPVista7@2x.png
> 
> Oh sorry, it seems that the review system actually removes the hg renames
> from the displayed diffs. I'll test this patch later today.

Did you test the patch??
Comment on attachment 8672460 [details] [diff] [review]
bug1185606

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

Looks good to me ! This patch just needs peer review now.

(In reply to Sebastian H. [:archaeopteryx][:aryx] from comment #22)
> Created attachment 8673841 [details]
> screenshot reload button wo patch (top) and with patch applied (bottom)

Thanks !
Attachment #8672460 - Flags: feedback+ → review+
Attachment #8672460 - Flags: review?(jaws)
Comment on attachment 8672460 [details] [diff] [review]
bug1185606

>@@ -380,19 +380,19 @@ browser.jar:
> % override chrome://browser/skin/loop/toolbar@2x.png                  chrome://browser/skin/loop/toolbar-aero@2x.png                    os=WINNT osversion=6
> % override chrome://browser/skin/loop/toolbar@2x.png                  chrome://browser/skin/loop/toolbar-aero@2x.png                    os=WINNT osversion=6.1
> % override chrome://browser/skin/loop/toolbar@2x.png                  chrome://browser/skin/loop/toolbar-win8@2x.png                    os=WINNT osversion=6.2
> % override chrome://browser/skin/loop/toolbar@2x.png                  chrome://browser/skin/loop/toolbar-win8@2x.png                    os=WINNT osversion=6.3
> % override chrome://browser/skin/preferences/checkbox.png             chrome://browser/skin/preferences/checkbox-aero.png               os=WINNT osversion=6
> % override chrome://browser/skin/preferences/checkbox.png             chrome://browser/skin/preferences/checkbox-aero.png               os=WINNT osversion=6.1
> % override chrome://browser/skin/preferences/checkbox.png             chrome://browser/skin/preferences/checkbox-xp.png                 os=WINNT osversion<6
> 
>-% override chrome://browser/skin/reload-stop-go.png                   chrome://browser/skin/reload-stop-go-preWin10.png                 os=WINNT osversion<=6.3
>-% override chrome://browser/skin/reload-stop-go@2x.png                chrome://browser/skin/reload-stop-go-preWin10@2x.png              os=WINNT osversion<=6.3
>-% override chrome://browser/skin/urlbar-history-dropmarker.png        chrome://browser/skin/urlbar-history-dropmarker-preWin10.png      os=WINNT osversion<=6.3
>-% override chrome://browser/skin/urlbar-history-dropmarker@2x.png     chrome://browser/skin/urlbar-history-dropmarker-preWin10@2x.png   os=WINNT osversion<=6.3
>+% override chrome://browser/skin/reload-stop-go.png                   chrome://browser/skin/reload-stop-go-XPVista7.png                 os=WINNT osversion<=6.1
>+% override chrome://browser/skin/reload-stop-go@2x.png                chrome://browser/skin/reload-stop-go-XPVista7@2x.png              os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker.png        chrome://browser/skin/urlbar-history-dropmarker-XPVista7.png      os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker@2x.png     chrome://browser/skin/urlbar-history-dropmarker-XPVista7@2x.png   os=WINNT osversion<=6.1

Please move this to the existing section with osversion<=6.1 overrides: http://hg.mozilla.org/mozilla-central/annotate/4f4615ffec6a/browser/themes/windows/jar.mn#l325
Attachment #8672460 - Flags: review?(jaws)
Attached patch bug1185606.diff (obsolete) — Splinter Review
Attachment #8674781 - Flags: review?(dao)
Comment on attachment 8674781 [details] [diff] [review]
bug1185606.diff

>@@ -333,16 +333,20 @@ browser.jar:
> % override chrome://browser/skin/toolbarbutton-dropdown-arrow.png     chrome://browser/skin/toolbarbutton-dropdown-arrow-XPVista7.png   os=WINNT osversion<=6.1
> % override chrome://browser/skin/places/autocomplete-star.png         chrome://browser/skin/places/autocomplete-star-XPVista7.png       os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab.png                chrome://browser/skin/tabbrowser/newtab-XPVista7.png              os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab@2x.png             chrome://browser/skin/tabbrowser/newtab-XPVista7@2x.png           os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab-inverted.png       chrome://browser/skin/tabbrowser/newtab-inverted-XPVista7.png     os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab-inverted@2x.png    chrome://browser/skin/tabbrowser/newtab-inverted-XPVista7@2x.png  os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/tab-arrow-left.png        chrome://browser/skin/tabbrowser/tab-arrow-left-XPVista7.png      os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/tab-arrow-left@2x.png     chrome://browser/skin/tabbrowser/tab-arrow-left-XPVista7@2x.png   os=WINNT osversion<=6.1
>+% override chrome://browser/skin/reload-stop-go.png                   chrome://browser/skin/reload-stop-go-XPVista7.png                 os=WINNT osversion<=6.1
>+% override chrome://browser/skin/reload-stop-go@2x.png                chrome://browser/skin/reload-stop-go-XPVista7@2x.png              os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker.png        chrome://browser/skin/urlbar-history-dropmarker-XPVista7.png      os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker@2x.png     chrome://browser/skin/urlbar-history-dropmarker-XPVista7@2x.png   os=WINNT osversion<=6.1

Another nit: please keep this list sorted alphabetically
Attachment #8674781 - Flags: review?(dao)
Attached patch bug1185606.diff (obsolete) — Splinter Review
Attachment #8672460 - Attachment is obsolete: true
Attachment #8674781 - Attachment is obsolete: true
Attachment #8674850 - Flags: review?(dao)
Comment on attachment 8674850 [details] [diff] [review]
bug1185606.diff

>@@ -327,22 +327,26 @@ browser.jar:
> % override chrome://browser/skin/sync-horizontalbar.png               chrome://browser/skin/sync-horizontalbar-XPVista7.png             os=WINNT osversion<=6.1
> % override chrome://browser/skin/sync-horizontalbar@2x.png            chrome://browser/skin/sync-horizontalbar-XPVista7@2x.png          os=WINNT osversion<=6.1
> % override chrome://browser/skin/syncProgress-horizontalbar.png       chrome://browser/skin/syncProgress-horizontalbar-XPVista7.png     os=WINNT osversion<=6.1
> % override chrome://browser/skin/syncProgress-horizontalbar@2x.png    chrome://browser/skin/syncProgress-horizontalbar-XPVista7@2x.png  os=WINNT osversion<=6.1
> % override chrome://browser/skin/syncProgress-toolbar.png             chrome://browser/skin/syncProgress-toolbar-XPVista7.png           os=WINNT osversion<=6.1
> % override chrome://browser/skin/syncProgress-toolbar@2x.png          chrome://browser/skin/syncProgress-toolbar-XPVista7@2x.png        os=WINNT osversion<=6.1
> % override chrome://browser/skin/toolbarbutton-dropdown-arrow.png     chrome://browser/skin/toolbarbutton-dropdown-arrow-XPVista7.png   os=WINNT osversion<=6.1
> % override chrome://browser/skin/places/autocomplete-star.png         chrome://browser/skin/places/autocomplete-star-XPVista7.png       os=WINNT osversion<=6.1
>+% override chrome://browser/skin/reload-stop-go.png                   chrome://browser/skin/reload-stop-go-XPVista7.png                 os=WINNT osversion<=6.1
>+% override chrome://browser/skin/reload-stop-go@2x.png                chrome://browser/skin/reload-stop-go-XPVista7@2x.png              os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab.png                chrome://browser/skin/tabbrowser/newtab-XPVista7.png              os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab@2x.png             chrome://browser/skin/tabbrowser/newtab-XPVista7@2x.png           os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab-inverted.png       chrome://browser/skin/tabbrowser/newtab-inverted-XPVista7.png     os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/newtab-inverted@2x.png    chrome://browser/skin/tabbrowser/newtab-inverted-XPVista7@2x.png  os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/tab-arrow-left.png        chrome://browser/skin/tabbrowser/tab-arrow-left-XPVista7.png      os=WINNT osversion<=6.1
> % override chrome://browser/skin/tabbrowser/tab-arrow-left@2x.png     chrome://browser/skin/tabbrowser/tab-arrow-left-XPVista7@2x.png   os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker.png        chrome://browser/skin/urlbar-history-dropmarker-XPVista7.png      os=WINNT osversion<=6.1
>+% override chrome://browser/skin/urlbar-history-dropmarker@2x.png     chrome://browser/skin/urlbar-history-dropmarker-XPVista7@2x.png   os=WINNT osversion<=6.1

The sorting works slightly different here. First the files in the top-level directory (that's chrome://browser/skin/), then sub-directories. E.g.:

a.foo
x.foo
a/a.foo
a/b.foo
b/a.foo
Attachment #8674850 - Flags: review?(dao)
Attached patch bug1185606.diff (obsolete) — Splinter Review
Attachment #8674850 - Attachment is obsolete: true
Attachment #8674868 - Flags: review?(dao)
Comment on attachment 8674868 [details] [diff] [review]
bug1185606.diff

Thanks!
Attachment #8674868 - Flags: review?(dao) → review+
Keywords: checkin-needed
I tried to land this patch but it failed to apply. Is your repository up to date, Shaif?
Okk ,, hg status gives 
?F:\mozilla\browser\themes\windows/jar.mn

I am not sure if hg update /revert will work??  Or should I create a new repo..
(In reply to Shaif Chowdhury from comment #32)
> Okk ,, hg status gives 
> ?F:\mozilla\browser\themes\windows/jar.mn
> 
> I am not sure if hg update /revert will work??  Or should I create a new
> repo..

I usually fix patch conflicts using these commands :
- hg qpop -a
This unapplies all patches

- hg pull -u
This will update your local copy of the source code.

And then I reapply the patch, fix the conflicts and do hg qref
This should work.
Attachment #8675397 - Flags: review?(dao)
Comment on attachment 8675397 [details] [diff] [review]
bug1185606.diff (r=dao)

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

You don't need to re-request review once you get an r+ from a peer.
Attachment #8675397 - Flags: review?(dao) → review+
Comment on attachment 8675397 [details] [diff] [review]
bug1185606.diff (r=dao)

Just making it clear that I didn't grant the original r+, but :dao did.
Attachment #8675397 - Attachment description: bug1185606.diff → bug1185606.diff (r=dao)
https://hg.mozilla.org/mozilla-central/rev/4fb9ae267e5d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.