Status

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(fennec1.0+)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
Makes sure prefetcher starts up whenever dirty tiles are created or when viewport size changes.
Depends on: 538571
Blocks: 538669
Attachment #421112 - Flags: review?(pavlov)
Attachment #421112 - Flags: review?(21)
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.
> 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.
(and by first, I mean the first paint since the last endCriticalMove)
tracking-fennec: --- → 1.0+
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 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.
Attachment #421112 - Flags: review?(21) → review+
Attachment #421112 - Flags: review?(rfrostig)
Comment on attachment 421112 [details] [diff] [review]
Patch

r+ with above comments.
Attachment #421112 - Flags: review?(rfrostig) → review+
Attachment #421112 - Flags: review?(pavlov) → review+
Pushed http://hg.mozilla.org/mobile-browser/rev/a81494dcc9be
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
bugspam
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.