Closed Bug 781053 Opened 13 years ago Closed 13 years ago

Animate the throbber using empty transactions

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Details

Attachments

(3 files, 7 obsolete files)

Part 3 - Force the throbber into it's own layer v4
2.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 1 - Allow conversion of nsImageBoxFrame to an ImageLayer v3
13.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2 - Trigger empty transactions when an animated image in an ImageLayer changes frame v3
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.

Attachment

General

Creator:
Created:
Updated:
Size: