Closed
Bug 1288302
Opened 8 years ago
Closed 8 years ago
Stylo: implement support for background-image: url()
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
(Blocks 1 open bug)
Details
Attachments
(9 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
bholley
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
bholley
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Details | |
58 bytes,
text/x-review-board-request
|
manishearth
:
review+
|
Details |
5.48 KB,
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
I have some local patches that make background-image: url() values work, though I still need to work out a proper way to kick off image loading from Servo without appending each one to a RwLock<Vec<...>> (which is what my hacky patches are doing, and which ruins restyle performance).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8800090 -
Flags: review?(manishearth)
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8800090 [details]
Support url() values in background-image and mask-image in stylo.
https://reviewboard.mozilla.org/r/85116/#review83676
Attachment #8800090 -
Flags: review?(manishearth) → review+
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8800083 [details]
Bug 1288302 - Part 1: Make css::ImageValue constructable OMT.
https://reviewboard.mozilla.org/r/85102/#review83678
::: layout/style/nsCSSValue.cpp:2849
(Diff revision 1)
> aString,
> do_AddRef(new PtrHolder<nsIURI>(aBaseURI, false)),
> do_AddRef(new PtrHolder<nsIURI>(aReferrer)),
> do_AddRef(new PtrHolder<nsIPrincipal>(aOriginPrincipal)))
> {
> MOZ_ASSERT(NS_IsMainThread());
I think you can remove the main thread check here as far as Initialize() checks that.
Attachment #8800083 -
Flags: review?(xidorn+moz) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.
https://reviewboard.mozilla.org/r/85104/#review83686
::: layout/style/nsStyleStruct.cpp
(Diff revision 1)
> MOZ_ASSERT(mType == eStyleImageType_Image,
> "Can't track image when there isn't one!");
>
> - // Register the image with the document
> + aImageTracker->Add(mImage);
> - nsIDocument* doc = aContext->Document();
> - if (doc) {
I suppose callers may need null check given this... It seems from code, aContext->Document() is not promised to always return non-null, although I'm not sure when that could happen.
Probably consider adding another overload which accepts nsPresContext* and do the null check there. If you believe Document() would never return null in this case, feel free to re-request review with some explanation.
Attachment #8800084 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.
https://reviewboard.mozilla.org/r/85104/#review83686
> I suppose callers may need null check given this... It seems from code, aContext->Document() is not promised to always return non-null, although I'm not sure when that could happen.
>
> Probably consider adding another overload which accepts nsPresContext* and do the null check there. If you believe Document() would never return null in this case, feel free to re-request review with some explanation.
I believe aContext->Document() will only return null after nsPresContext::DetachShell() is called. That DetachShell() call is done at the end of PresShell::Destroy(), and this happens after both the frame tree (which is holding on to some style contexts) is destroyed, and the style set is destroyed (which, through the rule tree, holds on to the remaining style contexts). So I believe at this point we won't have any more nsStyleFoo::Destroy() calls. If some style structs were held alive after the pres shell went away, then I think the pres arena would assert that some of its objects weren't destroyed.
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.
https://reviewboard.mozilla.org/r/85104/#review83686
> I believe aContext->Document() will only return null after nsPresContext::DetachShell() is called. That DetachShell() call is done at the end of PresShell::Destroy(), and this happens after both the frame tree (which is holding on to some style contexts) is destroyed, and the style set is destroyed (which, through the rule tree, holds on to the remaining style contexts). So I believe at this point we won't have any more nsStyleFoo::Destroy() calls. If some style structs were held alive after the pres shell went away, then I think the pres arena would assert that some of its objects weren't destroyed.
OK. It seems nsPresContext::mDocument is actually never cleared even after it is detached from PresShell. And there is an assertion that when it attaches to shell, there is a document, so not having a document indicates that the nsPresContext is never attached to any PresShell. Given that style structs and frame tree are ultimately owned by PresShell, such a nsPresContext should never be used in this case.
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8800084 [details]
Bug 1288302 - Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext.
https://reviewboard.mozilla.org/r/85104/#review83756
Attachment #8800084 -
Flags: review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review83958
Cancel r? mainly for the issue when image load fails.
::: layout/style/nsStyleStruct.h:365
(Diff revision 1)
> + mozilla::css::ImageValue* mImageValue; // we can AddRef on an arbitrary
> + // thread, but Release on main
I'd prefer using RefPtr here rather than manually maintaining the refcount.
::: layout/style/nsStyleStruct.cpp:1992
(Diff revision 1)
> + if (!req) {
> + // The URL resolution or image load failed.
> + return false;
> + }
When this happens, mResolved is true, while mRequestProxy is null. There would be null-deference if {Lock,Unlock}Image is called, and {Track,Untrack}Image would either assert or operate on an invalid pointer.
Should the callsites of those methods guarantee to only call them when .get() returns non-null? If yes, you probaly want an extra assertion in those methods. If no, you may want to add another branch to check mRequestProxy.
Attachment #8800085 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review83958
> I'd prefer using RefPtr here rather than manually maintaining the refcount.
Oh, NS_ReleaseOnMainThread(someRefPtrVariable.forget()) is an established pattern. I'll use that.
> When this happens, mResolved is true, while mRequestProxy is null. There would be null-deference if {Lock,Unlock}Image is called, and {Track,Untrack}Image would either assert or operate on an invalid pointer.
>
> Should the callsites of those methods guarantee to only call them when .get() returns non-null? If yes, you probaly want an extra assertion in those methods. If no, you may want to add another branch to check mRequestProxy.
That's true. But in a later patch I will check the return value from Resolve(), and set an nsStyleImage (which stores an nsStyleImageRequest) from nsStyleImageType_Image to eStyleImageType_Null. Still, probably better to null check mRequestProxy inside LockImage etc., in case we use nsStyleImageRequest in a different way for some other properties.
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8800086 [details]
Bug 1288302 - Part 4: Perform final main thread work on style structs sourced from ServoComputedValues.
https://reviewboard.mozilla.org/r/85108/#review84018
Attachment #8800086 -
Flags: review?(xidorn+moz) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85110/#review83968
::: layout/style/nsRuleNode.cpp:140
(Diff revision 1)
> +static void
> +SetStyleImageRequest(function<void(nsStyleImageRequest*)> aCallback,
> + nsPresContext* aPresContext,
> + const nsCSSValue& aValue)
I don't really understand how does it make sense to use callback here? Wouldn't it be easier to make this function, as well as SetImageRequest, just return an already_AddRefed<_>?
::: layout/style/nsStyleStruct.h:898
(Diff revision 1)
> + for (uint32_t i = 0; i < mImageCount; ++i)
> + mLayers[i].ResolveImages(aContext);
Please wrap the loop body with {}.
::: layout/style/nsStyleStruct.cpp:2026
(Diff revision 1)
> + aImageTracker->AddRef();
> + nsMainThreadPtrHandle<imgRequestProxy> req = mRequestProxy;
> + NS_DispatchToMainThread(NS_NewRunnableFunction([aImageTracker, req]() mutable {
> + aImageTracker->Remove(req);
> + aImageTracker->Release();
> + }));
I'm concerned about the lifetime management here. Please create an nsRunnable for it rather than using lambda.
(Actually using lambda could be fine if we can use "initialized lambda captures". But we can't.)
Assignee | ||
Comment 19•8 years ago
|
||
I found another problem with the design of nsStyleImageRequest, while trying to use it for list-style-image. It can only buffer up a single call to TrackImage() before it's resolved. But if we share the same nsStyleImageRequest object in multiple style structs (as will happen when inheriting the property), we'll try to call TrackImage() more than once on the same object.
So either this buffering up of TrackImage calls needs to be done at on the object that stores the RefPtr<nsStyleImageRequest>, or the nsStyleImageRequest itself needs to be able to count how many TrackImage() calls it's had before being resolved. The latter would also need to assume that the ImageTracker object being used when finally acting on the buffered up TrackImage() calls is the same, no matter which style struct is the first one to resolve the nsStyleImageRequest. Which is true, but it seems cleaner to do it separately from each struct.
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85110/#review83968
> I don't really understand how does it make sense to use callback here? Wouldn't it be easier to make this function, as well as SetImageRequest, just return an already_AddRefed<_>?
Yeah, I'm not sure why I didn't convert the macros into that pattern. Can I do that as a followup?
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85110/#review83968
> Yeah, I'm not sure why I didn't convert the macros into that pattern. Can I do that as a followup?
OK.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•8 years ago
|
||
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review84930
Mostly looks good to me. But I'm not very familiar with all the image things, so I'd want dholbert to check as well.
::: layout/style/nsStyleStruct.h:322
(Diff revision 2)
> + // Mode describing whether LockImage/UnlockImage calls will be made
> + // when obtaining and releasing the imgRequestProxy, and additionally
> + // whether RequestDiscard will be called on release.
> + enum class Mode : uint8_t {
> + Locking, // used by most nsStyleImageRequest
> + LockingAndDiscarding, // only used by nsCursorImage
> + NonLocking, // only used by nsStyleContentData
> + };
It might be clearer to do it via flags, that is, having two flags, one for locking, and the other for requesting discard on destroy.
::: layout/style/nsStyleStruct.cpp:1970
(Diff revision 2)
> + {
> + RefPtr<StyleImageRequestCleanupTask> task =
> + new StyleImageRequestCleanupTask(mLockingMode,
> + mRequestProxy.forget(),
> + mImageValue.forget(),
> + mImageTracker.forget());
Should we probably just skip this block if mRequestProxy is nullptr and assert in the task that mRequestProxy is non-null?
It doesn't seem to make sense to create this cleanup task if we know it wouldn't do anything.
Attachment #8800085 -
Flags: review?(xidorn+moz) → review+
Updated•8 years ago
|
Attachment #8800085 -
Flags: review?(dholbert)
Assignee | ||
Comment 32•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review84930
> It might be clearer to do it via flags, that is, having two flags, one for locking, and the other for requesting discard on destroy.
OK, I'll change this to use flags. I realised yesterday that the mode for nsCursorImage is wrong, anyway, since for cursors we don't want to add them to the ImageTracker, but we still do want to lock/unlock/discard.
> Should we probably just skip this block if mRequestProxy is nullptr and assert in the task that mRequestProxy is non-null?
>
> It doesn't seem to make sense to create this cleanup task if we know it wouldn't do anything.
We still need to release the mImageValue on the main thread, though.
Comment 33•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review84930
> We still need to release the mImageValue on the main thread, though.
Oh, hmmm, yeah, you're right.
Comment 34•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review84930
> OK, I'll change this to use flags. I realised yesterday that the mode for nsCursorImage is wrong, anyway, since for cursors we don't want to add them to the ImageTracker, but we still do want to lock/unlock/discard.
Yeah, since document doesn't render it. That probably means you need three separate flags, then.
Updated•8 years ago
|
Attachment #8800087 -
Flags: review?(dholbert)
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8800088 [details]
Bug 1288302 - Part 6: Add FFI function to set nsStyleImageRequest values.
https://reviewboard.mozilla.org/r/85112/#review84948
Attachment #8800088 -
Flags: review?(xidorn+moz) → review+
Comment 36•8 years ago
|
||
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
Hmm, MozReview still doesn't handle redirecting review properly.
Attachment #8800087 -
Flags: review?(xidorn+moz)
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8800088 [details]
Bug 1288302 - Part 6: Add FFI function to set nsStyleImageRequest values.
https://reviewboard.mozilla.org/r/85112/#review84956
::: layout/style/ServoBindings.cpp:732
(Diff revision 2)
> + RefPtr<nsStyleImageRequest> req =
> + new nsStyleImageRequest(nsStyleImageRequest::Mode::Locking, urlBuffer,
> + do_AddRef(aBaseURI), do_AddRef(aReferrer),
> + do_AddRef(aPrincipal));
> + aImage->SetImageRequest(req);
This makes me think nsStyleImage::SetImageRequest should probably take already_AddRefed<_> rather than a pointer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 46•8 years ago
|
||
mozreview-review |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review85016
::: layout/style/nsStyleStruct.h:369
(Diff revision 3)
> +
> +private:
> + ~nsStyleImageRequest();
> + nsStyleImageRequest& operator=(const nsStyleImageRequest& aOther) = delete;
> +
> + void TrackAndLock();
Probably better renaming this to MayTrackAndLock() given it doesn't always do that.
::: layout/style/nsStyleStruct.cpp:40
(Diff revision 3)
> #include "mozilla/dom/AnimationEffectReadOnlyBinding.h" // for PlaybackDirection
> #include "mozilla/dom/ImageTracker.h"
> #include "mozilla/Likely.h"
> #include "nsIURI.h"
> #include "nsIDocument.h"
> +#include "nsNetUtil.h"
It doesn't seem to me any code you add in this file need this?
Comment 47•8 years ago
|
||
mozreview-review |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review85734
::: layout/style/nsStyleStruct.h:309
(Diff revision 3)
> + * any thread. The nsStyleImageRequest is not considered "resolved"
> + * at this point, and the Resolve() method must be called later
> + * to initiate the image load and make calls to get() valid.
> + *
> + * Calls to TrackImage(), UntrackImage(), LockImage(), UnlockImage() and
> + * RequestDiscard() are made to the imgRequestProxy as appropriate, according
nit: to the proxy and the tracker.
::: layout/style/nsStyleStruct.h:317
(Diff revision 3)
> + * The main thread constructor takes a pointer to the css::ImageValue that
> + * is the specified url() value, while the off-main-thread constructor
> + * creates a new css::ImageValue to represent the url() information passed
> + * to the constructor. This ImageValue is held on to for the comparisons done
> + * in DefinitelyEquals(), so that we don't need to call into the non-OMT-safe
> + * imgRequestProxy::Equals().
I don't see imgRequestProxy::Equals anywhere. What is this referring to?
::: layout/style/nsStyleStruct.h:350
(Diff revision 3)
> + const imgRequestProxy* get() const {
> + MOZ_ASSERT(IsResolved(), "Resolve() must be called first");
> + MOZ_ASSERT(NS_IsMainThread());
> + return mRequestProxy.get();
> + }
Nit: given the three lines it's probably nicer to const-cast-call the non-const method here.
::: layout/style/nsStyleStruct.h:361
(Diff revision 3)
> + MOZ_ASSERT(IsResolved(), "Resolve() must be called first");
> + MOZ_ASSERT(NS_IsMainThread());
> + return mRequestProxy.get();
> + }
> +
> + bool DefinitelyEquals(const nsStyleImageRequest& aOther) const;
Would be good to add a comment indicating what kind of equality we're talking about here.
::: layout/style/nsStyleStruct.cpp:1910
(Diff revision 3)
> + if (!mRequestProxy) {
> + return NS_OK;
> + }
> +
> + MOZ_ASSERT(mImageTracker);
I'd rather just avoid instatiating and dispatching the cleanup task if these things are null.
::: layout/style/nsStyleStruct.cpp:2011
(Diff revision 3)
> + nsCSSValue value;
> + value.SetImageValue(mImageValue);
> + mRequestProxy = value.GetImageValue(aPresContext->Document());
> + if (!aPresContext->IsDynamic()) {
> + mRequestProxy = nsContentUtils::GetStaticRequest(mRequestProxy);
> + }
I'm a bit wary of duplicating this code from SetImageRequest. Can we find a way to share it? Or does it go away?
Attachment #8800085 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 48•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review85734
> I don't see imgRequestProxy::Equals anywhere. What is this referring to?
Oh, that should be something like "Equals on the nsIURI objects returned from imgRequestProxy::GetURI".
> I'd rather just avoid instatiating and dispatching the cleanup task if these things are null.
We always have a css::ImageValue object to release.
> I'm a bit wary of duplicating this code from SetImageRequest. Can we find a way to share it? Or does it go away?
It doesn't go away. I'll find a way to share it.
Comment 49•8 years ago
|
||
mozreview-review |
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85110/#review85746
::: layout/base/nsCSSRendering.cpp
(Diff revision 3)
> - // We could do something fancy to avoid the TrackImage/UntrackImage
> - // work, but it doesn't seem worth it. (We need to call TrackImage
> - // since we're not going through nsRuleNode::ComputeBorderData.)
> - newStyleBorder.TrackImage(aPresContext->Document()->ImageTracker());
What replaces this work?
::: layout/style/nsComputedDOMStyle.cpp:2087
(Diff revision 3)
> + // XXXheycam If we had some problem resolving the imgRequestProxy,
> + // maybe we should just use the URL stored in the nsStyleImage's
> + // mImageValue?
What does the spec say?
::: layout/style/nsRuleNode.cpp:144
(Diff revision 3)
> + nsStyleImageRequest::Mode aModeFlags =
> + nsStyleImageRequest::Mode::Track |
> + nsStyleImageRequest::Mode::Lock)
Are there any callers with the non-default for the last param? I don't see them in this patch.
::: layout/style/nsStyleStruct.h
(Diff revision 3)
> -#ifdef DEBUG
> - bool mImageTracked;
> -#endif
Really glad to see these bools go, 7 years and a bit after I added them. ;-)
::: layout/style/nsStyleStruct.h:823
(Diff revision 3)
> ~Layer();
>
> // Initialize mRepeat and mOrigin by specified layer type
> void Initialize(LayerType aType);
>
> - // Register/unregister images with the document. We do this only
> + void ResolveImages(nsPresContext* aContext) {
Seems like this should be (singular) ResolveImage?
Attachment #8800087 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 50•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800087 [details]
Bug 1288302 - Part 5: Make nsStyleImage use nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85110/#review85746
> What replaces this work?
Nothing; this tracking was only to avoid an assertion that checked whether we had called TrackImage. Now that we are sharing the same nsStyleImageRequest object, which itself has called TrackImage, we don't need to do anything.
> What does the spec say?
If the problem was just in producing the imgRequestProxy, I'm pretty sure we should still be returning the absolutized URL. The spec says that if resolving a relative URL to an absolute URL fails, we should return the relative URL. I'll file a followup bug to do this. (The current Gecko behaviour in this case is to return "none".)
> Are there any callers with the non-default for the last param? I don't see them in this patch.
No, but is one in bug 1310560.
> Seems like this should be (singular) ResolveImage?
Yeah. I was following the naming scheme of TrackImages and LockImages, which also on some objects only worked on a single image, but now that they are being removed I should fix this to be the singular.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 59•8 years ago
|
||
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0488e9c0024
Part 1: Make css::ImageValue constructable OMT. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf6ad588219
Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc27315f0b4
Part 3: Add nsStyleImageRequest. r=xidorn,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb97ae080e7
Part 4: Perform final main thread work on style structs sourced from ServoComputedValues. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/daba6ffe8319
Part 5: Make nsStyleImage use nsStyleImageRequest. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/6861f02dadfa
Part 6: Add FFI function to set nsStyleImageRequest values. r=xidorn
Comment 60•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0488e9c0024
https://hg.mozilla.org/mozilla-central/rev/4bf6ad588219
https://hg.mozilla.org/mozilla-central/rev/4dc27315f0b4
https://hg.mozilla.org/mozilla-central/rev/cbb97ae080e7
https://hg.mozilla.org/mozilla-central/rev/daba6ffe8319
https://hg.mozilla.org/mozilla-central/rev/6861f02dadfa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 61•8 years ago
|
||
Backed out for the bug 1311921 Talos regressions: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd0f297a13d
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 62•8 years ago
|
||
OK, think I've found the problem.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4241b302fb8f&newProject=try&newRevision=5738016310e2&framework=1&showOnlyImportant=0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c5486099fe132cd26bc274c543e4c5dc411a048
Assignee | ||
Comment 63•8 years ago
|
||
I was basically calling LockImage() twice (once due to processing Mode::Track, once for Mode::Lock), which meant we never unlocked an image until the ImageTracker itself went away.
Attachment #8806293 -
Flags: review?(xidorn+moz)
Comment 64•8 years ago
|
||
mozreview-review |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review89224
::: layout/style/nsStyleStruct.cpp:1928
(Diff revision 4)
> + if (mModeFlags & Mode::Discard) {
> + mRequestProxy->RequestDiscard();
> + }
Discard should probably be the last to call, because it is basically a no-op if there is still any lock, which means if anything does "Track | Discard", the discard may never take effect. (It seems we currently don't have anything does that, though.)
Comment 65•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #63)
> Created attachment 8806293 [details] [diff] [review]
> Part 5.1: Don't use Track and Lock flags together for background-images.
>
> I was basically calling LockImage() twice (once due to processing
> Mode::Track, once for Mode::Lock), which meant we never unlocked an image
> until the ImageTracker itself went away.
I don't quite understand. Even if one uses Track, the image would still be automatically removed from ImageTracker when nsStyleImageRequest passes away, right? And if it is the last one which requests tracking the image, the image would also be unlocked by ImageTracker, wouldn't it? Then why did we never unlock the image?
Updated•8 years ago
|
Flags: needinfo?(cam)
Assignee | ||
Comment 66•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #65)
> I don't quite understand. Even if one uses Track, the image would still be
> automatically removed from ImageTracker when nsStyleImageRequest passes
> away, right? And if it is the last one which requests tracking the image,
> the image would also be unlocked by ImageTracker, wouldn't it? Then why did
> we never unlock the image?
The only time we end up with the imgRequestProxy having its mLockCount go down to zero was when the ImageTracker went away (or if restyling removed the image). This is because the nsStyleImageRequest itself did one lock, and the ImageTracker did another lock. When we switch tabs, for example, we'll call ImageTracker::SetLockingState(false), to bulk-unlock all of the images in the document. But since every CSS image had an additional lock held by the nsStyleImageRequest, we were never able to discard decoded image data until the image or the whole document went away.
So I believe this was just causing us to keep decoded image data around longer than we are currently. I plotted some graphs of the RSS measurements after each page reload in the Tp5 test, and it did seem like the drops in memory were shifted a bit later. The net effect being that the average memory usage over the entire run was higher.
Flags: needinfo?(cam)
Assignee | ||
Comment 67•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800085 [details]
Bug 1288302 - Part 3: Add nsStyleImageRequest.
https://reviewboard.mozilla.org/r/85106/#review89224
> Discard should probably be the last to call, because it is basically a no-op if there is still any lock, which means if anything does "Track | Discard", the discard may never take effect. (It seems we currently don't have anything does that, though.)
Good point. I'll fix that when re-landing.
Comment 68•8 years ago
|
||
Talked with heycam on IRC, and it seems we hold all the style structs when a document is put into the bfcache. And the old patches of this bug makes style structs lock the images, which prevents their data from being destroyed, and thus the regression on memory usage.
The idea about tracking vs. locking for images in style data is from bug 512260, and we do not yet have any document about when an image should be tracked vs. locked by the style struct. Given that previously, only list-style-image and cursor lock the image, it is supposed that we lock images which are usually small, and track the rest.
This issue brings me an idea that we can probably put the mode flags in the type level, so it could be easier to reason about the behavior of each nsStyleImageRequest without investigating all paths it could be setup. But that could be done in a separate bug, I guess.
Actually I think it is possible that it isn't worth distinguishing between list-style-image, cursor, and other images. We can probably just drop the mode flag and do track on all images, and manually discard cursor image in the style struct rather than in nsStyleImageRequest. But that certainly should be done in a separate bug.
Comment 69•8 years ago
|
||
Comment on attachment 8806293 [details] [diff] [review]
Part 5.1: Don't use Track and Lock flags together for background-images.
Review of attachment 8806293 [details] [diff] [review]:
-----------------------------------------------------------------
In addition to the comment below, you may need to update your part 6 as well.
::: layout/style/nsStyleStruct.h
@@ +327,3 @@
> enum class Mode : uint8_t {
> Track = 0x1, // used by all except nsCursorImage
> + Lock = 0x2, // used only by nsStyleList
It seems to me we do lock for cursor image as well, not only list-style-image.
So if those two are exclusive to each other and we want either one anyway, should we make them one flag? It could probably just be Lock, and if the flag is not set, track it, and if it is set, lock it.
Attachment #8806293 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 70•8 years ago
|
||
Comment on attachment 8806293 [details] [diff] [review]
Part 5.1: Don't use Track and Lock flags together for background-images.
Review of attachment 8806293 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/style/nsStyleStruct.h
@@ +327,3 @@
> enum class Mode : uint8_t {
> Track = 0x1, // used by all except nsCursorImage
> + Lock = 0x2, // used only by nsStyleList
OK. It might be a little less explicit, but would prevent passing in both mutually exclusive options.
Whatever mode flags are passed in, the image is going to be locked (since the ImageTracker will do locking). So I think the flag we keep should be Track rather than Lock. Lack of the Track flag would then do Locking itself, instead.
Assignee | ||
Comment 71•8 years ago
|
||
Attachment #8806293 -
Attachment is obsolete: true
Attachment #8806600 -
Flags: review?(xidorn+moz)
Updated•8 years ago
|
Attachment #8806600 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 72•8 years ago
|
||
Comment 73•8 years ago
|
||
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2645d5688d
Part 1: Make css::ImageValue constructable OMT. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c122cf760d7
Part 2: Pass ImageTracker to style struct image tracking methods instead of nsPresContext. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/731ccb5d2ce4
Part 3: Add nsStyleImageRequest. r=xidorn,bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/65583d15ca91
Part 4: Perform final main thread work on style structs sourced from ServoComputedValues. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/112b8aac9300
Part 5: Make nsStyleImage use nsStyleImageRequest. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c213c3194aa8
Part 5.1: Merge Track and Lock flags so we don't set them together. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef55ee026e2
Part 6: Add FFI function to set nsStyleImageRequest values. r=xidorn
Comment 74•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e2645d5688d
https://hg.mozilla.org/mozilla-central/rev/8c122cf760d7
https://hg.mozilla.org/mozilla-central/rev/731ccb5d2ce4
https://hg.mozilla.org/mozilla-central/rev/65583d15ca91
https://hg.mozilla.org/mozilla-central/rev/112b8aac9300
https://hg.mozilla.org/mozilla-central/rev/c213c3194aa8
https://hg.mozilla.org/mozilla-central/rev/4ef55ee026e2
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•