875335 - Finish OMTC titlebar drawing

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

This bug deals with the remaining OMTC work from bug 676241.

Comment 1

[Markus Stange [:mstange]]

The new patch in bug 873944 apparently doesn't trigger a composition during drawRect when none has been scheduled from Gecko. So it won't fix the bug which I'd hoped it would fix, namely that hovering the titlebar buttons in OMTC drawintitlebar mode currently doesn't repaint them in their hovered state.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 1: call NotifyDirtyRegion during viewWillDraw

Comment 3

[Markus Stange [:mstange]]

Attachment: part 2: trigger recomposition from viewWillDraw even if no layers changed

This is quite the brute force approach and looks very hacky. Any better ideas?

The ClientLayerManager.cpp hunk makes us call PrepareWindowEffects in the empty transaction case, too, not only in the normal transaction case. I think we want this anyway.

The nsViewManager.cpp hunk makes us call BeginTransaction+EndEmptyTransaction from viewWillDraw even if no views have ForcedRepaint().

The rest causes ShadowLayerForwarder to trigger a recomposition during viewWillDraw even though the transaction mTxn is empty.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 759732 [details] [diff] [review]
part 1: call NotifyDirtyRegion during viewWillDraw

I think this patch would be good to take anyway, regardless of the validity of the next patch.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 759734 [details] [diff] [review]
part 2: trigger recomposition from viewWillDraw even if no layers changed

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

::: layout/base/nsPresShell.cpp
@@ +5520,5 @@
>      // that will cause us to forget to update the real layer manager!
>      if (!(aFlags & PAINT_LAYERS)) {
> +       if (layerManager->HasShadowManager() && Compositor::GetBackend() != LAYERS_BASIC) {
> +         return;
> +       }

I assume this indention change wasn't intentional?

::: view/src/nsViewManager.cpp
@@ +634,5 @@
>        }
> +    } else {
> +      LayerManager *manager = aWidget->GetLayerManager();
> +      if (manager->HasShadowManager() &&
> +          Compositor::GetBackend() != LAYERS_BASIC) {

How about just taking the normal branch if this check is true?

Either move it up, or put it inside ForcedRepaint().

If we have OMTC, then getting a paint event means we always want to force a repaint. If we ever get OMTC everywhere, then we could drop the forced repaint concept entirely.

I'd feel better about that rather than having explicit Begin/EndTransaction calls inside view code.

Comment 6

[Markus Stange [:mstange]]

Attachment: part 1

Comment 7

[Markus Stange [:mstange]]

Attachment: part 2

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 759734 [details] [diff] [review]
> part 2: trigger recomposition from viewWillDraw even if no layers changed
> 
> Review of attachment 759734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresShell.cpp
> @@ +5520,5 @@
> >      // that will cause us to forget to update the real layer manager!
> >      if (!(aFlags & PAINT_LAYERS)) {
> > +       if (layerManager->HasShadowManager() && Compositor::GetBackend() != LAYERS_BASIC) {
> > +         return;
> > +       }
> 
> I assume this indention change wasn't intentional?

Huh, not sure how this happened.

> ::: view/src/nsViewManager.cpp
> @@ +634,5 @@
> >        }
> > +    } else {
> > +      LayerManager *manager = aWidget->GetLayerManager();
> > +      if (manager->HasShadowManager() &&
> > +          Compositor::GetBackend() != LAYERS_BASIC) {
> 
> How about just taking the normal branch if this check is true?
> 
> Either move it up, or put it inside ForcedRepaint().

I've moved it up.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 761925 [details] [diff] [review]
part 2

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

::: view/src/nsViewManager.cpp
@@ +632,5 @@
> +    LayerManager *manager = aWidget->GetLayerManager();
> +    if (view &&
> +        (view->ForcedRepaint() ||
> +         (manager->HasShadowManager() &&
> +          Compositor::GetBackend() != LAYERS_BASIC))) {

I'm assuming this will land after bug 873944?

In which case these two lines can just become !manager->NeedsWidgetInvalidation()

Comment 9

[Markus Stange [:mstange]]

Attachment: part2

Yes, I'll wait for bug 873944 before landing this.

Comment 10

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/381f39912071
https://hg.mozilla.org/integration/mozilla-inbound/rev/c21f4e5561bb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/381f39912071
https://hg.mozilla.org/mozilla-central/rev/c21f4e5561bb