[Webkit-unassigned] [Bug 78305] [Qt][Texmap] Refactor backing-store code in TextureMapper

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78305





--- Comment #3 from Noam Rosenthal <noam.rosenthal at nokia.com>  2012-02-10 09:14:27 PST ---
(In reply to comment #2)
> (From update of attachment 126428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126428&action=review
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:49
> > +    virtual int maxTextureDimension() { return 2000; }
> 
> Would it make sense to make it possible to use different dimentions horizontally, vertically?
It might look nice, but what is the use for that?

> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:304
> > +    m_compositedImage = TextureMapperCompositedImage::get(m_image.get(), textureMapper);
> > +    m_contentsLayer = m_media ? m_media : m_compositedImage.get();
> 
> Would it make sense to check for m_media first?
You mean before testing for m_compositedImage? that wouldn't work, because we need to reset the composited image if m_image is 0.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.h:109
> > +    virtual int maxTextureDimension() { return 10000; }
> 
> Why such a crazy big default? if you want to avoid tiling why not use MAX_INT or so.
For super-huge images I still want a bit of tiling.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:31
> > +void TextureMapperTile::updateContents(TextureMapper* textureMapper, Image* image, const IntRect& dirtyRect)
> > +{
> > +    IntRect targetRect = enclosingIntRect(m_rect);
> 
> Please notice that webkit just switched to floats for layouts. Maybe we soon need to change to using LayoutRects instead
Right.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:44
> > +        m_texture->reset(targetRect.size(), false);
> 
> specify what the false means
Sure, had a boolean trap there... I'll add a comment and make it into an enum in a different patch.

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:82
> > +PassRefPtr<TextureMapperBackingStore> TextureMapperNode::getBackingStore()
> 
> why not just backingStore() ?
Because it might create the backing-store; I like using the word get when the function can have side-effects.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:99
> > +    FloatRect dirtyRect = m_state.needsDisplay ? layerRect() : m_state.needsDisplayRect;
> 
> is this correct?
Yes. needsDisplay means we have to update the entire layerRect.

> 
> > Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:49
> > +#if PLATFORM(QT)
> > +    QImage qtImage = m_backBuffer->createQImage();
> > +    pixels = qtImage.constBits();
> > +#endif
> 
> You intent this file to move out of /qt/ ?
Yes, most of this is not Qt specific though we're the only ones to do this cross-process AC so far.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.