Closed Bug 1036494 Opened 7 years ago Closed 5 years ago

Combine stopwatch icons into one file

Categories

(DevTools :: General, defect)

Other
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: ntim, Unassigned, Mentored)

References

Details

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

Attachments

(2 files, 1 obsolete file)

We currently have 2 separate files for the stopwatch normal and checked state (after the hdpi work).
I just figured out that combining both in one file is easy. 
Here's the method :
- Add a active id to the whole icon (paths wrapped into a g tag)
- Add this style :
g:target {
  fill: /* checked blue color */
}
- Update references to checked stopwatch ([url]#active)
- Remove checked image

Since I don't have my laptop, I'm gonna leave this for someone else.
OS: Android → All
Hi can I get an opportunity to start my bugzilla journey with this bug?
Sure ! If you don't know how the process works, you can check codefirefox.com which provides great tutorials :)
Assignee: nobody → trishul.goel
Status: NEW → ASSIGNED
Hi Tim, can you point to any documentation to start, as I am totally new to this?
Flags: needinfo?(ntim007)
First, I'd recommend http://codefirefox.com for setting up your firefox build, managing, creating and submitting patches (pretty much everythig you need). I really recommend that site, since it provides some visual (video) tutorials that are easier to understand.

Mozilla developer network provides a bunch of documentation about the same stuff : https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Also, I recommend http://mxr.mozilla.org/mozilla-central to browse the source code easily.

Once you've figured out the build process, see Comment 0 & 1 for what to do.

Thanks for wanting to work on this bug ! Feel free to needinfo me if you need help :)
Flags: needinfo?(ntim007)
Hi Tim,
I have successfully built the setup, and ran the build.
I changed the SVG file as required (added style and active id on g tag)
I changed the reference all css files and jar.mn for windows/osx/linux.
But changes are not being reflected on my local build.
the code in profiler.inc.css is

#profiler-start {
  list-style-image: url("chrome://browser/skin/devtools/profiler-stopwatch.svg");
}

#profiler-start[checked] {
  list-style-image: url("chrome://browser/skin/devtools/profiler-stopwatch.svg#active");
} 

Did I miss something?
Flags: needinfo?(ntim007)
(In reply to Trishul from comment #6)
> Hi Tim,
> I have successfully built the setup, and ran the build.
> I changed the SVG file as required (added style and active id on g tag)
> I changed the reference all css files and jar.mn for windows/osx/linux.
> But changes are not being reflected on my local build.
> the code in profiler.inc.css is
> 
> #profiler-start {
>   list-style-image:
> url("chrome://browser/skin/devtools/profiler-stopwatch.svg");
> }
> 
> #profiler-start[checked] {
>   list-style-image:
> url("chrome://browser/skin/devtools/profiler-stopwatch.svg#active");
> } 
> 
> Did I miss something?

You need to create a new patch first (hg qnew stopwatch.patch -m "Bug 1036494 - Combine stopwatch icons into one file. r=bgrins"). Make sure you set up hg first (see codefirefox.com for how to do that).
Flags: needinfo?(ntim007)
Made the required changes and created patch

- added style and id in profiler-stopwatch.svg
- updated the file references (added #active)
- removed the profiler-stopwatch-checked.svg file
Attachment #8455021 - Flags: review?(ntim007)
Comment on attachment 8455021 [details] [diff] [review]
Combining profile-stopwatch image

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

This is going in the right direction :)
Here are some changes you still need to make that I forgot to mention :
- In jar.mn files remove the stopwatch-checked line (dont replace it, just remove it)
- I think there are some other references that you missed (just do an mxr search and you'll find them right away)
- There is an extra change at the end of the patch, please remove it.
Attachment #8455021 - Flags: review?(ntim007)
Thanks Tim :)

