The default bug view has changed. See this FAQ.

Use bit twiddling for gfxUtils::NextPowerOfTwo on non-arm builds

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

Trunk
mozilla14
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 607355 [details] [diff] [review]
patch
Attachment #607355 - Flags: review?(gwright)
Comment on attachment 607355 [details] [diff] [review]
patch

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

Looks fine otherwise.

::: gfx/thebes/gfxUtils.h
@@ +210,1 @@
>      return 1 << (32 - CountLeadingZeroes(aNumber - 1));

Perhaps it's best to just call __builtin_clz directly here instead of having two separate ifdefs.
Attachment #607355 - Flags: review?(gwright) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 607671 [details] [diff] [review]
updated patch

Inlined the clz call.

I've asked for re-review because I've removed the Android copyright notice (doesn't seem to cover anything now) and wanted to check that was OK.
Attachment #607355 - Attachment is obsolete: true
Attachment #607671 - Flags: review?(gwright)
Comment on attachment 607671 [details] [diff] [review]
updated patch

lgtm
Attachment #607671 - Flags: review?(gwright) → review+
(In reply to Nick Cameron from comment #3)
> Created attachment 607671 [details] [diff] [review]
> updated patch
> 
> Inlined the clz call.
> 
> I've asked for re-review because I've removed the Android copyright notice
> (doesn't seem to cover anything now) and wanted to check that was OK.

Oops only just noticed this. The copyright should still apply to the ARM optimised call that uses __builtin_clz, as well as the IsPowerOfTwo code. However both are very generic so the copyright notice may be moot.

Also - just noticed that your indentation is set to 4 spaces for the ARM case and 2 spaces otherwise. I think it should be 4 for all.
(Assignee)

Comment 6

5 years ago
Created attachment 607836 [details] [diff] [review]
updated patch

Fixed indent, undeleted copyright notice.
Attachment #607671 - Attachment is obsolete: true
Attachment #607836 - Flags: review+
(Assignee)

Comment 7

5 years ago
Successfully navigated push to Try: https://tbpl.mozilla.org/?tree=Try&rev=43826350ded4
(Assignee)

Updated

5 years ago
Whiteboard: checkin-needed
Keywords: checkin-needed
Whiteboard: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c650e8748

George, you are not currently listed as a Graphics peer on the wiki. Please update that.
https://wiki.mozilla.org/Modules/Core

Also, is there anything that can be tested here? Or do existing tests already cover it?
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla14
(In reply to Ryan VanderMeulen from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c650e8748
> 
> George, you are not currently listed as a Graphics peer on the wiki. Please
> update that.
> https://wiki.mozilla.org/Modules/Core

Yeah, that doesn't seem real up-to-date. For some reason Canvas 2D/WebGL was down in layout, so I pulled it up to graphics.
(In reply to Ryan VanderMeulen from comment #8)
> George, you are not currently listed as a Graphics peer on the wiki. Please
> update that.
> https://wiki.mozilla.org/Modules/Core

That'd be because I'm not one :)
errr...so why are patches being checked in on your r+?
(Assignee)

Comment 12

5 years ago
Ah, sorry, my bad, still fairly new to this, didn't realise I needed a review from a graphics peer to check in.
Target Milestone: mozilla14 → ---
Patches *always* need review from a module peer before checking in. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/939ec96b6b38
(Assignee)

Updated

5 years ago
Attachment #607836 - Flags: review+ → review?(joe)
We haven't been enforcing this at all for the graphics module at all. The current list of peers in that document is considerably out of date - Though it may be technically correct since new peers haven't been discussed at all to my knowledge.

I think this can probably be relanded, considering that the majority of recent gfx work would fall under the same classification.
Comment on attachment 607836 [details] [diff] [review]
updated patch

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

Arguably the graphics peers list is outdated. Meh. I've been asked to review lots of things a little outside gfx as well *shrugs*. I'm listed as a GFx peer though! Look at that!
Attachment #607836 - Flags: review?(joe) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> We haven't been enforcing this at all for the graphics module at all. The
> current list of peers in that document is considerably out of date - Though
> it may be technically correct since new peers haven't been discussed at all
> to my knowledge.
> 
> I think this can probably be relanded, considering that the majority of
> recent gfx work would fall under the same classification.

Yeah, the majority of recent WebGL work is me and bjacob reviewing eachothers' patches. Should we just add the lot of us as peers, so as to make the documentation match the reality?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
This 'peer' thing just gets in the way and I had forgotten it existed before this reminded me about it. Please just reland?
I agree with bjacob. It seems that the gfx module doesn't follow the normal review rules, for one reason or another..
In fact, the reality is that different people are better reviewers for different files. Even if someone is relatively new, they might still be the best reviewer for a part of the codebase where they have been the most active since they started.
I'll reland this tomorrow if nobody else beats me to it.
https://hg.mozilla.org/mozilla-central/rev/e54c650e8748
https://hg.mozilla.org/mozilla-central/rev/939ec96b6b38
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64fb9a403e6

For great justice.
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/f64fb9a403e6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.