743612 - Gonk fallback rendering path is broken

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The logic in nsWindow::nsWindow() is no longer correct.

Comment 1

[Cody Brocious [:Daeken]]

Attachment: Implement proper fallback

Implemented OMTC fallback path by moving context construction out of NSWindow's constructor.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 615853 [details] [diff] [review]
Implement proper fallback

>diff --git a/widget/gonk/nsWindow.cpp b/widget/gonk/nsWindow.cpp

>+static bool sConstructed;

This name is a little confusing: let's call it sScreenInitialized.

>+    sGLContext = GLContextProvider::CreateForWindow(this);

By this point, we've already set gScreenSize based on
Framebuffer::GetSize().  They /should/ be the same, but let's do the
following

    DebugOnly<nsIntRect> fbBounds = gScreenBounds;
    sGLContext = GLContextProvider::CreateForWindow(this);
    MOZ_ASSERT(fbBounds == gScreenBounds);

to be sure.

>+        } else
>             LOG("Could not create OGL LayerManager");

Nit: braces around this else clause.

>+    nsIntSize screenSize;
>+    sFramebufferOpen = Framebuffer::Open(&screenSize);

If we're always getting the size from GetSize(), we can remove the
param to this function.

Also, please make the change to GetSize() that we discussed on IRC.

r=me with those changes.

Comment 3

[Cody Brocious [:Daeken]]

Attachment: Fixed issues raised by cjones

I've implemented fixes for the issues mentioned in the last comment and what we spoke about on IRC.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 615973 [details] [diff] [review]
Fixed issues raised by cjones

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Comment on attachment 615853 [details] [diff] [review]
> Implement proper fallback
>
> >+    sGLContext = GLContextProvider::CreateForWindow(this);
> 
> By this point, we've already set gScreenSize based on
> Framebuffer::GetSize().  They /should/ be the same, but let's do the
> following
> 
>     DebugOnly<nsIntRect> fbBounds = gScreenBounds;
>     sGLContext = GLContextProvider::CreateForWindow(this);
>     MOZ_ASSERT(fbBounds == gScreenBounds);
> 
> to be sure.

Didn't address this.

>diff --git a/widget/gonk/Framebuffer.cpp b/widget/gonk/Framebuffer.cpp

>+static gfxIntSize *sScreenSize = nsnull;

Need to free this in Close().

> bool
> GetSize(nsIntSize *aScreenSize) {
>-    if (0 <= sFd)
>+    // If the framebuffer has been opened, we should always have the size.
>+    if (0 <= sFd || sScreenSize) {
>+        MOZ_ASSERT(sScreenSize);

No need to assert this: if it's null, we'll just crash anyway.

Would like to see the version with those fixed.

Comment 5

[Cody Brocious [:Daeken]]

Attachment: Fixed most recent issues raised by cjones

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Fix OMTC on gonk

patch bitrotted.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 636608 [details] [diff] [review]
Fix OMTC on gonk

r+ with free in Close().  Changed locally, will push to inbound.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/a22ca1e05bb8
then backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/074372a19576
then relanded https://hg.mozilla.org/integration/mozilla-inbound/rev/a047da349d59

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a047da349d59