697230 - Make style image decode block onload

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Testcase

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm going to do this.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Centralize image observers

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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-4, no longer working on Mozilla)]

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 (Exited; not receiving bugmail, old account, do not use)]

The dependency is actually not very deep at all.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

TTo be clear, I think the general lifetime management in that patch is broken.....

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Centralize Image Observers

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Push onload blocking logic down into Imagelib.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Make style images block onload

Comment 12

[Joe Drew (not getting mail)]

omigodomigodomigod

Comment 13

[Joe Drew (not getting mail)]

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 (Exited; not receiving bugmail, old account, do not use)]

(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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Centralize image observers

Unbitrotted.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Push onload blocking logic down into imagelib

Unbitrotted.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 612889 [details] [diff] [review]
Part 1: Centralize image observers

r=me with the nits on IRC.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 612891 [details] [diff] [review]
Part 2: Push onload blocking logic down into imagelib

sr=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

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 (Exited; not receiving bugmail, old account, do not use)]

I tried to land this but it turned Android reftests very orange :-(

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 0: Make table frame classes call into their superclass in DidSetStyleContext

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Centralize image observers

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 618049 [details] [diff] [review]
Part 0: Make table frame classes call into their superclass in DidSetStyleContext

r=me

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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