1264365 - Un-remove nsBaseWidget::IsSmalPopup

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

In bug 943084 we removed IsSmallPopup but the reason isn't quite clear to me. The bug says that in the context of OMTC, forcing the basic backend for popup widgets doesn't make sense. I am going to assume that it's because back then we didn't support having several compositor backends at the same time, but now we do. Or maybe was it simplify things (everything is either accelerated or not accelerated).

But having lots of gl contexts on the compositor thread (we have one per accelerated widget) means switching between them, which is costly, and switching between different contexts is typically the kind of place where eventual driver bugs get the sneakiest.

So I'll resurrect the IsSmallPopup check and we'll see how it goes.

Comment 1

[Nicolas Silva [:nical]]

Actually, after a closer look at the patch in bug 943084, it was used to choose whether to use OMTC for that widget and not only to choose the compsoitor backend. So what I am doing here is not entirely contradicting what happened in that bug which is reassuring.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Use the basic compositor for popup widgets

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8741396 [details] [diff] [review]
Use the basic compositor for popup widgets

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

r=me with that fixed

::: widget/nsBaseWidget.cpp
@@ +922,5 @@
>  }
>  
> +bool nsBaseWidget::IsSmallPopup() const
> +{
> +  return mWindowType != eWindowType_popup && mPopupType != ePopupTypePanel;

I think this should be "mWindowType == eWindowType_popup ..."

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8372d58243b

Comment 5

[Nicolas Silva [:nical]]

(In reply to David Anderson [:dvander] from comment #3)
> I think this should be "mWindowType == eWindowType_popup ..."

Yeah, silly me. Fixed in the patch that landed.

Comment 6

[Carsten Book [:Tomcat]]

backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=26294140&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c10754af667

Comment 8

[Carsten Book [:Tomcat]]

seems https://treeherder.mozilla.org/logviewer.html#?job_id=26296821&repo=mozilla-inbound is also related

Comment 9

[Nicolas Silva [:nical]]

I made another silly mistake on the patch I tried to land. I pushed a fixed version to try, let's see.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77bf32454ef47fee0836a7924466e74c21d18644

Comment 10

[Nicolas Silva [:nical]]

New try push with fixed patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=04d3d2de0bcc29c0a011aa220bdfc2a3aa19ddbb

I also made it linux-only because it does not buy us anything on mac since we still use a gl context per (basic) widget to get stuff to the screen.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9723c6c6136
Disable hardware acceleration for small popup widgets. r=dvander

Comment 12

[Carsten Book [:Tomcat]]

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