Closed Bug 781053 Opened 7 years ago Closed 7 years ago

Animate the throbber using empty transactions

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Details

Attachments

(3 files, 7 obsolete files)

2.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
There's a lot of duplicated code here, it might be worth refactoring nsImageBoxFrame to share an ancestor class with nsImageFrame.

Or at least do that with nsDisplayXULImage/nsDisplayImage.
Ugly, but apparently the xul 'layer' attribute is pretty broken at the moment.

This also causes the overlapping leaf layers mochitest to fail. I guess we could only add the 3d transform for the animated image and remove it when display the favicon.
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Ugly, but apparently the xul 'layer' attribute is pretty broken at the
> moment.

It is? We need to fix that!

> This also causes the overlapping leaf layers mochitest to fail. I guess we
> could only add the 3d transform for the animated image and remove it when
> display the favicon.

Yeah I think we should.
No longer fails test.

I also notice that my splitting of part 1/2 could do with some work.
Attachment #649890 - Attachment is obsolete: true
Attachment #649897 - Attachment is obsolete: true
Attachment #663907 - Flags: review?(roc)
Attachment #649887 - Attachment is obsolete: true
Attachment #649888 - Attachment is obsolete: true
Attachment #663908 - Flags: review?(roc)
Comment on attachment 663908 [details] [diff] [review]
Part 1 - Allow conversion of nsImageBoxFrame to an ImageLayer v2

Review of attachment 663908 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's probably worth creating a shared superclass for nsDisplayImage/nsDisplayXULImage container GetImageContainer/ConfigureLayer so that FrameLayerBuilder doesn't have to deal with both image classes.
Comment on attachment 663910 [details] [diff] [review]
Part 2 - Trigger empty transactions when an animated image in an ImageLayer changes frame v2

Review of attachment 663910 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.h
@@ +381,5 @@
>     * aFrame and that container layer.
>     */
>    static gfxSize GetThebesLayerScaleForFrame(nsIFrame* aFrame);
>  
> +  void StoreImageLayerForFrame(nsIFrame* aFrame, PRUint32 aDisplayItemKey, ImageLayer* aImage);

Document this

@@ +477,5 @@
>      DisplayItemData(Layer* aLayer, uint32_t aKey, LayerState aLayerState, uint32_t aGeneration);
>      ~DisplayItemData();
>  
>      nsRefPtr<Layer> mLayer;
> +    nsRefPtr<ImageLayer> mImageLayer;

Why can't we just use mLayer?

::: layout/generic/nsImageFrame.cpp
@@ +690,5 @@
> +  if (layer) {
> +    ImageLayer* image = static_cast<ImageLayer*>(layer);
> +    nsRefPtr<ImageContainer> container;
> +    nsresult rv = aContainer->GetImageContainer(getter_AddRefs(container));
> +    image->SetContainer(container);

Why do we need to do this? Can't we have animated images just reset the current image in their own ImageContainer?
Comment on attachment 663907 [details] [diff] [review]
Part 3 - Force the throbber into it's own layer v3

Review of attachment 663907 [details] [diff] [review]:
-----------------------------------------------------------------

What is wrong with the XUL "layer" attribute? Is is that it only works on XUL container elements? I think we could easily make it work for children of XUL container elements by hacking nsBoxFrame::BuildDisplayListForChildren.
Comment on attachment 663907 [details] [diff] [review]
Part 3 - Force the throbber into it's own layer v3

An attribute, as roc suggests, seems like a much better option than the rather non-obvious transform style. That said, if you do end up using a style like this, it should probably live in browser/base/content/browser.css (content style) rather than the individual files in the theme.
> ::: layout/base/FrameLayerBuilder.h
> @@ +381,5 @@
> >     * aFrame and that container layer.
> >     */
> >    static gfxSize GetThebesLayerScaleForFrame(nsIFrame* aFrame);
> >  
> > +  void StoreImageLayerForFrame(nsIFrame* aFrame, PRUint32 aDisplayItemKey, ImageLayer* aImage);
> 
> Document this

OK
> 
> @@ +477,5 @@
> >      DisplayItemData(Layer* aLayer, uint32_t aKey, LayerState aLayerState, uint32_t aGeneration);
> >      ~DisplayItemData();
> >  
> >      nsRefPtr<Layer> mLayer;
> > +    nsRefPtr<ImageLayer> mImageLayer;
> 
> Why can't we just use mLayer?

We already store the ThebesLayer there, in this case we have 2 layers for the given frame/key pair. I'll call it mOptLayer though, in case we ever decide we want to do this for the ColorLayer optimization too.

> 
> ::: layout/generic/nsImageFrame.cpp
> @@ +690,5 @@
> > +  if (layer) {
> > +    ImageLayer* image = static_cast<ImageLayer*>(layer);
> > +    nsRefPtr<ImageContainer> container;
> > +    nsresult rv = aContainer->GetImageContainer(getter_AddRefs(container));
> > +    image->SetContainer(container);
> 
> Why do we need to do this? Can't we have animated images just reset the
> current image in their own ImageContainer?

Sounds like it should work, I'll have a look at imglib.
Attachment #663907 - Attachment is obsolete: true
Attachment #663907 - Flags: review?(roc)
Attachment #663977 - Flags: review?(roc)
Attachment #663908 - Attachment is obsolete: true
Attachment #663908 - Flags: review?(roc)
Attachment #663978 - Flags: review?(roc)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1057422754e0 for the assertions and crashes like that one you got on 10.8 on your try push.
Why is this XUL only?  This would be a great optimization for cases like https://github.com/mozilla-b2g/gaia/issues/4542, which is the exact same problem in web content.  (Working around by using lower-framerate animation.)
The only thing that's XUL-only about it is the use of the "layer" attribute to force content into its own layer. Is that something we want to expose to Web content?
Oh, I saw

      aDisplayItemKey == nsDisplayItem::TYPE_XUL_IMAGE ||

but missed the

      aDisplayItemKey == nsDisplayItem::TYPE_IMAGE ||

just above it.  D'oh!  Sorry.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> The only thing that's XUL-only about it is the use of the "layer" attribute
> to force content into its own layer. Is that something we want to expose to
> Web content?

We already do this in effect with 3d transforms.  I don't think exposing a specific "layer" attribute is something we should do, though.
Assignee: nobody → matt.woodrow
You need to log in before you can comment on or make changes to this bug.