826817 - Make all platforms always fire WillPaintWindow/DidPaintWindow

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will simplify some code.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Send WillPaintWindow/DidPaintWindow from all widget implementations

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove will-send-did-paint and did-send-will-paint flags from nsIWidgetListener::PaintWindow

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Remove will-send-did-paint flag from nsIWidgetListener::WillPaintWindow

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Remove will-send-did-paint and did-send-will-paint flags from nsIPresShell APIs

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 698131 [details] [diff] [review]
Part 1: Send WillPaintWindow/DidPaintWindow from all widget implementations

>diff --git a/widget/xpwidgets/PuppetWidget.cpp b/widget/xpwidgets/PuppetWidget.cpp
>     if (mozilla::layers::LAYERS_D3D10 == mLayerManager->GetBackendType()) {
>-      mAttachedWidgetListener->PaintWindow(this, region, nsIWidgetListener::WILL_SEND_DID_PAINT);
>+      mAttachedWidgetListener->PaintWindow(this, region,
>+        nsIWidgetListener::SENT_WILL_PAINT | nsIWidgetListener::WILL_SEND_DID_PAINT);
>     } else {
>       nsRefPtr<gfxContext> ctx = new gfxContext(mSurface);
>       ctx->Rectangle(gfxRect(0,0,0,0));
>       ctx->Clip();
>       AutoLayerManagerSetup setupLayerManager(this, ctx,
>                                               BUFFER_NONE);
>-      mAttachedWidgetListener->PaintWindow(this, region, nsIWidgetListener::WILL_SEND_DID_PAINT);
>+      mAttachedWidgetListener->PaintWindow(this, region,
>+        nsIWidgetListener::SENT_WILL_PAINT | nsIWidgetListener::WILL_SEND_DID_PAINT);
>       mTabChild->NotifyPainted();
>     }
>   }
> 
>-  if (mAttachedWidgetListener) {
>-    mAttachedWidgetListener->DidPaintWindow();
>-  }
>+  mAttachedWidgetListener->DidPaintWindow();

I think you should keep the null check on mAttachedWidgetListener before DidPaintWindow and also have null checks before PaintWindow.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 698131 [details] [diff] [review]
Part 1: Send WillPaintWindow/DidPaintWindow from all widget implementations

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>-bool nsChildView::PaintWindow(nsIntRegion aRegion, bool aIsAlternate)
>-{
>-  nsIWidget* widget = this;
>+nsIWidgetListener* nsChildView::GetPaintListener() {
>   nsIWidgetListener* listener = mWidgetListener;
> 
>   // If there is no listener, use the parent popup's listener if that exists.
>   if (!listener && mParentWidget) {
>     nsWindowType type;
>     mParentWidget->GetWindowType(type);
>     if (type == eWindowType_popup) {
>       widget = mParentWidget;
>       listener = mParentWidget->GetWidgetListener();
>     }
>   }
> 
>+  return listener;
>+}
>+
>+bool nsChildView::PaintWindow(nsIntRegion aRegion, bool aIsAlternate)
>+{
>+  nsIWidget* widget = this;
>+  nsIWidgetListener* listener = GetPaintListener();
>   if (!listener)
>     return false;
> 
>   bool returnValue = false;
>   bool oldDispatchPaint = mIsDispatchPaint;
>   mIsDispatchPaint = true;
>-  uint32_t flags = nsIWidgetListener::SENT_WILL_PAINT;
>+  uint32_t flags =
>+    nsIWidgetListener::SENT_WILL_PAINT | nsIWidgetListener::WILL_SEND_DID_PAINT;
>   if (aIsAlternate) {
>     flags |= nsIWidgetListener::PAINT_IS_ALTERNATE; 
>   }
>   returnValue = listener->PaintWindow(widget, aRegion, flags);
>+
>+  listener = GetPaintListener();
>+  if (listener) {
>+    listener->DidPaintWindow();
>+  }
>+
>   mIsDispatchPaint = oldDispatchPaint;
>   return returnValue;
> }

The old code would change widget from this to mParentWidget when it changed the listener to the parent listener to, so I think this might be wrong actually.

Comment 7

[Timothy Nikkel (:tnikkel)]

I rebased these and applied the review comments. Were there any significant changes to the patches in your queue that I might be missing?

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: new Part 1

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: interdiff of old part 1 to new part 1

This is the only significant change.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #7)
> I rebased these and applied the review comments. Were there any significant
> changes to the patches in your queue that I might be missing?

No, I haven't done anything with them.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 706923 [details] [diff] [review]
new Part 1

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

Feel free to land these patches

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Feel free to land these patches

That was my plan.

Comment 13

[Timothy Nikkel (:tnikkel)]

https://tbpl.mozilla.org/?tree=Try&rev=acb356168cc7

Comment 14

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/954ef23857ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd2536c95b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/198516229e54
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b4392eb8a1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/954ef23857ba
https://hg.mozilla.org/mozilla-central/rev/dbd2536c95b3
https://hg.mozilla.org/mozilla-central/rev/198516229e54
https://hg.mozilla.org/mozilla-central/rev/f4b4392eb8a1