The default bug view has changed. See this FAQ.

Make style image decode block onload

RESOLVED FIXED in mozilla17

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

238.27 KB, patch
Details | Diff | Splinter Review
39.10 KB, patch
khuey
: review+
Details | Diff | Splinter Review
4.77 KB, patch
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.
Created attachment 570721 [details] [diff] [review]
Testcase
I'm going to do this.
Assignee: nobody → khuey
Blocks: 685516
Depends on: 714752
Created attachment 585726 [details] [diff] [review]
Part 1: Centralize image observers
Attachment #585726 - Flags: review?(dbaron)
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
Blocks: 432391
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.
The dependency is actually not very deep at all.
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-
TTo be clear, I think the general lifetime management in that patch is broken.....
Created attachment 607395 [details] [diff] [review]
Part 1: Centralize Image Observers
Attachment #570721 - Attachment is obsolete: true
Attachment #585726 - Attachment is obsolete: true
Attachment #607395 - Flags: review?(bzbarsky)
Created attachment 607396 [details] [diff] [review]
Part 2: Push onload blocking logic down into Imagelib.
Attachment #607396 - Flags: superreview?(bzbarsky)
Attachment #607396 - Flags: review?
Attachment #607396 - Flags: review? → review?(joe)
Created attachment 607398 [details] [diff] [review]
Part 3: Make style images block onload
Attachment #607398 - Flags: review?(bzbarsky)
omigodomigodomigod
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+
(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?
Created attachment 612889 [details] [diff] [review]
Part 1: Centralize image observers

Unbitrotted.
Attachment #607395 - Attachment is obsolete: true
Attachment #607395 - Flags: review?(bzbarsky)
Attachment #612889 - Flags: review?(bzbarsky)
Created attachment 612891 [details] [diff] [review]
Part 2: Push onload blocking logic down into imagelib

Unbitrotted.
Attachment #607396 - Attachment is obsolete: true
Attachment #607396 - Flags: superreview?(bzbarsky)
Attachment #612891 - Flags: superreview?(bzbarsky)
Attachment #612891 - Flags: review+
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 on attachment 612891 [details] [diff] [review]
Part 2: Push onload blocking logic down into imagelib

sr=me
Attachment #612891 - Flags: superreview?(bzbarsky) → superreview+
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+
I tried to land this but it turned Android reftests very orange :-(
Created attachment 618049 [details] [diff] [review]
Part 0: Make table frame classes call into their superclass in DidSetStyleContext
Attachment #618049 - Flags: review?(bzbarsky)
Created attachment 618051 [details] [diff] [review]
Part 1: Centralize image observers
Attachment #612889 - Attachment is obsolete: true
Attachment #618051 - Flags: review+
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+

Updated

5 years ago
Blocks: 688897

Updated

5 years ago
No longer blocks: 688897
Blocks: 688897
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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 783015

Updated

5 years ago
Depends on: 783220
Depends on: 783355
Depends on: 783162

Updated

5 years ago
Depends on: 784068
Depends on: 783126
Depends on: 783906

Updated

5 years ago
Depends on: 789846

Updated

5 years ago
Depends on: 791571

Updated

4 years ago
Depends on: 816498

Updated

4 years ago
Depends on: 818388

Updated

4 years ago
Depends on: 819922
Depends on: 838513

Updated

4 years ago
Depends on: 843308
Depends on: 874763

Updated

2 years ago
Depends on: 1170799
Blocks: 1227377
You need to log in before you can comment on or make changes to this bug.