Closed Bug 625269 Opened 10 years ago Closed 10 years ago

Reduce some unnecessary resizing of Items when resizing the window


(Firefox Graveyard :: Panorama, defect, P2)



(Not tracked)

Firefox 4.0b10


(Reporter: mitcho, Assigned: mitcho)




(1 file, 3 obsolete files)

Attached patch Patch v1, which just does step 1 (obsolete) — Splinter Review
(This is a bug for a much simpler version of bug 601534, which we have punted on for fx4.)

Two ideas:

STEP 1: When we resize the screen, only run our "resize" code if either (a) we've actually run out of space, i.e. some items no longer fit, or (b) some items feel "cramped", i.e. their sizes are different than their userSize. Note this will solve bug 598216.

STEP 2: Tweak the actual "resize" code which makes items push and squish each other slightly, to minimize some unnecessary resizing.

A patch which simply does the first part is attached for consideration, for starters.
Attachment #503381 - Flags: feedback?(ian)
Assignee: nobody → mitcho
Comment on attachment 503381 [details] [diff] [review]
Patch v1, which just does step 1

Looks like a good direction. Comments: 

* Create a combined computeShouldResizeItems and clearShouldResizeItems as dicussed in IRC, re: "It seems a little odd that you're caching the results of computeMinimalRect but not the results of feelsCramped; don't they change from the same causes? Also, they both do very similar loops, so you could combine them and reduce your loop count. On the other hand, maybe there's no point in caching anyway, as it only optimizes for the resize calls that don't change any bounds -- the ones that are going to be fast anyways."

>+    // Here are reasons why we *won't* resize:
>+    // 1. Panorama isn't visible
>+    // 2. the screen dimensions haven't changed
>+    // 3. everything on the screen fits and nothing feels cramped

