Closed Bug 1264365 Opened 8 years ago Closed 8 years ago

Un-remove nsBaseWidget::IsSmalPopup

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
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.
Whiteboard: [gfx-noted]
Attachment #8741396 - Flags: review?(dvander)
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 ..."
Attachment #8741396 - Flags: review?(dvander) → review+
(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.
backed out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=26294140&repo=mozilla-inbound
Flags: needinfo?(nical.bugzilla)
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
Flags: needinfo?(nical.bugzilla)
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.
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9723c6c6136
Disable hardware acceleration for small popup widgets. r=dvander
https://hg.mozilla.org/mozilla-central/rev/e9723c6c6136
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.