Closed
Bug 781053
Opened 12 years ago
Closed 12 years ago
Animate the throbber using empty transactions
Categories
(Core :: Layout, defect)
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 |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #649897 -
Attachment is obsolete: true
Attachment #663907 -
Flags: review?(roc)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #649887 -
Attachment is obsolete: true
Attachment #649888 -
Attachment is obsolete: true
Attachment #663908 -
Flags: review?(roc)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #663910 -
Flags: review?(roc)
Assignee | ||
Comment 8•12 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=54e4f2b81524
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 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
> ::: 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.
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #663907 -
Attachment is obsolete: true
Attachment #663907 -
Flags: review?(roc)
Attachment #663977 -
Flags: review?(roc)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #663908 -
Attachment is obsolete: true
Attachment #663908 -
Flags: review?(roc)
Attachment #663978 -
Flags: review?(roc)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #663910 -
Attachment is obsolete: true
Attachment #663910 -
Flags: review?(roc)
Attachment #663979 -
Flags: review?(roc)
Attachment #663978 -
Flags: review?(roc) → review+
Attachment #663979 -
Flags: review?(roc) → review+
Attachment #663977 -
Flags: review?(roc) → review+
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ad17b87332 https://hg.mozilla.org/integration/mozilla-inbound/rev/56fc051c1932 https://hg.mozilla.org/integration/mozilla-inbound/rev/85d6cbd01d39 Try server: https://tbpl.mozilla.org/?tree=Try&rev=b08ca921f7c9
Comment 18•12 years ago
|
||
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.
Comment 22•12 years ago
|
||
Backed out (see bug 539356 comment 337): https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f86e3a3240
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c0f9a43e9b09 https://hg.mozilla.org/mozilla-central/rev/ed784e42dfe9 https://hg.mozilla.org/mozilla-central/rev/921fe135bf2c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Assignee: nobody → matt.woodrow
You need to log in
before you can comment on or make changes to this bug.
Description
•