Closed
Bug 987311
Opened 11 years ago
Closed 11 years ago
Convert CompositorTypes.h to typed enums
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(8 files, 7 obsolete files)
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
And I mean, convert all of it, including the bitflags such as TextureFlags, now that we have have typed enum bitflags (bug 987290) and serialize them (bug 987305).
This discovered real bugs in ContentClient.cpp mishandling TextureFlags, see details below.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8395861 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8395863 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8395867 -
Flags: review?(jmuizelaar)
Updated•11 years ago
|
Attachment #8395861 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8395928 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8395930 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8395931 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•11 years ago
|
||
So here is the big bug in ContentClient.cpp, that part 3 fixes.
In ContentClientRemoteBuffer::BuildTextureClients (and the Deprecated version further down in this file), we have this: (see http://hg.mozilla.org/mozilla-central/file/bb4dd9872236/gfx/layers/client/ContentClient.cpp#l198 ) :
void
ContentClientRemoteBuffer::BuildTextureClients(SurfaceFormat aFormat,
const nsIntRect& aRect,
uint32_t aFlags)
{
// ... some code ...
mTextureInfo.mTextureFlags = aFlags & ~TEXTURE_DEALLOCATE_CLIENT;
// ... some code ...
if (aFlags & BUFFER_COMPONENT_ALPHA) {
// ... some code ...
mTextureInfo.mTextureFlags |= TEXTURE_COMPONENT_ALPHA;
}
// ... some code ...
}
Here we see that aFlags is first used as if it were TextureFlags (TEXTURE_DEALLOCATE_CLIENT is a TextureFlags enum value) and later as if it were RotatedContentBuffer flags (BUFFER_COMPONENT_ALPHA is defined in a nested enum in class RotatedContentBuffer).
So both can't be right.
My understanding of this code is that aFlags really is RotatedContentBuffer flags, and the bug is in the first line above, wrongly interpreting it as TextureFlags,
mTextureInfo.mTextureFlags = aFlags & ~TEXTURE_DEALLOCATE_CLIENT;
while the code further down in this function is actually correctly converting these flags into TextureFlags:
if (aFlags & BUFFER_COMPONENT_ALPHA) {
// ... some code ...
mTextureInfo.mTextureFlags |= TEXTURE_COMPONENT_ALPHA;
}
The part 3 patch here implements the RotatedContentBuffer --> TextureFlags conversion correctly once and for all, in a static TextureFlagsForRotatedContentBufferFlags function.
The aFlags & ~TEXTURE_DEALLOCATE_CLIENT is interpreted as just the product of earlier confusion and now that we are creating fresh new TextureFlags, we don't need to worry about that.
Assignee | ||
Comment 8•11 years ago
|
||
This is the ContentClient.cpp bug fix on its own, before we do the typed enum conversion.
Hg paleontology traced this to bug 893301, new textures for ContentClient.
Attachment #8397945 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 9•11 years ago
|
||
This should now be free of behavior changes.
Attachment #8395867 -
Attachment is obsolete: true
Attachment #8395867 -
Flags: review?(jmuizelaar)
Attachment #8397947 -
Flags: review?(jmuizelaar)
Updated•11 years ago
|
Attachment #8397945 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Assignee: nobody → bjacob
Whiteboard: [leave open]
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8395861 -
Attachment is obsolete: true
Attachment #8395863 -
Attachment is obsolete: true
Attachment #8395928 -
Attachment is obsolete: true
Attachment #8395930 -
Attachment is obsolete: true
Attachment #8395931 -
Attachment is obsolete: true
Attachment #8397947 -
Attachment is obsolete: true
Attachment #8395863 -
Flags: review?(jmuizelaar)
Attachment #8395928 -
Flags: review?(jmuizelaar)
Attachment #8395930 -
Flags: review?(jmuizelaar)
Attachment #8395931 -
Flags: review?(jmuizelaar)
Attachment #8397947 -
Flags: review?(jmuizelaar)
Attachment #8412570 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8412572 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8412573 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8412574 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8412575 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8412576 -
Flags: review?(nical.bugzilla)
Updated•11 years ago
|
Attachment #8412570 -
Flags: review?(nical.bugzilla) → review+
Updated•11 years ago
|
Attachment #8412572 -
Flags: review?(nical.bugzilla) → review+
Updated•11 years ago
|
Attachment #8412573 -
Flags: review?(nical.bugzilla) → review+
Updated•11 years ago
|
Attachment #8412574 -
Flags: review?(nical.bugzilla) → review+
Updated•11 years ago
|
Attachment #8412575 -
Attachment is patch: true
Updated•11 years ago
|
Attachment #8412575 -
Flags: review?(nical.bugzilla) → review+
Comment 18•11 years ago
|
||
Comment on attachment 8412576 [details] [diff] [review]
6/6 - Other enums remaining manual changes
Review of attachment 8412576 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/CompositorTypes.h
@@ +217,5 @@
> Front = 1,
> Back = 2,
> OnWhiteFront = 3,
> + OnWhiteBack = 4,
> + HighBound
Note for later: we should really move the sentinels out the enums. -WError makes them annoying because switch statements now *have* to to contains cases for them (or have an empty default case which completely voids the point of checking that all enum variants are handled).
Attachment #8412576 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 19•11 years ago
|
||
The problem is we often need the sentinels for various things, such as: validating that enum values are in range when we serialize/deserialize them (cf. ****EnumSerializer); constructing arrays with as many entries as there are enum values (cf. EnumeratedArray).
Having sentinels as enum values is currently the only way to have them automatically have the right value.
In the future, for C++17 there is a proposal to add full compile-time reflection of enums, which would solve that problem.
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfd9a457f34
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2542a5c865
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ba0a377450
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc0d016de7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d5a171564bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1b41adeaef
Whiteboard: [leave open]
Comment 21•11 years ago
|
||
I got errors with vs2013.2 rc.
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(481) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(484) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(488) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
C:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(489) : error C2677: 二进制“[”: 没有找到接受“mozilla::layers::MaskType”类型的全局运算符(或没有可接受的转换)
......
Comment 22•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1dfd9a457f34
https://hg.mozilla.org/mozilla-central/rev/6a2542a5c865
https://hg.mozilla.org/mozilla-central/rev/62ba0a377450
https://hg.mozilla.org/mozilla-central/rev/dcc0d016de7a
https://hg.mozilla.org/mozilla-central/rev/8d5a171564bd
https://hg.mozilla.org/mozilla-central/rev/3f1b41adeaef
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 23•11 years ago
|
||
Getting other reports like that in comment 21 in #developers, so this was backed out (along with bug 989144 and bug 989027, which depended on it).
http://hg.mozilla.org/mozilla-central/rev/0ecc4193e630
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•11 years ago
|
||
After recent pull, with no patches in queue on Win8.1 Update 1 w/ VS2012 I was crashing due to changeset: 64bd above: https://pastebin.mozilla.org/4972777
Swapped to Ubuntu and crashed trying to build Android/Fennec: https://pastebin.mozilla.org/4973038
Assignee | ||
Comment 25•11 years ago
|
||
By "crashing" you mean "failed to compile", right? I seem to see only build errors in these pastebins.
Thanks for the reports, looking into it, will try to reland asap (this bitrots super fast).
Can I build with MSVC 2012 on tryserver?
Regarding your Fennec build error, what compiler/SDK/NDK are you using?
Updated•11 years ago
|
Target Milestone: mozilla31 → ---
Comment 26•11 years ago
|
||
Sorry for the imprecise terminology ... yes, build was failing.
When building for Fennec, my mozconfig is: https://pastebin.mozilla.org/4980730
Assignee | ||
Comment 27•11 years ago
|
||
Not requiring review, as it's basically a trivially required consequence of the other changes here. Arrays indexed by typed enums should be EnumeratedArray's, so that the strong typing is consistent.
Still needinfo'ing Bas mostly in a "FYI" fashion. Also, I hope that I got it right style-wise.
Flags: needinfo?(bas)
Assignee | ||
Comment 29•11 years ago
|
||
So this fixes the build on MSVC 2012; the last reported issue was that DIAGNOSTIC_NONE error on Android, but this looks like a posisble mis-merge, as it built fine on Android slaves and there is no occurence left of DIAGNOSTIC_NONE after my patches. I'm going to try relanding now.
Assignee | ||
Comment 30•11 years ago
|
||
Assignee | ||
Comment 31•11 years ago
|
||
Relanded:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8022b696d583
http://hg.mozilla.org/integration/mozilla-inbound/rev/360040db4f34
http://hg.mozilla.org/integration/mozilla-inbound/rev/865619dc14ca
http://hg.mozilla.org/integration/mozilla-inbound/rev/2ff75b768645
http://hg.mozilla.org/integration/mozilla-inbound/rev/6a36a9e6fa35
http://hg.mozilla.org/integration/mozilla-inbound/rev/e261a64bebf3
http://hg.mozilla.org/integration/mozilla-inbound/rev/c29805c4ffa1 <-- MSVC 2012 fix
https://hg.mozilla.org/mozilla-central/rev/8022b696d583
https://hg.mozilla.org/mozilla-central/rev/360040db4f34
https://hg.mozilla.org/mozilla-central/rev/865619dc14ca
https://hg.mozilla.org/mozilla-central/rev/2ff75b768645
https://hg.mozilla.org/mozilla-central/rev/6a36a9e6fa35
https://hg.mozilla.org/mozilla-central/rev/e261a64bebf3
https://hg.mozilla.org/mozilla-central/rev/c29805c4ffa1
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•