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

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: wrahman0, Mentored)

Tracking

unspecified
mozilla40
x86
macOS
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`.
I will take a look at this.
How can I go about testing my solution?
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)
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)
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)
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+
Yeah Ill submit a patch for both later tonight.
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)
Sure, I'm not currently at near by laptop so expect the change later tonight.
Flags: needinfo?(wrahman0)
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+
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.
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
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.
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!
Glad to help :D
(vouched)
Flags: needinfo?(dteller)
https://hg.mozilla.org/mozilla-central/rev/58c91976be46
Status: NEW → RESOLVED
Closed: 4 years ago
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.