Tile cleanup #84

mrobinson · 2014-07-16T20:10:52Z

Tiles textures are maintained inside the TileGrid alongside buffers and Servo is no longer involved in the creation of Textures.


        Remove the Tile trait

It's unclear whether LayerBuffers will always represent the contents of one Tile and there's only one implementation of this trait.

zwarich · 2014-07-16T20:44:01Z

texturegl.rs

-            id: 0,
-            target: TextureTarget2D,
-            weak: true,
+id: 0,


Bad indentation here.

zwarich · 2014-07-16T20:44:09Z

texturegl.rs

-            id: *gl2::gen_textures(1).get(0),
-            target: target,
-            weak: false,
+id: *gl2::gen_textures(1).get(0),


Bad indentation here.

zwarich · 2014-07-16T20:44:45Z

texturegl.rs

+id: 0,
+        target: TextureTarget2D,
+        weak: true,
+        flip: NoFlip,
        }


The fields should be indented from the constructor.

zwarich · 2014-07-16T20:44:53Z

texturegl.rs

+id: *gl2::gen_textures(1).get(0),
+        target: target,
+        weak: false,
+        flip: NoFlip,
        };


The fields should be indented from the constructor.

zwarich · 2014-07-16T20:49:18Z

tiling.rs

+
+pub struct Tile {
+    /// The buffer displayed by this tile.
+    buffer: Option<Box<LayerBuffer>>,


When does buffer ever take None?

In the next patch set the Tile will be created with a None buffer after the first buffer request and before it returns.

I see. When we double-buffer, the Tile will have to hold on to both the front and back buffers itself, even when they are being rendered into by the render task.

Sounds reasonable, especially when the TileGrid is creating its own buffers.

zwarich · 2014-07-16T20:51:36Z

tiling.rs


    // The size of tiles in this grid in device pixels.
    tile_size: uint,

    // Tiles that are currently unused or outside the last-known visible rectangle.
-    unused_tiles: Vec<Box<LayerBuffer>>,
+    unused_buffers: Vec<Box<LayerBuffer>>,


You probably want to change the comment here to refer to buffers rather than tiles, and get rid of the 'last-known visible rectangle' part, since buffers are now independent of tiles.

zwarich · 2014-07-16T20:55:15Z

tiling.rs

@@ -77,14 +137,14 @@ impl TileGrid {

        for tile_index in tile_indexes_to_take.iter() {
            match self.tiles.pop(tile_index) {
-                Some(tile) => self.unused_tiles.push(tile),
+                Some(ref mut tile) => self.add_unused_buffer(tile.buffer.take()),


If the tiles field of TileGrid is a HashMap<Point2D<uint>, Tile>, why does this need to be ref mut tile? Can't you just have a Some(tile)?

I believe this was necessary because tile.buffer.take() requires a mutable borrow of tile, since buffer.take() mutates buffer.

Since pop actually transfers ownership, I think it would make more sense to have this be a Some(tile) and then use an &mut tile for the call. I think this better reflects what's going on.

I get this, but perhaps I'm doing it incorrectly:

src/support/layers/rust-layers/tiling.rs:140:60: 140:64 error: cannot borrow immutable local variable `tile` as mutable src/support/layers/rust-layers/tiling.rs:140 Some(tile) => self.add_unused_buffer((&mut tile).buffer.take()),

Oops, you would need a Some(mut tile), which makes it uglier. I guess the current thing in your patch is best.

zwarich · 2014-07-16T21:00:26Z

tiling.rs

+    buffer: Option<Box<LayerBuffer>>,
+
+    /// A handle to the GPU texture.
+    pub texture: Texture,


When buffer is None what is texture? Should it have a type that reflects that it is sometimes empty?

Tile has a builtin None instance provided by the Zero trait. I think that's the best thing to use since it already exists.


        TileGrid keeps a grid of Tiles instead of Buffers

This will allow the TileGrid to better track the state of each Tile.


        Move texture creation to Layer

This allows Layers to create their own textures instead of relying on Servo to do the job. Textures now track whether or not they should be flipped by passing the buffer to the factory method.


        Move Texture creation to the TileGrid

This way each Tile can be responsible for creating its own Textures.


        Merge pull request #84 from mrobinson/tile-cleanup

Tile cleanup

Remove the Tile trait

237a7e6

It's unclear whether LayerBuffers will always represent the contents of one Tile and there's only one implementation of this trait.