Last Comment Bug 697230 - Make style image decode block onload
: Make style image decode block onload
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: mozilla17
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
:
Mentors:
Depends on: 838513 714752 783015 783126 783162 783220 783355 783906 784068 789846 791571 816498 818388 819922 843308 874763 1170799
Blocks: 432391 685516 688897 1227377
  Show dependency treegraph
 
Reported: 2011-10-25 12:57 PDT by Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
Modified: 2015-11-23 18:21 PST (History)
20 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (235.26 KB, patch)
2011-10-31 08:41 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
no flags Details | Diff | Review
Part 1: Centralize image observers (64.73 KB, patch)
2012-01-04 06:19 PST, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bzbarsky: review-
Details | Diff | Review
Part 1: Centralize Image Observers (78.25 KB, patch)
2012-03-19 17:39 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
no flags Details | Diff | Review
Part 2: Push onload blocking logic down into Imagelib. (39.20 KB, patch)
2012-03-19 17:40 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
joe: review+
Details | Diff | Review
Part 3: Make style images block onload (238.27 KB, patch)
2012-03-19 17:43 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bzbarsky: review+
Details | Diff | Review
Part 1: Centralize image observers (78.20 KB, patch)
2012-04-06 08:18 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bzbarsky: review+
Details | Diff | Review
Part 2: Push onload blocking logic down into imagelib (39.10 KB, patch)
2012-04-06 08:19 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
khuey: review+
bzbarsky: superreview+
Details | Diff | Review
Part 0: Make table frame classes call into their superclass in DidSetStyleContext (4.77 KB, patch)
2012-04-24 14:40 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bzbarsky: review+
Details | Diff | Review
Part 1: Centralize image observers (75.95 KB, patch)
2012-04-24 14:40 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
khuey: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-25 12:57:03 PDT
We do it for DOM images, we should do it for style images too.

It would also eliminate the need for Bug 695765.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-31 08:41:38 PDT
Created attachment 570721 [details] [diff] [review]
Testcase
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-10 07:37:03 PST
I'm going to do this.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-01-04 06:19:07 PST
Created attachment 585726 [details] [diff] [review]
Part 1: Centralize image observers
Comment 4 :Ms2ger 2012-01-04 06:41:59 PST
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
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-01-24 15:12:26 PST
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.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-01-24 22:51:44 PST
The dependency is actually not very deep at all.
Comment 7 Boris Zbarsky [:bz] 2012-02-29 22:08:47 PST
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/
Comment 8 Boris Zbarsky [:bz] 2012-02-29 22:09:31 PST
TTo be clear, I think the general lifetime management in that patch is broken.....
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-19 17:39:21 PDT
Created attachment 607395 [details] [diff] [review]
Part 1: Centralize Image Observers
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-19 17:40:32 PDT
Created attachment 607396 [details] [diff] [review]
Part 2: Push onload blocking logic down into Imagelib.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-19 17:43:55 PDT
Created attachment 607398 [details] [diff] [review]
Part 3: Make style images block onload
Comment 12 Joe Drew (not getting mail) 2012-03-19 18:40:13 PDT
omigodomigodomigod
Comment 13 Joe Drew (not getting mail) 2012-03-20 07:31:22 PDT
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?
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-03-20 07:49:37 PDT
(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?
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-04-06 08:18:39 PDT
Created attachment 612889 [details] [diff] [review]
Part 1: Centralize image observers

Unbitrotted.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-04-06 08:19:41 PDT
Created attachment 612891 [details] [diff] [review]
Part 2: Push onload blocking logic down into imagelib

Unbitrotted.
Comment 17 Boris Zbarsky [:bz] 2012-04-06 20:08:15 PDT
Comment on attachment 612889 [details] [diff] [review]
Part 1: Centralize image observers

r=me with the nits on IRC.
Comment 18 Boris Zbarsky [:bz] 2012-04-06 20:14:49 PDT
Comment on attachment 612891 [details] [diff] [review]
Part 2: Push onload blocking logic down into imagelib

sr=me
Comment 19 Boris Zbarsky [:bz] 2012-04-06 20:15:23 PDT
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.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-04-07 15:18:28 PDT
I tried to land this but it turned Android reftests very orange :-(
Comment 21 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-04-24 14:40:00 PDT
Created attachment 618049 [details] [diff] [review]
Part 0: Make table frame classes call into their superclass in DidSetStyleContext
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-04-24 14:40:44 PDT
Created attachment 618051 [details] [diff] [review]
Part 1: Centralize image observers
Comment 23 Boris Zbarsky [:bz] 2012-04-24 20:09:18 PDT
Comment on attachment 618049 [details] [diff] [review]
Part 0: Make table frame classes call into their superclass in DidSetStyleContext

r=me

Note You need to log in before you can comment on or make changes to this bug.