In about:performance, jank level 10 is displayed as black instead of red

RESOLVED FIXED in Firefox 40

Status

()

Toolkit
Performance Monitoring
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Yoric, Assigned: wrahman0, Mentored)

Tracking

unspecified
mozilla40
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

This is due to a typo in aboutPerformance.xhtml, in which we define two styles for `jank9`, instead of one for `jank9` and one for `jank10`.

The problem appears in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutperformance/content/aboutPerformance.xhtml#71

The class `jank9` at line 71 should be replaced with `jank10`.
(Assignee)

Comment 1

3 years ago
I will take a look at this.
(Assignee)

Comment 2

3 years ago
How can I go about testing my solution?
(Assignee)

Comment 3

3 years ago
Repeated message. Forgot to mark need more info.
Flags: needinfo?(dteller)
The first step is to build Firefox. Have you done that already?

Once you have built Firefox, the simplest way is probably to:
1. Open about:performance;
2. Open the web console;
3. In the webconsole, write the following:
  for (var start = Date.now(); Date.now() - start < 2000; Date.now());
4. Look quickly :)

This will cause the webconsole to use way too much CPU, which should result in a jank level of 10.
Flags: needinfo?(dteller)
(Assignee)

Comment 5

3 years ago
Yeah, I have already built Firefox. I just didn't know how to see this color. It makes sense and seems to be a very simple fix. I will get it up by the end of the day. 

Could you assign me this bug so it appears under "My bugs"?
Flags: needinfo?(dteller)
The general policy is to wait until the first patch to assign a bug.
Flags: needinfo?(dteller)
(Assignee)

Comment 7

3 years ago
So I was running the about:performance page and it seems like none of the jank * are colored. Any idea?
Flags: needinfo?(dteller)
Best guess: there may be a syntax error in your CSS. If you wish, you can attach your patch so that I can take a look.
Flags: needinfo?(dteller)
(Assignee)

Comment 9

3 years ago
Created attachment 8588483 [details] [diff] [review]
bug1150514_jank9_tojank10.diff

jank10 is appearing red now. It would be more prominent if all of the jank* were bold. Do you think I should bold all the jank numbers as well?
Flags: needinfo?(dteller)
Attachment #8588483 - Flags: review?(dteller)
Assignee: nobody → wrahman0
Flags: needinfo?(dteller)
Comment on attachment 8588483 [details] [diff] [review]
bug1150514_jank9_tojank10.diff

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

Looks good to me.
Making jank level bold is a good idea. Do you want to submit a patch that does both?
Attachment #8588483 - Flags: review?(dteller) → review+
(Assignee)

Comment 11

3 years ago
Yeah Ill submit a patch for both later tonight.
(Assignee)

Comment 12

3 years ago
Created attachment 8590589 [details] [diff] [review]
bug1150514_jank9_tojank10.diff

Added bolded jank text.
Attachment #8588483 - Attachment is obsolete: true
Attachment #8590589 - Flags: review?(dteller)
Comment on attachment 8590589 [details] [diff] [review]
bug1150514_jank9_tojank10.diff

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

This looks good.
Could you change your commit message as follows, before we can land the patch?

Bug 1150514 - in about:performance, fixing CSS of jank levels;r=Yoric
Attachment #8590589 - Flags: review?(dteller) → review+
Flags: needinfo?(wrahman0)
(Assignee)

Comment 14

3 years ago
Sure, I'm not currently at near by laptop so expect the change later tonight.
Flags: needinfo?(wrahman0)
(Assignee)

Comment 15

3 years ago
Created attachment 8591181 [details] [diff] [review]
bug1150514 - in about:performance, fixing CSS of jank levels;r=Yoric

How does that look? Let me know if you want anything else changed.
Attachment #8590589 - Attachment is obsolete: true
Attachment #8591181 - Flags: review?(dteller)
Comment on attachment 8591181 [details] [diff] [review]
bug1150514 - in about:performance, fixing CSS of jank levels;r=Yoric

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

I'm afraid that your patch still doesn't have a commit message.

If you are using mq, you can set the commit message by doing
 hg qref -m "Bug 1150514 - in about:performance, fixing CSS of jank levels;r=Yoric"
if you are using heads, you can set the commit message by doing
 hg commit --amend -m "Bug 1150514 - in about:performance, fixing CSS of jank levels;r=Yoric"
Attachment #8591181 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 17

3 years ago
Oh, I see. What I did was set the message when I was submitting the patch on bugzilla. I am using mercurial, so I will do what you mentioned above. Sorry about the that.
(Assignee)

Comment 18

3 years ago
Created attachment 8592040 [details] [diff] [review]
bug1150514_fixing_css_for_jank_levels.diff

Updated the patch message to be "Bug 1150514 - in about:performance, fixing CSS of jank levels;r=Yoric" as per request.
Attachment #8591181 - Attachment is obsolete: true
Attachment #8592040 - Flags: review?(dteller)
Comment on attachment 8592040 [details] [diff] [review]
bug1150514_fixing_css_for_jank_levels.diff

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

Yep, looks good.
Attachment #8592040 - Flags: review?(dteller) → review+
Let's land this.
Keywords: checkin-needed
(Assignee)

Comment 21

3 years ago
Is there anything else required from my end before this lands?
Flags: needinfo?(dteller)
Nope, this is going to land within a few hours.
Flags: needinfo?(dteller)
Next step is picking another bug :)

By the way, if you haven't opened an account on mozillians.org, you can do it now. I'll vouch for you.
(Assignee)

Comment 24

3 years ago
Yeah, I was thinking about focusing on the about:performance side of things. I will be sending a patch later today about a temporary fix for the refresh rate of that page. It should be increased from 2000 ms to probably 10000 ms. This fix should do until we land Bug 1150863. 

Also, thanks for the vouch offer! Really motivates me to keep working hard here! Here is my profile info: https://mozillians.org/en-US/u/wrahman0/
Flags: needinfo?(dteller)
https://hg.mozilla.org/integration/fx-team/rev/58c91976be46
Keywords: checkin-needed
Whiteboard: [lang=css][good first bug] → [lang=css][good first bug][fixed-in-fx-team]
(In reply to Carsten Book [:Tomcat] from comment #25)
> https://hg.mozilla.org/integration/fx-team/rev/58c91976be46

also thanks for contributing to mozilla!
(Assignee)

Comment 27

3 years ago
Glad to help :D
(vouched)
Flags: needinfo?(dteller)

Comment 29

3 years ago
https://hg.mozilla.org/mozilla-central/rev/58c91976be46
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=css][good first bug][fixed-in-fx-team] → [lang=css][good first bug]
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.