Closed
Bug 697230
Opened 13 years ago
Closed 12 years ago
Make style image decode block onload
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(4 files, 5 obsolete files)
238.27 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
39.10 KB,
patch
|
khuey
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
4.77 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
75.95 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
We do it for DOM images, we should do it for style images too. It would also eliminate the need for Bug 695765.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #585726 -
Flags: review?(dbaron)
Comment 4•12 years ago
|
||
Comment on attachment 585726 [details] [diff] [review] Part 1: Centralize image observers >--- /dev/null >+++ b/layout/style/ImageLoader.cpp >+ImageLoader::SetAnimationModeEnumerator(nsISupports* aKey, FrameSet* aValue, >+#ifdef DEBUG >+ { >+ nsCOMPtr<imgIRequest> debugRequest = do_QueryInterface(aKey); >+ NS_ASSERTION(debugRequest == request, "This is bad"); >+ } >+#endif MOZ_ASSERT(nsCOMPtr<imgIRequest>(do_QueryInterface(aKey))); >+ImageLoader::Init() >+{ >+ NS_ASSERTION(mDocument, "Waht?"); "What" >+ImageLoader::AssociateRequestToFrame(imgIRequest* aRequest, >+ nsIFrame* aFrame) >+{ >+ bool result = true; Move this to the two places where you need it. >+ImageLoader::MaybeAddRequest(imgIRequest* aRequest) >+{ >+ // See if we already know about this request. >+ nsCOMPtr<imgIDecoderObserver> observer; >+ >+ aRequest->GetDecoderObserver(getter_AddRefs(observer)); No need for the empty line. >+imgIRequest* >+ImageLoader::FindAppropriateClone(imgIRequest* aRequest) >+{ >+ NS_ASSERTION(aRequest, "This should never be null"); >+ >+ imgIRequest* request = nsnull; >+ >+ request = mClonedRequests.GetWeak(aRequest); imgIRequest* request = mClonedRequests.GetWeak(aRequest); >+ >+ request = request ? request : aRequest; if (!request) { request = aRequest; } >+ImageLoader::DoRedraw(FrameSet* aFrameSet) >+{ >+ // Invalidate the entire frame >+ // XXX We really only need to invalidate the client area of the frame... TODO and a bug number, please. And in general, I'd suggest MOZ_ASSERTs. >--- /dev/null >+++ b/layout/style/ImageLoader.h >+class ImageLoader : public nsStubImageDecoderObserver { { on a new line. >+public: >+ ImageLoader(nsIDocument* aDocument) >+ : mDocument(aDocument), >+ mHavePainted(false), >+ mInClone(false) ImageLoader(nsIDocument* aDocument) : mDocument(aDocument) , mHavePainted(false) , mInClone(false) >+ // imgIDecoderObserver (override nsStubImageDecoderObserver) >+ NS_IMETHOD OnStartContainer(imgIRequest *aRequest, imgIContainer *aImage); >+ NS_IMETHOD OnStopRequest(imgIRequest *aRequest, bool aLastPart); >+ NS_IMETHOD OnImageIsAnimated(imgIRequest *aRequest); * is on the wrong side
So one thing I'm a bit worried about here (just based on the commit message) is that this patch has a pretty deep dependency on bug 497995, which we can't ship yet (and will likely have to back out of the next aurora) because of bug 713643.
Assignee | ||
Comment 6•12 years ago
|
||
The dependency is actually not very deep at all.
Comment 7•12 years ago
|
||
Comment on attachment 585726 [details] [diff] [review] Part 1: Centralize image observers >+++ b/layout/style/ImageLoader.cpp Please use the new MPL License block. Bonus points for editor modelines. >+// A class that handles style system image loads (other image loads are handled >+// by the nodes in the content tree). Please use a /* */ comment for the mxr comment. >+ImageLoader::DropDocumentReference() >+ // After we return, the only thing that has an owning reference to us should >+ // be our caller. I don't understand what that comment, has to do with the following line. Just nix it? >+bool >+ImageLoader::AssociateRequestToFrame(imgIRequest* aRequest, None of the callers check the return value. Why have it at all? >+ bool result = true; Declare this in the places where you actually use it, please. >+ NS_ASSERTION(mRequests.IsInitialized() && >+ mFrames.IsInitialized() && >+ mClonedRequests.IsInitialized(), "This is bad!"); MOZ_ASSERT, please. This just shouldn't happen. As discussed on irc, this looks like it can double-add frames/requests. If the *Set clases are meant to be sets, they should be idempotent on double-adds. >+ImageLoader::DisassociateRequestFromFrame(imgIRequest* aRequest, This needs to do something with removing stuff from a RequestSet, I'd think. >+ImageLoader::ClearAll() Perhaps call this Reset? >+ for (OwningRequestSet::size_type i = mAllRequests.Length(); i != 0; --i) { I'm not sure I understand this mAllRequests thing. Doesn't it effectively leak things for the page lifetime if someone keeps setting style.background to different images? Why do we need it? Seems like the cloned request map might have similar issues, by the way. >+ImageLoader::OnImageIsAnimated(imgIRequest* aRequest) >+ nsLayoutUtils::RegisterImageRequest(mDocument->GetShell()->GetPresContext(), GetShell() can be null, no? >+++ b/layout/style/ImageLoader.h I'd really like to see some documentation about lifetimes here, as well as documentation for the various public APIs. >+ typedef nsClassHashtable<nsISupportsHashKey, >+ FrameSet> RequestHashtable; How about RequestToFrameMap? Similar for FrameToRequestMap. And s/mFrames/mFrameToRequestMap/
Attachment #585726 -
Flags: review?(dbaron) → review-
Comment 8•12 years ago
|
||
TTo be clear, I think the general lifetime management in that patch is broken.....
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #570721 -
Attachment is obsolete: true
Attachment #585726 -
Attachment is obsolete: true
Attachment #607395 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #607396 -
Flags: superreview?(bzbarsky)
Attachment #607396 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #607396 -
Flags: review? → review?(joe)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #607398 -
Flags: review?(bzbarsky)
Comment 12•12 years ago
|
||
omigodomigodomigod
Comment 13•12 years ago
|
||
Comment on attachment 607396 [details] [diff] [review] Part 2: Push onload blocking logic down into Imagelib. Review of attachment 607396 [details] [diff] [review]: ----------------------------------------------------------------- Mostly nit-ish things. Hooray! \o/ ::: content/base/src/nsImageLoadingContent.h @@ +62,5 @@ > class imgILoader; > class nsIIOService; > > +class nsImageLoadingContent : public nsIImageLoadingContent, > + public imgIOnloadBlocker I think you should also remove nsImageLoadingContent::mBlockingOnload, right? ::: image/public/imgIOnloadBlocker.idl @@ +5,5 @@ > + * > + * The contents of this file are subject to the Mozilla Public License Version > + * 1.1 (the "License"); you may not use this file except in compliance with > + * the License. You may obtain a copy of the License at > + * http://www.mozilla.org/MPL/ Just use MPL2 ::: image/src/imgRequest.cpp @@ +671,5 @@ > + > + if (mBlockingOnload) { > + mBlockingOnload = false; > + > + tracker.RecordUnblockOnload(); I don't understand why you unblock onload on both stopframe and stopcontainer. Can you elaborate & add a comment?
Attachment #607396 -
Flags: review?(joe) → review+
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Joe Drew (:JOEDREW!) from comment #13) > Comment on attachment 607396 [details] [diff] [review] > Part 2: Push onload blocking logic down into Imagelib. > > Review of attachment 607396 [details] [diff] [review]: > ----------------------------------------------------------------- > > Mostly nit-ish things. Hooray! \o/ > > ::: content/base/src/nsImageLoadingContent.h > @@ +62,5 @@ > > class imgILoader; > > class nsIIOService; > > > > +class nsImageLoadingContent : public nsIImageLoadingContent, > > + public imgIOnloadBlocker > > I think you should also remove nsImageLoadingContent::mBlockingOnload, right? I bet I should. > ::: image/public/imgIOnloadBlocker.idl > @@ +5,5 @@ > > + * > > + * The contents of this file are subject to the Mozilla Public License Version > > + * 1.1 (the "License"); you may not use this file except in compliance with > > + * the License. You may obtain a copy of the License at > > + * http://www.mozilla.org/MPL/ > > Just use MPL2 I've been told elsewhere just to wait for the mass change. > ::: image/src/imgRequest.cpp > @@ +671,5 @@ > > + > > + if (mBlockingOnload) { > > + mBlockingOnload = false; > > + > > + tracker.RecordUnblockOnload(); > > I don't understand why you unblock onload on both stopframe and > stopcontainer. Can you elaborate & add a comment? Did you read the comment in OnStopContainer?
Assignee | ||
Comment 15•12 years ago
|
||
Unbitrotted.
Attachment #607395 -
Attachment is obsolete: true
Attachment #607395 -
Flags: review?(bzbarsky)
Attachment #612889 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•12 years ago
|
||
Unbitrotted.
Attachment #607396 -
Attachment is obsolete: true
Attachment #607396 -
Flags: superreview?(bzbarsky)
Attachment #612891 -
Flags: superreview?(bzbarsky)
Attachment #612891 -
Flags: review+
Comment 17•12 years ago
|
||
Comment on attachment 612889 [details] [diff] [review] Part 1: Centralize image observers r=me with the nits on IRC.
Attachment #612889 -
Flags: review?(bzbarsky) → review+
Comment 18•12 years ago
|
||
Comment on attachment 612891 [details] [diff] [review] Part 2: Push onload blocking logic down into imagelib sr=me
Attachment #612891 -
Flags: superreview?(bzbarsky) → superreview+
Comment 19•12 years ago
|
||
Comment on attachment 607398 [details] [diff] [review] Part 3: Make style images block onload Nix the window stuff in ImageLoader::UnblockOnload? Or at least document very clearly why it's there if it's there for a reason. r=me with that.
Attachment #607398 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•12 years ago
|
||
I tried to land this but it turned Android reftests very orange :-(
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #618049 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #612889 -
Attachment is obsolete: true
Attachment #618051 -
Flags: review+
Comment 23•12 years ago
|
||
Comment on attachment 618049 [details] [diff] [review] Part 0: Make table frame classes call into their superclass in DidSetStyleContext r=me
Attachment #618049 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5709499c7c66 https://hg.mozilla.org/mozilla-central/rev/5c730c1f2274 https://hg.mozilla.org/mozilla-central/rev/55b5cab554b5 https://hg.mozilla.org/mozilla-central/rev/54f586a529ae
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•