Support plugin in layers-free mode

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Currently we do the plugin painting in layer building state. So we need to do some changes for layers-free mode.
Component: Audio/Video: Recording → Graphics: WebRender
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Currently I only handle nsDisplayPlugin. I'm not sure which test will use nsDisplayPluginReadback. With the patch, 'layout/reftests/bugs/632423-1.html' could pass in layers-free mode.
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8897753 [details]
Bug 1390440 - Support nsDisplayPlugin for webrender layers-free mode.

https://reviewboard.mozilla.org/r/169058/#review180184
Attachment #8897753 - Flags: review?(matt.woodrow) → review+

Comment 5

a year ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9f65581eed4
Support nsDisplayPlugin for webrender layers-free mode. r=mattwoodrow

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9f65581eed4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
According to the coverage report, the code you added is not covered by automated tests: https://codecov.io/gh/marco-c/gecko-dev/src/master/layout/generic/nsPluginFrame.cpp#L1411.

Is it expected? Would it be possible to write tests covering the code?
Flags: needinfo?(ethlin)
(Assignee)

Comment 8

a year ago
We already have automated tests but the preference is off by default. I think it will be covered after bug 1389000 is landed.
Flags: needinfo?(ethlin)
We can turn on some tests that cover this code via the pref(gfx.webrender.layers-free,true) skip-if(!webrender) annotation (e.g. in bug 1392523) as a stopgap until bug 1389000 lands.
(Assignee)

Comment 10

a year ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> We can turn on some tests that cover this code via the
> pref(gfx.webrender.layers-free,true) skip-if(!webrender) annotation (e.g. in
> bug 1392523) as a stopgap until bug 1389000 lands.

Okay, I'll add 'layout/reftests/bugs/632423-1.html' (plugin test) in bug 1392523 as well.
Should CreateWebRenderCommands be covered now? It looks like it still isn't.
Yeah it should be covered by the test at http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/reftests/bugs/reftest.list#1654

Note that the test only runs on the linux64-qr platform ("Linux x64 QuantumRender" on TreeHerder), which might not be included in the code coverage analysis. That might explain why it's not showing up as covered.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> Yeah it should be covered by the test at
> http://searchfox.org/mozilla-central/rev/
> 6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/reftests/bugs/reftest.
> list#1654
> 
> Note that the test only runs on the linux64-qr platform ("Linux x64
> QuantumRender" on TreeHerder), which might not be included in the code
> coverage analysis. That might explain why it's not showing up as covered.

Yes, that's why. I thought we were running those tests on the normal linux64 platform (and thus linux64-ccov).
(Assignee)

Comment 14

a year ago
(In reply to Marco Castelluccio [:marco] from comment #13)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> > Yeah it should be covered by the test at
> > http://searchfox.org/mozilla-central/rev/
> > 6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/reftests/bugs/reftest.
> > list#1654
> > 
> > Note that the test only runs on the linux64-qr platform ("Linux x64
> > QuantumRender" on TreeHerder), which might not be included in the code
> > coverage analysis. That might explain why it's not showing up as covered.
> 
> Yes, that's why. I thought we were running those tests on the normal linux64
> platform (and thus linux64-ccov).

Right, should be the platform problem. I just checked on my local device and I think it's covered now.
You need to log in before you can comment on or make changes to this bug.