Closed Bug 897262 Opened 7 years ago Closed 7 years ago
Wasted work in ns
Base Screen::Check Minimum Brightness()
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0 (Beta/Release) Build ID: 20130316161634 Steps to reproduce: The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it. In method nsBaseScreen::CheckMinimumBrightness() in widget/xpwidgets/nsBaseScreen.cpp, the loop in line 70 keeps overriding "brightness" with "i" when "mBrightnessLocks[i]" is greater than zero. Therefore, only the last written value is visible out of the loop and all the other writes and iterations are not necessary. The patch iterates from the end of "i" and breaks the first time when "brightness" is set.
Hey, thanks for the patch! To request review, you need to put someone else's name in the "requestee" field. See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed for more detail.
Component: Untriaged → Widget
Product: Firefox → Core
Comment on attachment 784380 [details] [diff] [review] 897262.patch I don't see any functional change. This is a micro-optimization right? Tentatively r+ on the answer being yes.
Attachment #784380 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #3) > Comment on attachment 784380 [details] [diff] [review] > 897262.patch > > I don't see any functional change. This is a micro-optimization right? > Tentatively r+ on the answer being yes. Thanks for your review. Yes, it is a micro-optimization.
Cool. Thanks for the patch! Let me know if you need help finding something else to work on. This should land to our integration branch in a few hours and be merged to the central branch in ~24 hours.
Assignee: nobody → pchang9
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
I can see a behaviour change. If no mBrightnessLocks[i] are greater than zero, this is an infinite loop, as i >= 0 is always true for an unsigned integer.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :Ms2ger from comment #8) > I can see a behaviour change. If no mBrightnessLocks[i] are greater than > zero, this is an infinite loop, as i >= 0 is always true for an unsigned > integer. Thanks for pointing out the problem! I have update the patch, please review it.
Looks good but the patch no longer applies to mozilla-central because your first patch has landed. Can you attach a 'rebased' version of the patch?
(In reply to Benoit Girard (:BenWa) from comment #11) > Looks good but the patch no longer applies to mozilla-central because your > first patch has landed. Can you attach a 'rebased' version of the patch? Sure, please review 897262-2.patch. Do I have to say more in patch description?
Landed on try server https://tbpl.mozilla.org/?tree=Try&rev=bb88895a4186
Comment on attachment 787789 [details] [diff] [review] 897262-2.patch A better description for this change would be 'Bug 897262 - Avoid infinite loop in nsBaseScreen::CheckMinimumBrightness(). r=bgirard.' but I think it's fine as-is since it's a small fix.
Attachment #787789 - Flags: review?(bgirard) → review+
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.