897262 - Wasted work in nsBaseScreen::CheckMinimumBrightness()

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[pchang9]

Attachment: patch.diff

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 2

[pchang9]

Attachment: 897262.patch

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[pchang9]

(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.

Comment 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50dfd0c5bc36

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/50dfd0c5bc36

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 9

[pchang9]

Attachment: 897262.patch

Comment 10

[pchang9]

(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.

Comment 11

[Benoit Girard (:BenWa)]

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?

Comment 12

[pchang9]

Attachment: 897262-2.patch

Comment 13

[pchang9]

(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?

Comment 14

[pchang9]

Landed on try server
https://tbpl.mozilla.org/?tree=Try&rev=bb88895a4186

Comment 15

[Benoit Girard (:BenWa)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08bd150857fb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08bd150857fb