Closed
Bug 1185606
Opened 9 years ago
Closed 9 years ago
Use windows 10 urlbar icons on Windows 8
Categories
(Firefox :: Theme, defect)
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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
ntim
:
review+
|
Details | Diff | Splinter Review |
They fit better into Windows 8 than the current Aero style ones.
Comment 1•9 years ago
|
||
Would you like to take this bug, Tim?
Reporter | ||
Comment 2•9 years ago
|
||
(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.
Reporter | ||
Updated•9 years ago
|
Mentor: ntim.bugs
Whiteboard: [good first bug][lang=css]
Reporter | ||
Comment 3•9 years ago
|
||
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
Reporter | ||
Comment 4•9 years ago
|
||
(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 have to replace 6.3 with 6.1 for those lines @Tim for the prior os support? am I rite?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 7•9 years ago
|
||
(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.
Reporter | ||
Comment 9•9 years ago
|
||
(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
Comment 10•9 years ago
|
||
(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
Reporter | ||
Comment 11•9 years ago
|
||
(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.
Reporter | ||
Comment 12•9 years ago
|
||
You can also use a Windows VM, and install the build prerequisites, but this takes quite some time to get done.
Reporter | ||
Comment 13•9 years ago
|
||
Bhuvanesh, are you currently working on this ?
Flags: needinfo?(bhuvanesh0712)
Comment 14•9 years ago
|
||
(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)
Reporter | ||
Comment 15•9 years ago
|
||
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
Assignee | ||
Comment 16•9 years ago
|
||
I will work on this...
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → chowdhuryshaif
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•9 years ago
|
||
Could you please test it?
Attachment #8672460 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 18•9 years ago
|
||
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+
Assignee | ||
Comment 19•9 years ago
|
||
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
Reporter | ||
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
(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??
Reporter | ||
Comment 23•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Attachment #8672460 -
Flags: review?(jaws)
Comment 24•9 years ago
|
||
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)
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8674781 -
Flags: review?(dao)
Comment 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8672460 -
Attachment is obsolete: true
Attachment #8674781 -
Attachment is obsolete: true
Attachment #8674850 -
Flags: review?(dao)
Comment 28•9 years ago
|
||
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)
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8674850 -
Attachment is obsolete: true
Attachment #8674868 -
Flags: review?(dao)
Comment 30•9 years ago
|
||
Attachment #8674868 -
Flags: review?(dao) → review+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 31•9 years ago
|
||
I tried to land this patch but it failed to apply. Is your repository up to date, Shaif?
Assignee | ||
Comment 32•9 years ago
|
||
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..
Reporter | ||
Comment 33•9 years ago
|
||
(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
Reporter | ||
Comment 35•9 years ago
|
||
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+
Reporter | ||
Comment 36•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8674868 -
Attachment is obsolete: true
Comment 37•9 years ago
|
||
Keywords: checkin-needed
Comment 38•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in
before you can comment on or make changes to this bug.
Description
•