You might add a parenthetical to item 1 to say that we then attempt to resize next time Panorama is shown (otherwise we'd have a bit of a problem!).
Attachment #503381 - Flags: feedback?(ian) → feedback+
Punting "STEP 2" out of the scope of this bug, as per IRC discussion with Ian. Bug 625656 filed as followup.
Blocks: 625656
Blocks: 625668
Attached patch Patch v2 (obsolete) — Splinter Review
Again, as per the comment above, this is only "STEP 1".

1. We can't just keep a cache of _shouldResizeItems, because we still need to compare the minimalRect with the new screen bounds. What we can cache, however, are _minimalRect and _feelsCramped, and that's what I've done. I have, however, moved to the unified interface of computeShouldResizeItems and clearShouldResizeItems that we agreed upon.
2. The test includes two TODO lines which currently fail, due to the newly filed bug 625668.

All tabview mochitest pass locally. Sending to try.
Attachment #503381 - Attachment is obsolete: true
Comment on attachment 503759 [details] [diff] [review]
Patch v2

Whoops. I lied.
Attachment #503759 - Attachment description: Patch v1 → Patch v2
Attachment #503759 - Flags: review?(ian)
Summary: Reduce unnecessary resizing of Items when resizing the window → Reduce some unnecessary resizing of Items when resizing the window
Note two new bugs found during this work: bug 625666 and bug 625668.
Blocks: 608028
Passed try.
Comment on attachment 503759 [details] [diff] [review]
Patch v2

>+    if (!this.computeShouldResizeItems())
>+      return;

Seems like this routine should be called just shouldResizeItems(); when we're calling it, we don't really care whether it's computing or not, and in fact sometimes it isn't really (relying on cached values and what not). 

>+  computeShouldResizeItems: function UI_computeShouldResizeItems() {
>+    // First, compute a minimal rect for all the top-level items on the screen:
>+    // set rect with the minimum rect
>+    if (this._minimalRect === undefined) {
>+      let minimalRect = new Rect(0, 0, 1, 1);
>+      // ensure that the rect contains all the Items
>+      Items.getTopLevelItems()
>+        .forEach(function UI_computeShouldResizeItems_unionBounds(item) {
>+          let bounds = new Rect(item.getBounds());
>+          bounds.inset(-Trenches.defaultRadius, -Trenches.defaultRadius);
>+          minimalRect = minimalRect.union(bounds);
>+        });
>+      // ensure it extends to, but not beyond, the origin
>+      minimalRect.left = 0;
>+  = 0;
>+      this._minimalRect = minimalRect;
>+    }
>+    let newPageBounds = Items.getPageBounds();
>+    if (this._minimalRect.width > newPageBounds.width ||
>+        this._minimalRect.height > newPageBounds.height) {
>+      return true;
>+    }
>+    if (this._feelsCramped !== undefined)
>+      return this._feelsCramped;
>+    // Second, look at whether any top-level items feel "cramped" due to
>+    // squishing (a technical term).
>+    this._feelsCramped = Items.getTopLevelItems()
>+      .some(function UI_computeShouldResizeItems_checkCramped(item) {
>+        let bounds = item.getBounds();
>+        return item.userSize &&
>+          (item.userSize.x > bounds.width || item.userSize.y > bounds.height);
>+      });
>+    return this._feelsCramped;

Seems like it's worth combining the two loops; if you're going to go through all of the top level items getting their bounds while checking minimalRect, you might as well do the cramped check at the same time.

>+  currentGroup.setSize(600, 600, true);
>+	currentGroup.setUserSize();

There's a tab character on the above line.

>+  let down1 = function down1(resized) {
>+    ok(!resized, "A little smaller: the group should not have been resized.");
>+    checkResized(currentGroup, 50, 50, up1, contentWindow, win);
>+  };

This test structure (testing the restult of the previous test in the same routine as setting up the next test) is really confusing, and it's also brittle (depending as it does on the test order). Seems like you could fix up checkResized to take in the result you're expecting and the explanation string.

>+  let down2 = function down2(resized) {
>+    ok(resized, "Much smaller: the group should have been resized.");
>+    contentWindow.Utils.log(currentGroup.getBounds());
>+    contentWindow.Utils.log(currentGroup.userSize);

Kill these logs or turn them into info calls.

>+  let up2 = function up2(resized) {
>+		// TODO: bug 625668

More tab characters

>+  let down3 = function down3(resized) {
>+		// TODO: bug 625668

More tabs

>+    currentGroup.setSize(100,100,true);
>+		currentGroup.setUserSize();


>+	// start by making it a little smaller.

And another. :)

>+  // Use executeSoon because the resize code, if we do hit it, may take a
>+  // little bit of time.
>+  executeSoon(function() {

Do you really need this? If so, it's not because of how long the resize code may take; it would be because the resize event doesn't get distributed until we give control back to the browser. Do check to make sure you actually need it, though. 

>\ No newline at end of file

Attachment #503759 - Flags: review?(ian) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Thanks for your comments! Sent to try both with and without the executeSoon as I just wasn't sure about that one point.
Attachment #503759 - Attachment is obsolete: true
Attachment #504551 - Flags: review?(ian)
Comment on attachment 504551 [details] [diff] [review]
Patch v3

So, did you need the executeSoon? If not, please remove.

Otherwise looks good!
Attachment #504551 - Flags: review?(ian) → review+
> So, did you need the executeSoon? If not, please remove.

Most platforms' mochitests for some reason didn't show up in tbpl. Requested reruns: bug 627588.
Attachment #504551 - Flags: approval2.0?
Nevermind. The instance without the executeSoon also passed, so we'll use that.
Still waiting for approval first, though.
Can you explain what this actually does, and why we want to take it?
(In reply to comment #13)
> Can you explain what this actually does, and why we want to take it?

In trunk right now, if a window is resized, we try to push some of the groups around and resize them, even if the resizing doesn't actually infringe on the space of any of the top-level items. This patch fixes this. It greatly improves performance of window resizing while Panorama is open, respecting the user's layout choices as much as possible (and much more than before). As a consequence, it also should fix bug 598216.
What's the level of risk? Did you remove the executeSoon, and how'd the tryserver runs go?
(In reply to comment #15)
> What's the level of risk? Did you remove the executeSoon, and how'd the
> tryserver runs go?

I believe it's low risk. I've removed executeSoon (locally, haven't uploaded it here, but the checkin patch will have no executeSoon). Try without executeSoon passed, modulo known non-Panorama intermittent oranges:
Attachment #504551 - Flags: approval2.0? → approval2.0+
Thanks Dietrich! :)
Attachment #504551 - Attachment is obsolete: true
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
verified with nightly build of minefield. though seeing bug 625668 when expanding the view.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.