- I just removed the "profiler-stopwatch-checked" lines from the jar.mn (now do I need to add another line for [url]#active ? )
- I can find only 5 files with "profiler-stopwatch-checked.svg" on mxr (3 jar.mn,canvasdebugger.inc.css, profiler.inc.css)
- How to remove extra things at the end of patch(just delete the following?)

diff --git a/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf b/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf
deleted file mode 120000
--- a/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf
+++ /dev/null
@@ -1,1 +0,0 @@
-../../../../tools/perf
\ No newline at end of file
Flags: needinfo?(ntim007)
(In reply to Trishul from comment #10)
> Thanks Tim :)
> 
> - I just removed the "profiler-stopwatch-checked" lines from the jar.mn (now
> do I need to add another line for [url]#active ? )
Nope, new lines in jar.mn are only for new files. Here, we are deleting the checked file, and the checked state isn't in a new file now.

> - I can find only 5 files with "profiler-stopwatch-checked.svg" on mxr (3
> jar.mn,canvasdebugger.inc.css, profiler.inc.css)
Yep the canvasdebugger file. I don't think the profiler file is needed since we're gonna replace the profiler soon.

> - How to remove extra things at the end of patch(just delete the following?)
> 
> diff --git a/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf
> b/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf
> deleted file mode 120000
> --- a/media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf
> +++ /dev/null
> @@ -1,1 +0,0 @@
> -../../../../tools/perf
> \ No newline at end of file
Well, just restore the previous version of the file. (see mxr if you dont have it). Popping the patch then removing that part could also work, but I dont recommend doing so.
Flags: needinfo?(ntim007)
- removed the "profiler-stopwatch-checked" lines
- removed extra things at the end of patch
Attachment #8455021 - Attachment is obsolete: true
Attachment #8455374 - Flags: review?(ntim007)
Comment on attachment 8455374 [details] [diff] [review]
Combining profile-stopwatch image

Looks good in terms of code, can't test the patch as I don't have my laptop. Gonna ask Brian for review since I'm not a peer.
Attachment #8455374 - Flags: review?(ntim007)
Attachment #8455374 - Flags: review?(bgrinstead)
Attachment #8455374 - Flags: review+
Comment on attachment 8455374 [details] [diff] [review]
Combining profile-stopwatch image

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

I haven't been able to look into why this is, but when I load chrome://browser/skin/devtools/profiler-stopwatch.svg#active in the browser (or view the image while running a profile session) the stroke color is still white (#edf0f1) while it should be blue (#3BACE5).  Are you seeing this too?  Seems like it should be working based on the code, so I wonder if there is a different bug causing this.
Attachment #8455374 - Flags: review?(bgrinstead)
Attachment #8455374 - Flags: review+
Attachment #8455374 - Flags: feedback+
Screenshot of the profiler UI that I see in fx-team with the patch applied
I think it's because the fill attribute set on the g element is overriding the style. You might want to set th  fill in the style element as well.
(In reply to Brian Grinstead [:bgrins] from comment #15)
> Created attachment 8455844 [details]
> with-patch-applied-on-right.png
> 
> Screenshot of the profiler UI that I see in fx-team with the patch applied

The patch doesnt affect the profiler since it's getting replaced.
 > The patch doesnt affect the profiler since it's getting replaced.

It does affect the profiler.

(In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #16)
> I think it's because the fill attribute set on the g element is overriding
> the style. You might want to set th  fill in the style element as well.

I'm not sure, when loading chrome://browser/skin/devtools/profiler-stopwatch.svg#active in the browser (current fx-team) it shows up as white, but if I go to url bar and press enter it turns blue again.  When opening the file (from filesystem in Nightly) it starts out blue.
(In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #16)
> I think it's because the fill attribute set on the g element is overriding
> the style. You might want to set th  fill in the style element as well.

I tried the same svg image with an HTML file, and the results are as expected.
The icon turns blue when [url]#active is used. So I assume style is not overridden.
(In reply to Trishul from comment #19)
> (In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #16)
> > I think it's because the fill attribute set on the g element is overriding
> > the style. You might want to set th  fill in the style element as well.
> 
> I tried the same svg image with an HTML file, and the results are as
> expected.
> The icon turns blue when [url]#active is used. So I assume style is not
> overridden.

Yes, there seems to be some sort of bug with this inside of the toolbox.  Luckily, I just had another idea on how we can use this patch but without the #active target.  We don't actually *need* to use the active color since it has a blue background when running (we just switched to pause button in the debugger to act this way to be more consistent with the rest of the buttons).

So what I would do is:

1) Remove the :active coloring from the svg file
2) Remove references to chrome://browser/skin/devtools/profiler-stopwatch.svg#active
3) Remove the profiling buttons from the list of elements that have a filter: none in the light theme: http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#836.

Once this is done, it will actually be more consistent with the rest of the buttons.
Let's wait for Bug 879008 to land before landing this (it is basically rewriting profiler.inc.css, and we won't want to cause any conflicts).  You can proceed as listed in Comment 20, but we will need to do a final rebase after 879008 lands.
Depends on: 879008
(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Trishul from comment #19)
> > (In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #16)
> > > I think it's because the fill attribute set on the g element is overriding
> > > the style. You might want to set th  fill in the style element as well.
> > 
> > I tried the same svg image with an HTML file, and the results are as
> > expected.
> > The icon turns blue when [url]#active is used. So I assume style is not
> > overridden.
> 
> Yes, there seems to be some sort of bug with this inside of the toolbox. 
> Luckily, I just had another idea on how we can use this patch but without
> the #active target.  We don't actually *need* to use the active color since
> it has a blue background when running (we just switched to pause button in
> the debugger to act this way to be more consistent with the rest of the
> buttons).
> 
> So what I would do is:
> 
> 1) Remove the :active coloring from the svg file
> 2) Remove references to
> chrome://browser/skin/devtools/profiler-stopwatch.svg#active
> 3) Remove the profiling buttons from the list of elements that have a
> filter: none in the light theme:
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> toolbars.inc.css#836.
> 
> Once this is done, it will actually be more consistent with the rest of the
> buttons.

That would be inconsistent from the command buttons.
(In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #20)
> > (In reply to Trishul from comment #19)
> > > (In reply to Tim Nguyen [:ntim] (afk until end of July) from comment #16)
> > > > I think it's because the fill attribute set on the g element is overriding
> > > > the style. You might want to set th  fill in the style element as well.
> > > 
> > > I tried the same svg image with an HTML file, and the results are as
> > > expected.
> > > The icon turns blue when [url]#active is used. So I assume style is not
> > > overridden.
> > 
> > Yes, there seems to be some sort of bug with this inside of the toolbox. 
> > Luckily, I just had another idea on how we can use this patch but without
> > the #active target.  We don't actually *need* to use the active color since
> > it has a blue background when running (we just switched to pause button in
> > the debugger to act this way to be more consistent with the rest of the
> > buttons).
> > 
> > So what I would do is:
> > 
> > 1) Remove the :active coloring from the svg file
> > 2) Remove references to
> > chrome://browser/skin/devtools/profiler-stopwatch.svg#active
> > 3) Remove the profiling buttons from the list of elements that have a
> > filter: none in the light theme:
> > http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> > toolbars.inc.css#836.
> > 
> > Once this is done, it will actually be more consistent with the rest of the
> > buttons.
> 
> That would be inconsistent from the command buttons.

But it would be consistent with the other toolbar buttons - of which this is one.
There's another problem, black icons/text look ugly on the checked background in light theme. Also, the checked background is way too faint for a white icon.
As long as they all consistently look ugly, we can make updates to them all at once :)
No updates in many months, unassigning.
Assignee: trishul.goel → nobody
Status: ASSIGNED → NEW
Bug 1173397 gets rid of the blue image, there is only one remaining now.
No longer blocks: dt-theme-cleanup
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.