539062 - Less checkerboarding

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Patch

Makes sure prefetcher starts up whenever dirty tiles are created or when viewport size changes.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Globally the patch looks ok for me. One question thought.


>+  /** Crawler will recalculate the tiles it is supposed to fetch in the background. */
>+  recenterCrawler: function recenterCrawler() {
>+    let cr = this._criticalRect;
>+    this._crawler = new TileManager.CrawlIterator(this._tileCache, cr.clone());
>+  },
>+

Can we avoid to create a new object each time we recenter the crawler? It doesn't looks like a cheap operation.

Comment 2

[Benjamin Stover (:stechz)]

> Can we avoid to create a new object each time we recenter the crawler? It
> doesn't looks like a cheap operation.

The third patch does just that.

In this case it's not any worse than it was before--crawliterators are created whenever the first critical paint occurs.

Comment 3

[Benjamin Stover (:stechz)]

(and by first, I mean the first paint since the last endCriticalMove)

Comment 4

[Roy Frostig :froystig]

Comment on attachment 421112 [details] [diff] [review]
Patch

>-      if (!tile.boundRect.intersects(criticalRect))
>+      if (!tile.boundRect.intersects(criticalRect)) {
>+        // Dirty tile outside of viewport. Just remove and redraw later.
>         this._removeTileSafe(tile);
>-      else
>+        crawler.enqueue(tile.i, tile.j);
>+        outsideIsDirty = true;
>+      } else {
>         criticalIsDirty = true;
>+      }
> 
>       tile.markDirty(rects[i]);
> 
>-      if (crawler)
>-        crawler.enqueue(tile.i, tile.j);
>-
>       END_FOREACH_IN_RECT
>     }

Notice removed guard for |crawler| being null prior to |.enqueue|.  Can still be null.  Patch in "blocking" bug 538669 fixes this by initializing in the constructor, but as-is this is unsafe in this change alone, so perhaps add the null guard back here and remove in next patch.


>-      if (tile.isDirty()) {
>-        self._renderTile(tile);
>-      }
>       self._appendTileSafe(tile);
>+      self._renderTile(tile);
>     }

Bring back the render-before-append order as before, for consistency if anything.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 421112 [details] [diff] [review]
Patch

(In reply to comment #2)
> > Can we avoid to create a new object each time we recenter the crawler? It
> > doesn't looks like a cheap operation.
> 
> The third patch does just that.
> 
> In this case it's not any worse than it was before--crawliterators are created
> whenever the first critical paint occurs.

Right.

Comment 6

[Roy Frostig :froystig]

Comment on attachment 421112 [details] [diff] [review]
Patch

r+ with above comments.

Comment 7

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/a81494dcc9be

Comment 8

[Dão Gottwald [:dao]]

bugspam