When generating display lists for WebRender, we were not caching the
draw result via nsDisplayItemGenericImageGeometry::UpdateDrawResult (or
similar) after completing CreateWebRenderCommands. This is important
because reftests use this to force sync decoding for images; it may be a
reason for image-related intermittent failures on *-qr builds.
Additionally, we may have been requesting fallback in cases where fallback
could not do anything more than WebRender could. For example, if we can't
get an image container yet, there is no point in requesting fallback
because it might just be we haven't started decoding yet. We should just
return the actual draw result in such cases.
In addition to the image container, the draw result can also be useful
for callers to know whether or not the surface(s) in the container are
fully decoded or not. This is used in subsequent parts to avoid
flickering in some cases.
Fairly straightforward - went with extra indirection in the
FreeClipChains method to avoid all the extra variables and
code paths.
Differential Revision: https://phabricator.services.mozilla.com/D5342
--HG--
extra : moz-landing-system : lando
> dom/media/gmp/CDMStorageIdProvider.cpp(63,10): warning:
> local variable 'storageId' will be copied despite being returned by name [-Wreturn-std-move]
nsAutoCString -> nsCString, will add std::move().
> layout/painting/DisplayItemClip.cpp(581,10): warning:
> local variable 'str' will be copied despite being returned by name [-Wreturn-std-move]
nsAutoCString -> nsCString, will add std::move().
> layout/painting/DisplayItemClipChain.cpp(88,10): warning:
> local variable 'str' will be copied despite being returned by name [-Wreturn-std-move]
nsAutoCString -> nsCString, will add std::move().
> layout/painting/nsDisplayList.cpp(179,10): warning:
> local variable 'str' will be copied despite being returned by name [-Wreturn-std-move]
nsAutoCString -> nsCString, will add std::move().
> gfx/thebes/gfxWindowsPlatform.cpp(454,10): warning:
> moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
Will remove std::move().
> gfx/thebes/gfxFontEntry.cpp(245,20): warning:
> local variable 'name' will be copied despite being returned by name [-Wreturn-std-move]
nsAutoCString -> nsCString, will add std::move().
> netwerk/cookie/nsCookieService.cpp(4460,10): warning:
> local variable 'path' will be copied despite being returned by name [-Wreturn-std-move]
GetPathFromURI() result is stored in an nsAutoCString, so it might as well return that type.
> toolkit/components/extensions/WebExtensionPolicy.cpp(462,12): warning:
> local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]
> toolkit/components/extensions/WebExtensionPolicy.cpp(475,10): warning:
> local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]
`result` may be empty or may be arbitrarily long, so I'll use nsCString inside the function.
> toolkit/xre/CmdLineAndEnvUtils.h(349,10): warning:
> moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
Returning an UniquePtr, will remove std::move().
Also will `return s` instead of `return nullptr` when `(!s)`, to avoid extra construction which could also prevent elision (not entirely sure, but it's at least not worse!); and it's clearer that the two `return`s return the same already-constructed on-stack object.
> tools/profiler/core/shared-libraries-win32.cc(111,10): warning:
> local variable 'version' will be copied despite being returned by name [-Wreturn-std-move]
nsPrintfCString -> nsCString, will add std::move().
> xpcom/glue/FileUtils.cpp(179,10): warning:
> local variable 'fullName' will be copied despite being returned by name [-Wreturn-std-move]
> xpcom/glue/FileUtils.cpp(209,10): warning:
> local variable 'path' will be copied despite being returned by name [-Wreturn-std-move]
nsAuto{,C}String -> ns{,C}String, will add std::move().
This allowed removals of 'AllowCompilerWarnings' from layout/painting,
netwerk/cookie, and toolkit/components/extensions.
Differential Revision: https://phabricator.services.mozilla.com/D5425
--HG--
extra : moz-landing-system : lando
This is the only reason I haven't used it before for things like
StyleSheet::State.
Change the underlying type to be the underlying enum representation by default,
but allow to override it if wanted.
Assertions should catch misuses.
Differential Revision: https://phabricator.services.mozilla.com/D5248
--HG--
extra : moz-landing-system : lando
Introduce an ImageRendering argument for CreateImageKey which is then used at the CreateAsyncImageWebRenderCommands call to provide the proper filtering instead of using always Auto filtering. Update all calls to CreateImageKey to use the new interface.
Summary:
nsDisplayLists are currently a little bit error prone, since the
empty state of an nsDisplayList requires mTop == &mSentinel.
since &mSentinel will change when copied, while mTop won't,
this naturally creates an invalid state. Additionally, copies
don't quite make sense, since there is a requirement in the
destructor that destructed nsDisplayLists are empty - in which
case we would have to empty both the copied and the original
nsDisplayList - something which is unlikely to happen naturally.
Moves however are a natural operation - we just need to implement
the correct move behavior accounting for this mTop == &mSentinel
requirement.
Reviewers: mattwoodrow
Reviewed By: mattwoodrow
Bug #: 1489588
Differential Revision: https://phabricator.services.mozilla.com/D5293
--HG--
extra : rebase_source : e8889903c6cd03a8ec0ff90f189dbdcc79201eb3
extra : histedit_source : 9e0e18702e6e4b4feb5bea0548dc42250905f897
The frame for a background image should be associated with the image
request. This was done on the non-WebRender path in
nsCSSRendering::PaintStyleImageLayerWithSC but otherwise missing for the
WebRender path. This is important because the association is what allows
the FRAME_UPDATE from imagelib to cause a repaint. Without it, it will
manifest as incomplete or missing background images, in particular after
the background image SourceSurface has been released to save memory.
The image can be decoded but we haven't notified such (and hence the image state doesn't indicate such). We are able to draw the image at this point because painting has special access to the image that bypasses the image state (this happens via nsStyleImage::StartDecoding -> imgIRequest::StartDecodingWithResult).
In this state if we record a successful draw then any parts of the image that we newly drew but weren't invalidated for some other reason won't get updated. When the async decode notification gets processed it will invalidate the image and that will result in it getting updated. But if we get a paint that asks for sync decoding of images before this it will see that we sucessfully drew the image and not invalidate its area and the sync decode paint won't update the image.
So we still draw the image but we don't record a successful draw unless the image state is complete and hence the decode notification has been processed (image state and notifications are always in sync).
Note that we still have to draw the image in this state because if we get a normal paint after a sync decode paint we have to draw the image because if we don't we effectively erase the image.
Much like mask images. This is the easy fix, for now.
We need to override the ASR clips with Nothing() because we don't really want
children of this display item to get the parent filter applied. It's not only
redundant, but also may be incorrect if the mask image is not opaque for example
(maybe WR should prevent that?).
This was caught by layout/reftests/w3c-css/submitted/masking/mask-opacity-1a.html
Differential Revision: https://phabricator.services.mozilla.com/D4351
--HG--
extra : moz-landing-system : lando
Much like mask images. This is the easy fix, for now.
We need to override the ASR clips with Nothing() because we don't really want
children of this display item to get the parent filter applied. It's not only
redundant, but also may be incorrect if the mask image is not opaque for example
(maybe WR should prevent that?).
This was caught by layout/reftests/w3c-css/submitted/masking/mask-opacity-1a.html
Differential Revision: https://phabricator.services.mozilla.com/D4351
--HG--
extra : moz-landing-system : lando
We implement the layout part of offset-path. Now we don't have
offset-distance, so use the default value, 0%, for it.
Note: rename mCombinedTransform as mIndividualTransform, which only
stores the combined individual transforms. We apply the individual
transforms, motion path transform, and specified transform in
ReadTransforms. (We have to follow the order, so we don't combine the
specified transform in FinishStyle.)
Differential Revision: https://phabricator.services.mozilla.com/D2968
The area covered by flattened effects and their children should be considered in the background color selection to avoid defaulting to canvas background color.
Differential Revision: https://phabricator.services.mozilla.com/D3809
--HG--
extra : moz-landing-system : lando
Since fixed position elements are now scrollable, we need to ensure that
they're drawn into the viewport that they're attached to.
MozReview-Commit-ID: ADQXkLjwIzR
--HG--
extra : rebase_source : 2a2ec4983e2ec7f69a3c18389661e00e47ac5277
For each file touched in this patch, the file had an #include for nsContentUtils.h, but no other mentions of the string "nsContentUtils", nor any mention of its "ScriptBlocker"-related types. So these files likely don't need their nsContentUtils.h include anymore, and we can remove it to get a marginal win on build time/complexity.
Differential Revision: https://phabricator.services.mozilla.com/D3370
--HG--
extra : moz-landing-system : lando
This builds on bug 1428676 and introduces StyleAppearance, which replaces the
NS_THEME_* constants.
Really sorry for the size of the patch.
There's a non-trivial change in the gtk theme, which I submitted separately as
bug 1478385.
Differential Revision: https://phabricator.services.mozilla.com/D2361
MozReview-Commit-ID: DiSmMWK7Krp
Since fixed position elements are now scrollable, we need to ensure that they're
drawn using the layout viewport size instead of only the SPC-SPS, since
otherwise they'd be inaccurately clipped when scrolled.
MozReview-Commit-ID: 4p3pWnwluvz
--HG--
extra : rebase_source : f5bc1eae9bf2c8f4b9f78675e9da071de012160e
Effectively we compute the diff by lopping off the rounded part
of each rect where appropriate. This gets a bit weird to think
about with very mismatched radii, but I think it works out(?)
MozReview-Commit-ID: 472gx7Hw1Br
--HG--
extra : rebase_source : 817a84d78c98ac99797ab73b89f8c667189767d1
I've tried to unsuccessfully reproduce the same kind of failures locally under
rr with this patch applied, so that's a good sign.
This should fix the case where we don't invalidate from AttemptPartialUpdate
while trying to sync-decode.
This is a clear part of the problem, though not sure yet if it'll fix all the
issues we see in these tests. In any case I want to investigate those
separately.
Differential Revision: https://phabricator.services.mozilla.com/D2068
MozReview-Commit-ID: H7OEvbQUues
In some cases we get a gecko display list that looks like this:
WrapList with asr(<A>, <B>)
Item with asr (<B>) and clipchain(<something> [A])
In this case, we would initialize the WebRenderLayerScrollData for the
nested item using a stop-at ancestor of A (because that was the leafmost
ASR from the containing WrapList) but the item itself has an ASR of B,
which is an ancestor of A. So when walking up from B we'd never hit the
stop-at ancestor, and so we'd end up duplicating metrics from the
containing WRLSD onto the nested WRLSD. This generated an assertion
failure in the APZ code.
This patch detects this scenario and skips adding metrics on the nested
WRLSD. This produces an APZ tree equivalent to what the non-WebRender
path would produce.
MozReview-Commit-ID: 8eo6pzXXKBd
--HG--
extra : rebase_source : 0581c54c4d9fa6ca08249e42b306c7155022bec7
It's causing cache misses which are showing up in profiles of the displaylist
mutate test case.
To avoid checking Disconnected(), we can destroy the DisplayItemData
and unset the item's pointer to it, rather than calling Disconnect()
in the first place.
To avoid fetching the LayerManager (to compare with mRetainingManager)
we can store this value on the display item along with the
DisplayItemData.
This change ensures that if a nsDisplayItem holds a pointer to a
DisplayItemData then that DisplayItemData holds a pointer back, and
vice-versa.
MozReview-Commit-ID: AAVAs8c0CQY
--HG--
extra : rebase_source : e60c378877618592f311e690219099b1cfc4c9d4
This gets set to a non-zero value when we have an inactive ContainerLayer ancestor (filter in this case).
The current code assumes we'd never call BuildLayer on an image when that happen, but we force the pseudo-active
state here because background-position is animated (all properties have a transition).
MozReview-Commit-ID: 6pL8EJTNgWy
--HG--
extra : rebase_source : 6370fc79d5f47f0b5c4bbe86c0b605b90256b653
In the case of an invalid clip-path, the browser is supposed to discard the
mask entirely. In the non-webrender codepath this would happen
implicitly because the computed MaskUsage would have no flags set, and
so no actions would be taken on the gfxContext which contained the
display items rasterized so far. In the WebRender codepath, though, we
invoke the code on a A8 drawtarget that's zero-filled, so if PaintMask
fails to rasterize anything into it, it gets treated as a "mask everything
out" mask. Instead, this patch makes it so that we detect the scenario
where the computed MaskUsage is a no-op, and ensure that we don't apply
the mask in that case.
An alternative approach considered was to initialize the A8 drawtarget to
white instead of black but in cases where there is an actual mask, the
rest of the code assumes it is zero-filled and so that doesn't work.
MozReview-Commit-ID: Hw7nCiUXVJl
--HG--
extra : rebase_source : 241d550fa0ed1b3bd088c73d9565b166acbcece8
This allows us to call emplace_back, saving us a move.
MozReview-Commit-ID: 4wRiAxl7LSN
--HG--
extra : rebase_source : 0ea07c896c2807a65ad7493a22b8e2b61e68aa01
This allows us to call emplace_back, saving us a move.
MozReview-Commit-ID: 4wRiAxl7LSN
--HG--
extra : rebase_source : 48a0eee8f61f568e0910476dbf672cb43827187e
This call was added in bug 929362, but the key factor to fix the bug was just
setting a flag representing that the target frame doesn't allow the animation
running on the compositor and checking the flag in the function whether the
animation can be run on the compositor or not in later ticks. So the call
wasn't necessary in the first place.
The test case here fails without this fix. The test case actually doesn't
observe animation restyle count at all, so it might look a bit awkward in
file_restyles.html, if we add other test cases checking SchedulePaint calls
in future, we will move the tests in a different file.
(The reason there is no animation restyles in this case is that we properly
throttle the animation in this case.)
MozReview-Commit-ID: AyHciRJHM0s
--HG--
extra : rebase_source : f3963336ea9165b0a9c1a662bdac5c645b209219
This patch is an automatic replacement of s/NS_NOTREACHED/MOZ_ASSERT_UNREACHABLE/. Reindenting long lines and whitespace fixups follow in patch 6b.
MozReview-Commit-ID: 5UQVHElSpCr
--HG--
extra : rebase_source : 4c1b2fc32b269342f07639266b64941e2270e9c4
extra : source : 907543f6eae716f23a6de52b1ffb1c82908d158a
I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this SelectionManager assertion fails when running the Linux debug Web platform tests with e10s test-linux32/debug-web-platform-tests-reftests-e10s-6 W-e10s(Wr6). This assertion failure is bug 1221888.
Marionette INFO Testing http://web-platform.test:8000/css/CSS2/ui/outline-applies-to-005.xht == http://web-platform.test:8000/css/CSS2/reference/no-red-on-blank-page-ref.xht
###!!! ASSERTION: we should have saved a frame property: 'Error', file /builds/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp, line 1038
This patch DOES NOT fix the cause of the assertion failure (a missing HyperTextAccessible). It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.
MozReview-Commit-ID: L26bu4agM6y
--HG--
extra : rebase_source : 9a4188719fe5069cfbec47ae5fae0632ae1d5ee8
extra : intermediate-source : 0a3f719dce16fa80d6ae1bb20a41570050847731
extra : source : aadc67658e679893808256f60c480efeed426bc1
This should improve cache locality and help speed up layer building.
MozReview-Commit-ID: 9IvU23alnaa
--HG--
extra : rebase_source : 0ef20b8b758102876c3e60a4cfd8ffec977590a9
Those changesets which are related to animations introduced in bug 1462497
seems to cause new crashes. I think the first two changesets for the bug fixed
the original crash cases but the last two (i.e. these changesets) introduced
other crashes unfortunately.
MozReview-Commit-ID: 9LD2hIIXA2y
--HG--
extra : rebase_source : 102c5532128039dcf8274c214e17d58d9a1aa483
See comment for explanation. This seemed like the most conservative
way to solve the motionmark regression from bug 1468401.
MozReview-Commit-ID: GoD636xRynF
--HG--
extra : rebase_source : 841c1339cc1875e5c958802260fffd68e71b70e1
It's causing cache misses which are showing up in profiles of the
displaylist mutate test case.
Rather than calling item->Disconnected(), we can ensure the
DisplayItemData is destroyed when it should no longer be used.
We must also make sure we remove the sDisplayItem's dangling pointer.
Rather than calling data->mLayer->Manager() != mRetainingManager, we
can store a pointer to the manager along with the DisplayItemData on
the nsDisplayItem. The item should be in the cache so grabbing this
pointer to perform this check shouldn't cost us anything.
MozReview-Commit-ID: AAVAs8c0CQY
--HG--
extra : rebase_source : 966f343475dd687153310acb8b33db02e202f46f
Per the touch-action spec, the effective touch-action on an element includes
touch-action restrictions from ancestor elements up to and including the
element that has the "default action". This patch implements that behaviour
so that WebRender gets correct touch-action values on its display items.
MozReview-Commit-ID: Cw5uqAsE9qm
--HG--
extra : rebase_source : ef6e24a66385e097d50b3a03c06091464b1bb5b9
This pref was used to enable the building of nsDisplayLayerEventRegions
items without APZ, so that we could test it in isolation. However, we no
longer need to do so, and these display items are going to be deleted
anyway, so we can remove this pref.
MozReview-Commit-ID: LJVcFafCKyS
--HG--
extra : rebase_source : 76d8eeca8dca4ea88b8226bbe6b829dbc40e03e4
This removes the gfx.webrender.hit-test pref, assumes a value of true
everywhere it is used, and deletes all the resulting dead code.
Some gtests were setting this pref to false, and they are now updated to
set gfxVars::UseWebRender to false instead, which has the desired effect
of using the non-WR hit-testing codepath in APZ. (The data needed for
this codepath is set up by the gtests themselves).
MozReview-Commit-ID: 9ljDr8eEnv1
--HG--
extra : rebase_source : fbc321861428e7bb0bf7ab811935b682785debdc
To avoid creating a bunch of layers when we don't need to, this
tracks when a frame needs to be repainted, and invalidates the
IsStyleAnimated logic around mRestyleCounts if the frame is also
being invalidated.
MozReview-Commit-ID: 5Q96Cx6f3V0
--HG--
extra : rebase_source : 4512ef0c6cb50a730f7d5974ef6364d54b6c49b4
Transforms are containing blocks for fixed-pos items, so if a fixed-pos
item is inside a scrolled transform, then it should use that scrollframe
as the scroll target for hit-testing. This patch handles this case for
WebRender by stashing the appropriate ASR on the nsDisplayFixedPosition
item and using it instead of the presShell's root scrollframe in this
scenario.
The patch also adds a mochitest (which is basically a mochitested
version of the reftest in fixed-pos-scrolled-clip-3.html, with a
hit-test check to ensure that it's hitting the right scrollframe).
MozReview-Commit-ID: 7YQAeOiMMuP
--HG--
extra : rebase_source : 8d1c89d0c03c3e7d9383d0731f65a327a2c11a8d
Without this patch, the scrollId for display items inside a fixed-pos
item end as the ASR of the item. In the case of fixed-pos items that are
inside iframes, the ASR is the outer document's root scrollframe. This
means that e.g. wheel-scrolling while over a fixed-pos item inside an
iframe ends up scrolling the outer document's root scrollframe instead
of the iframe's root scrollframe.
In the non-WR codepath, there some APZ machinery that walks up in the
HitTestingTreeNode tree from the hit result, looking to see if that node
has a fixed-pos ancestor, and if so, uses the fixed-pos item's target
APZ as the real hit result. This machinery doesn't exist in WR, because
we don't use the HitTestingTreeNode tree for hit-testing in APZ.
Instead, we need to make sure that the item tag for those display items
already has the appropriate scrollid set.
This patch accomplishes this by introducing a new RAII class that is
pushed into the wr::DisplayListBuilder while we are building display
items inside a nsDisplayFixedPosition, and allows the desired scroll id to
be set on the hit-testing display items.
This behaviour is exercised by test_group_wheelevents, which can now be
enabled with this fix.
MozReview-Commit-ID: L2erPVzJeql
--HG--
extra : rebase_source : 1db630513cb1dc16d4e38649812e81f62c8da99c
GCC doesn't like StyleComplexColor with constructor in an anonymous
struct in an anonymous union. Replace the use of a union to access
`mBorder[..]Color` fields as an array with an accessor methods.
MozReview-Commit-ID: 1Wulh1qKYCZ
--HG--
extra : rebase_source : 390b8f852d144a54d9d374bcf3ae70ab6d145d50
Refactored StyleComplexColor to support "complex" blending between
background (numeric) color and foreground color (currentColor).
Made explicit the distinction between numeric, currentColor and a
complex blend in Gecko and Stylo.
This is to support SMIL animation, for example, of the form:
<animate from="rgb(10,20,30)" by="currentColor" ... />
MozReview-Commit-ID: IUAK8P07gtm
--HG--
extra : rebase_source : d3648101c6f65479b21e6f02945731cd5bb57663
For example, we don't run transform animations on large frames but once
the frame size is small enough we should run transform animations on the frame.
MozReview-Commit-ID: DkfB9g2Gcdi
--HG--
extra : rebase_source : 93a884c3063d4a6c2626e6695583664b24fe11c8
Andrew Osmond
Jonathan Watt
Doug Thayer
Cosmin Sabou
Gerald Squelart
Daniel Varga
Emilio Cobos Álvarez
Jeff Muizelaar
Henrik Winnemöller
Miko Mynttinen
Sebastian Hengst
Dorel Luca
Csoregi Natalia
Timothy Nikkel
Alex Gaynor
Markus Stange
Boris Chiou
Xidorn Quan
Kashav Madan
Daniel Holbert
Margareta Eliza Balazs
Ting-Yu Lin
Matt Woodrow
Coroiu Cristina
Jeremy Lempereur
Masatoshi Kimura
Ryan VanderMeulen
Kashav Madan
Bas Schouten
Kartikaya Gupta
Jamie Nicol
Ciure Andrei
Andreea Pavel
Hiroyuki Ikezoe
Ryan Hunt
Gurzau Raul
Olli Pettay
Martin Robinson
shindli
Chris Peterson
Dan Glastonbury
Tyson Smith
Botond Ballo
Andi-Bogdan Postelnicu
Lee Salzman
arthur.iakab