Stacks should generate thumbnails for non-front tabs at a lower priority

RESOLVED FIXED in Firefox 4.0b12

Status

defect
P2
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Unfocused, Assigned: seanedunn)

Tracking

({perf})

Trunk
Firefox 4.0b12
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [qa-][cleanup])

Attachments

(1 attachment, 12 obsolete attachments)

At the moment, stacks have thumbnails for all tabs, regardless of whether you can actually see them or not. To help with performance issues, only the frontmost in the stack should have a thumbnail. The rest of the stack can either have blank thumbnails, or (ideally) just a static image that makes it look like there are many tabs behind it.
Note that this still applies even if bug 597258 gets fixed.
Assignee: nobody → seanedunn
Keywords: perf
Priority: -- → P3

Comment 2

9 years ago
Part of the performance meta project.
Priority: P3 → P2
Target Milestone: --- → Firefox 4.0b8

Updated

9 years ago
Blocks: 598394

Updated

9 years ago
Blocks: 597043
We may still want to generate the thumbnails (so they'll be immediately available if/when the user expands the stack), but prioritize them below all visible thumbnails.

Comment 4

9 years ago
Another option is to chunk up the stack into a canvas as an optimization. This way not only do you not have to keep all of those obscured tabs updating, but dragging a stack will get much faster as only one object is actually being dragged.
(Assignee)

Comment 5

9 years ago
I'm going to solve this bug by putting tabs in a stack on the end of the work queue, and we'll leave groupItem imposter images (bug 604120) as a separate task.

Updated

9 years ago
Blocks: 604213

Updated

9 years ago
No longer blocks: 598394
(Assignee)

Updated

9 years ago
Depends on: 587231
(Assignee)

Comment 6

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Solved by splitting tab updates into two classes: those that are hidden, and those visible in an arrangement, or at the top of stacks. Hidden tabs get pushed onto the back of the queue, while visible tabs are pushed to the front, and get priority.
Attachment #483099 - Flags: feedback?(ian)
Comment on attachment 483099 [details] [diff] [review]
v1

>   inStack: function TabItem_inStack() {
>     return iQ(this.container).hasClass("stacked");
>   },

I know this isn't part of your patch, but in terms of performance (and correctness) we shouldn't be talking to the DOM any more than we have to. This should instead be: 

  return this.parent && this.parent.isStacked(); 
  
This implies adding an isStacked method to return the "private" _isStacked property of GroupItem.

>+  // Returns true if this item is showing on top of the stack,
>+  // determined by whether this tab is its parents topChild, or
>+  // if it doesn't have one, its first child.
>+  topOfStack: function TabItem_topOfStack() {
>+    return this.inStack() && ((this.parent.topChild == this) || 
>+        (!this.parent.topChild && this.parent.getChild(0) == this));
>+  },

In terms of encapsulation, seems like the TabItem should ask the GroupItem whether it's the top child of a stack; that way we don't have to remember to change TabItem if GroupItem's logic changes. Not a big deal, I suppose, for something as simple as this, but still worth considering. 

>-          this._tabsWaitingForUpdate.push(tab);
>+          // Place non-stacked, or top-of-stack tabitems at the head of the 
>+          // work queue. Everything else goes to the end.
>+          if (!tab.tabItem.inStack() || tab.tabItem.topOfStack())
>+            this._tabsWaitingForUpdate.unshift(tab);
>+          else
>+            this._tabsWaitingForUpdate.push(tab);

I'm concerned that this changes a part of our render queue to being LIFO, which would mean if we have enough things clammoring to be updated, the ones at the bottom of the high-priority stack won't get serviced. 

Probably a better approach would be to have two FIFO queues.
Attachment #483099 - Flags: feedback?(ian) → feedback-
blocking2.0: --- → ?
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 years ago
Posted patch v2 (obsolete) — Splinter Review
I created a new object for abstracting a two-queue update solution (TabPriorityQueue), as well as the issues you brought up.
Attachment #483099 - Attachment is obsolete: true
Attachment #489567 - Flags: feedback?(ian)
Comment on attachment 489567 [details] [diff] [review]
v2

I like the new queue object; seems like we're getting there. There are still some issues, perhaps from miscommunication on my part.

>   update: function TabItems_update(tab) {
>     try {
>       Utils.assertThrow(tab, "tab");
>       Utils.assertThrow(!tab.pinned, "shouldn't be an app tab");
>       Utils.assertThrow(tab.tabItem, "should already be linked");
> 
>       let shouldDefer = (
>         this.isPaintingPaused() ||
>-        this._tabsWaitingForUpdate.length ||
>+        this._tabsWaitingForUpdate.hasItems() ||
>         Date.now() - this._lastUpdateTime < this._heartbeatTiming
>       );
> 
>       let isCurrentTab = (
>         !UI._isTabViewVisible() &&
>         tab == gBrowser.selectedTab
>       );
> 
>       if (shouldDefer && !isCurrentTab) {
>-        if (this._tabsWaitingForUpdate.indexOf(tab) == -1)
>-          this._tabsWaitingForUpdate.push(tab);
>-        this.startHeartbeat();
>+        this._tabsWaitingForUpdate.push(tab);
>+        this.heartbeat();
>       } else
>         this._update(tab);
>     } catch(e) {
>       Utils.log(e);
>     }
>   },

This call to heartbeat has two issues: 

* Heartbeat actually does an update, so the whole point of having an update queue (disconnecting update events from update processing so if we get a big spike of events we don't try to process them all at the same time) is subverted.

* Heartbeat doesn't do anything if _heartbeatOn is false, nor does it set it to true, so this doesn't kick off the heartbeat if needed. 

I think you still need something like startHeartbeat for this.

>   _update: function TabItems__update(tab) {
>     try {
>       Utils.assertThrow(tab, "tab");
> 
>-      // ___ remove from waiting list if needed
>-      let index = this._tabsWaitingForUpdate.indexOf(tab);
>-      if (index != -1)
>-        this._tabsWaitingForUpdate.splice(index, 1);

We removed tabs from the update queue here not only for the heartbeat, but because it's possible for _update to be called directly from update, and it's possible the tab in question may already be in the queue. Removing it here saves us from a double update.

I'd recommend putting the remove back in here (or, I suppose, next to the call to _update in update, if we're confident _update won't be called from anywhere else).
>+    // _update will remove the tab from the waiting list
>+    let item = this._tabsWaitingForUpdate.pop();

This comment does not speak the truth (though perhaps this will change as per above).

>+  push: function TabPriorityQueue_push(tab) {
>+    let item = tab.tabItem;
>+    if (item.parent && (!item.parent.isStacked() || 
>+      item.parent.isTopOfStack(item))) {
>+      if (this._high.indexOf(tab) == -1)
>+        this._high.unshift(tab);
>+    } else {
>+      if (this._low.indexOf(tab) == -1)
>+        this._low.unshift(tab);
>+    }
>+  },

The priority logic is wrong here. I believe we want everything to go into high unless it's not the top item of a stack, right? It seems like it should be something like so:

  let isLowPriority = (item.parent && item.parent.isStacked() && !item.parent.expanded);
  
Also, this routine doesn't handle the case where a tab is already in one of the queues but now needs to shift to another (for instance when a group becomes stacked or unstacked). 

>+  pop: function TabPriorityQueue_pop() {
>+    let ret = null;
>+    let src = this._high;
>+    let alt = this._low;
>+    if (!this._popToggle) {
>+      src = this._low;
>+      alt = this._high;
>+    }
>+    if (src.length)
>+      ret = src.pop();
>+    else if (alt.length)
>+      ret = alt.pop();
>+    this._popToggle = !this._popToggle;
>+    return ret;
>+  },

We don't want _popToggle... we want everything in the high-priority queue to be processed before anything in the low-priority queue. Since all we're putting in the low-priority queue are tabs that are obscured, it's fine if they never get processed at all.
Attachment #489567 - Flags: feedback?(ian) → feedback-
(Assignee)

Comment 10

9 years ago
Posted patch v3 (obsolete) — Splinter Review
The last patch I attached confused things a bit because I don't think I properly applied dependency 587231 first. So, that should resolve the issues with heartbeat().

Low priority now follows 
      item.parent && (item.parent.isStacked() && 
      !item.parent.isTopOfStack(item) &&
      !item.parent.expanded
everything else goes to top, and tabs now switch queues if required. If they already exist in the destination queue, they stay where they are.

I had put in popToggle because I thought I had remembered that you didn't want any way for the high priority queue to keep low priority items from updating. I changed this back to a simple high, then low order. I also put in a peek(), so that the next item on the queue can be sent to _update, and removed there.
Attachment #489567 - Attachment is obsolete: true
Attachment #490185 - Flags: feedback?(ian)
Comment on attachment 490185 [details] [diff] [review]
v3

(In reply to comment #10)
> The last patch I attached confused things a bit because I don't think I
> properly applied dependency 587231 first. So, that should resolve the issues
> with heartbeat().

Much better!

> Low priority now follows 
>       item.parent && (item.parent.isStacked() && 
>       !item.parent.isTopOfStack(item) &&
>       !item.parent.expanded

Perfect... I forgot isTopOfStack, didn't I?

> everything else goes to top, and tabs now switch queues if required. If they
> already exist in the destination queue, they stay where they are.

Excellent.

> I had put in popToggle because I thought I had remembered that you didn't want
> any way for the high priority queue to keep low priority items from updating. I
> changed this back to a simple high, then low order. 

What I was concerned about was things in the high-priority queue never getting processed if it was LIFO. Sorry about the confusion. Looks great, though you still have the popToggle property... might as well kill that too. 

> I also put in a peek(), so
> that the next item on the queue can be sent to _update, and removed there.

Cool, thanks.

Also, I'm now the Panorama module owner, so I can officially review! :)
Attachment #490185 - Flags: feedback?(ian) → review+
So, this patch needs approval and try, but otherwise should be good to go.
(Assignee)

Comment 13

9 years ago
Posted patch v3 (obsolete) — Splinter Review
Unbitrotted
Try: http://hg.mozilla.org/try/pushloghtml?changeset=734aeba49d07
Attachment #490185 - Attachment is obsolete: true
(In reply to comment #13)
> Created attachment 490278 [details] [diff] [review]
> v3
> 
> Unbitrotted
> Try: http://hg.mozilla.org/try/pushloghtml?changeset=734aeba49d07

Was the try successful?  I can't tell from that link.
Attachment #490278 - Flags: approval2.0?
(Assignee)

Comment 15

9 years ago
Try failed, due to 587231 test fail. Fixing now.
blocking2.0: ? → final+

Comment 16

9 years ago
Comment on attachment 490278 [details] [diff] [review]
v3

Canceling approval request, as this is now a blocker
Attachment #490278 - Flags: approval2.0?

Updated

9 years ago
Blocks: 598154

Comment 17

9 years ago
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
Not common enough perf to block.
blocking2.0: final+ → -

Comment 19

8 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 20

8 years ago
bugspam (removing b9)
No longer blocks: 598154
I'm confused... this is R+ but we never requested approval because of the temporary blocker-ness confusion?
(In reply to comment #21)
> I'm confused... this is R+ but we never requested approval because of the
> temporary blocker-ness confusion?

Probably, plus the fact that there was a failing test (comment 15). Needs to be unrotted probably, and run through try again. Sounds like mitcho's on that.
Posted patch v3, unrotted (obsolete) — Splinter Review
There was some minor patch rot. Unrotted, and passed all tests locally. Pushed to try.

Rerequesting approval.
Attachment #490278 - Attachment is obsolete: true
Attachment #505619 - Flags: approval2.0?
Whiteboard: [qa-][cleanup]
Comment on attachment 505619 [details] [diff] [review]
v3, unrotted

Failed try:

Just one failure, consistent across all platforms:

browser_tabview_bug595560.js | The search result tab is shown - Got [object XULElement @ 0x7ddd790 (native @ 0x9f4b030)], expected [object XULElement @ 0x70fe760 (native @ 0x8ec1760)]
Attachment #505619 - Flags: approval2.0?

Comment 25

8 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 26

8 years ago
bugspam. Removing b10
No longer blocks: 608028
Pushed to try.

I believe the error in that last try push was due to another bug on trunk... pushing again to check.
Attachment #505619 - Attachment is obsolete: true
Comment on attachment 510158 [details] [diff] [review]
Patch v3, unrotted again february 6

Passed try modulo one known intermittent orange: http://tbpl.mozilla.org/?tree=MozillaTry&rev=312fd3d96ec5

Requesting approval
Attachment #510158 - Flags: approval2.0?
Target Milestone: Firefox 4.0b8 → ---
Comment on attachment 510158 [details] [diff] [review]
Patch v3, unrotted again february 6

This is pretty big and doesn't have any tests (not sure if it really is testable either...).  I'm inclined to punt on this.
Attachment #510158 - Flags: approval2.0? → approval2.0-
Sean, could you put a quick test together for this? If not, I could try to.

Shawn, this patch could have some minimal perf win and also gives us some nice infrastructure to use in bug 627237 (not blocking, but a recommendation from our security review). Do you think we can get a+ on this if we put a test together in the next day or two?
(In reply to comment #30)
> Shawn, this patch could have some minimal perf win and also gives us some nice
> infrastructure to use in bug 627237 (not blocking, but a recommendation from
> our security review). Do you think we can get a+ on this if we put a test
> together in the next day or two?
I would feel much more comfortable doing so with a test.
Sean, I started looking into writing a test for this, but then realized (searching for references to isTopOfStack) that it looks like this patch right now makes non-top-of-stack tabs have a lower priority in the update queue, but not actually not updating them at all. This doesn't match the description of this bug. Am I missing something in the code and misreading?
(Assignee)

Comment 33

8 years ago
Tabs which are in a stack, but not the top, get put into the low priority queue. When update() is called on them, their state may change, and their priority may change. Tabs in the low priority queue will be updated (via calling peek() to find the next one) once the high priority queue tabs are exhausted.
(In reply to comment #33)
> Tabs which are in a stack, but not the top, get put into the low priority
> queue. When update() is called on them, their state may change, and their
> priority may change. Tabs in the low priority queue will be updated (via
> calling peek() to find the next one) once the high priority queue tabs are
> exhausted.

Yes, this was roughly my understanding... in other words, the description here "Stacks should not generate thumbnails for anything but the front-most tab" is not actually what this patch does. I just want to confirm that. Am I missing something?
(Assignee)

Comment 35

8 years ago
Yes, the spec for the bug changed with Ian's comment #3. Title changed.
Summary: Stacks should not generate thumbnails for anything but the front-most tab → Stacks should generate thumbnails for non-front tabs at a lower priority
Sean, could you write a test for this?
(Assignee)

Comment 37

8 years ago
Ok. The test will ensure that rear tabs get updated, but only after front tabs.
(Assignee)

Updated

8 years ago
Blocks: 604699
(Assignee)

Updated

8 years ago
No longer blocks: 604699
(Assignee)

Comment 38

8 years ago
Posted patch v4 (test added 2010/2/9) (obsolete) — Splinter Review
Added test, as well as a small test support member to TabItem. It lets tests hook in a function to be called whenever a tab item updates.
Attachment #510158 - Attachment is obsolete: true
Attachment #511255 - Flags: feedback?(mitcho)
(Assignee)

Comment 39

8 years ago
Attachment #511255 - Attachment is obsolete: true
Attachment #511256 - Flags: feedback?(mitcho)
Attachment #511255 - Flags: feedback?(mitcho)
Comment on attachment 511256 [details] [diff] [review]
v5 (test added 2010/2/9, debug line removed)

(Tim: you are flagged for feedback to take a quick look at createGroupItemWithTabs. Thanks. :))

>+  // ___ testing support. Only tests may set this to a function to be called
>+  // when the tab is updated. It's returned to null after calling.
>+  this._updateTestFunc = null;

Not a huge fan of this kind of hook, myself, but I guess it's legit... it's for Ian to decide, ultimately, though.

>       Utils.assertThrow(tab._tabViewTabItem, "should already be linked");
>-
>+      

You added space here. ^^^

>+      // ___ testing support
>+      if (tabItem._updateTestFunc) {
>+        tabItem._updateTestFunc(tabItem);
>+        tabItem._updateTestFunc = null;
>+      }

I think, for this to be a (potentially) more useful hook for future tests, we let the test unregister the updateTestFunc itself, rather than null-ing it out here.

>+++ b/browser/base/content/test/tabview/browser_tabview_bug602432.js
>@@ -0,0 +1,140 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please use the new public domain header: bug 629514

>  let groupItem = createGroupItemWithTabs(contentWindow, 150, 150, 100, 
>    numTabs, true);

Why are you calling createGroupItemWithTabs with animate = true? Especially important because you call mainTestFunc right after that, without executeSoon. Try it with animate = false.

>  // Define main test function
>  //
>  let mainTestFunc = function() {

There's no point in encapsulating this as a function if you're calling it right after, right?

>+    // Force updates to be deferred
>+    contentWindow.TabItems.pausePainting();

So, a number of operations like setBounds or zoomIn will resumePainting. Is this safe here? I.e., is it going to be reliably paused throughout the test duration?


>+    let testFunc = function(tabItem) {
>+      if(--leftToUpdate==0)

nit: if > 0, return, instead of tab drift. { can go after the control statements, too (iirc).

Nice test logic.

>+    for(let c=0; c<children.length; ++c)
>+    {
>+      children[c]._updateTestFunc = testFunc; 
>+      contentWindow.TabItems.update(children[c].tab);
>+    }    

Right now in head.js's afterAllTabItemsUpdated we call update on each of them, essentially bypassing the TabPriorityQueue. Adding this kind of hook to check that they're all updated and then calling update() on them is much more graceful. Please file a followup to rewrite afterAllTabItemsUpdated in terms of this kind of TabPriorityQueue-obeying logic, and making sure all the tests still pass. (This bug should block bug 585689.)

>+function createGroupItemWithTabs(contentWindow, width, height, padding, numNewTabs, animate) {

>+    let newItem = contentWindow.gBrowser.loadOneTab("about:blank")._tabViewTabItem;

Sorry, does contentWindow have a gBrowser property here? Because it's chrome maybe? Just checking.

createGroupItemWithTabs is nice. Let's put this in head.js. Flagging Tim who's written a number of our head.js functions for feedback on this function.

>+function closeGroupItem(groupItem, callback) {
>+  groupItem.addSubscriber(groupItem, "groupHidden", function() {
>+    groupItem.removeSubscriber(groupItem, "groupHidden");
>+    groupItem.addSubscriber(groupItem, "close", function() {
>+      groupItem.removeSubscriber(groupItem, "close");
>+
>+      callback();
>+  
>+    });
>+    group.closeHidden();    

What is this group here? Shouldn't it be groupItem? Also, there's extra whitespace there.

Thanks!
Attachment #511256 - Flags: feedback?(tim.taubert)
Attachment #511256 - Flags: feedback?(mitcho)
Attachment #511256 - Flags: feedback-
(Assignee)

Updated

8 years ago
Blocks: 633096
(Assignee)

Comment 41

8 years ago
(In reply to comment #40)
> So, a number of operations like setBounds or zoomIn will resumePainting. Is
> this safe here? I.e., is it going to be reliably paused throughout the test
> duration?

pause/resumePainting are re-entrant.

> Please file a followup to rewrite afterAllTabItemsUpdated in terms of
> this kind of TabPriorityQueue-obeying logic, and making sure all the tests
> still pass. (This bug should block bug 585689.)

Bug 633096

> Sorry, does contentWindow have a gBrowser property here? Because it's chrome
> maybe? Just checking.

It may, but I've changed it to use win.gBrowser now to match the other tests.
Attachment #511256 - Attachment is obsolete: true
Attachment #511307 - Flags: feedback?(mitcho)
Attachment #511256 - Flags: feedback?(tim.taubert)
Comment on attachment 511307 [details] [diff] [review]
v6 (with test and corrections)

>+    // Set up tab priority queue
>+    this._tabsWaitingForUpdate = new TabPriorityQueue();
>+    

nit: space

>+        
>+      // ___ testing support

nit: space

>+      if (tabItem._updateTestFunc) {
>+        tabItem._updateTestFunc(tabItem);
>+      }

nit: drop the {}

>+++ b/browser/base/content/test/tabview/browser_tabview_bug602432.js
>+++ b/browser/base/content/test/tabview/head.js

nit: you've got (a little) more trailing whitespace in these files too.

f+ from me. Just make sure to make those changes in the next revision.

feedbacking Tim again for createGroupItemWithTabs, in case he notices anything.
Attachment #511307 - Flags: review?(ian)
Attachment #511307 - Flags: feedback?(tim.taubert)
Attachment #511307 - Flags: feedback?(mitcho) → feedback+
Comment on attachment 511307 [details] [diff] [review]
v6 (with test and corrections)

+function createGroupItemWithTabs(win, width, height, padding, numNewTabs, animate) {
+  let contentWindow = win.document.getElementById("tab-view").contentWindow;

Maybe we could use win.TabView.getContentWindow() here? That is a lot more flexible if we're changing how panorama is structured later.

+  // add blank items
+  contentWindow.GroupItems.setActiveGroupItem(groupItem);
+  for(let t=0; t<numNewTabs; t++) {

Nit: please add a space here: "for (let t..."

+  for(let t=0; t<numNewTabs; t++) {
+    let newItem = win.gBrowser.loadOneTab("about:blank")._tabViewTabItem;
+    ok(newItem.container, "Created element "+t+":"+newItem.container);
+  }

I could use that function in _many_ tests but we would definitely need some way to specify what urls we want to be loaded. So maybe instead of numNewTabs that could be an array with urls? We could then create a convenience function like createGroupItemWithBlankTabs() that accepts a "numNewTabs" argument?

Looks good overall, but I think we should make that bit more flexible (that's a very useful helper function :). And closeGroupItem() is a good catch, too!
Comment on attachment 511307 [details] [diff] [review]
v6 (with test and corrections)

Forgot to f+ :)
Attachment #511307 - Flags: feedback?(tim.taubert) → feedback+
(In reply to comment #43)
> And closeGroupItem() is a good catch, too!

I've actually wondered whether GroupItem_close should take an argument which means "right now, with no undo". It could be useful for tests, but also our future API. That's another bug, though. ;)
(In reply to comment #45)
> I've actually wondered whether GroupItem_close should take an argument which
> means "right now, with no undo". It could be useful for tests, but also our
> future API. That's another bug, though. ;)

+1 That would be great :)
(Assignee)

Comment 47

8 years ago
Posted patch v7 (w/ test and corrections) (obsolete) — Splinter Review
head.js now has a createGroupItemWithTabs( url array) and createGroupItemWithBlankTabs( number of tabs).

I fixed most of the spacing nits, except where it appeared that spacing was the style laid down in that block previously. Personally, I think that spacing improves readability.
Attachment #511307 - Attachment is obsolete: true
Attachment #511307 - Flags: review?(ian)
Regarding bug 633096 and this patch: wouldn't it be better to send a subscriber notification like "update" when a TabItem gets updated?

>+      // ___ testing support
>+      if (tabItem._updateTestFunc)
>+        tabItem._updateTestFunc(tabItem);

could be changed to:

tabItem._sendToSubscribers("update");

IMHO that's a bit clearer and would be nice to have as an API to attach to tabItem updates. What do you think? :)
(In reply to comment #47)
> I fixed most of the spacing nits, except where it appeared that spacing was the
> style laid down in that block previously. Personally, I think that spacing
> improves readability.

My spacing nits were *all* about trailing whitespace. It should not affect readability one bit.
(In reply to comment #48)
> >+        tabItem._updateTestFunc(tabItem);
> 
> could be changed to:
> 
> tabItem._sendToSubscribers("update");
> 
> IMHO that's a bit clearer and would be nice to have as an API to attach to
> tabItem updates. What do you think? :)

That is indeed the other option... let's have Ian weigh in.
(Assignee)

Comment 51

8 years ago
Posted patch v8 (more corrections) (obsolete) — Splinter Review
(In reply to comment #50)
> That is indeed the other option... let's have Ian weigh in.

Done. Far easier to just use the supported feature.

(In reply to comment #49)
> My spacing nits were *all* about trailing whitespace. It should not affect
> readability one bit.

Sorry, I mistook trailing whitespace for "extra line breaks".
Attachment #511315 - Attachment is obsolete: true
Attachment #511315 - Flags: review?(ian)
(Assignee)

Comment 52

8 years ago
Posted patch v9 (obsolete) — Splinter Review
(removed an errant _updateTestFunc initializer)
Attachment #511422 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #511423 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #511423 - Flags: approval2.0?
(Assignee)

Comment 54

8 years ago
Try passed.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Attachment #511423 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
This looks like a non-trivial change, which is a concern at this stage. How much of a perf or responsiveness win are we getting?

Does this effect perf of Panorama first-show time, or stack painting, or something else?
(Assignee)

Comment 56

8 years ago
From comment #30:
> Shawn, this patch could have some minimal perf win and also gives us some nice
> infrastructure to use in bug 627237 (not blocking, but a recommendation from
> our security review). Do you think we can get a+ on this if we put a test
> together in the next day or two?

So, no, the perf win is not large.
(In reply to comment #56)
> From comment #30:
> > Shawn, this patch could have some minimal perf win and also gives us some nice
> > infrastructure to use in bug 627237 (not blocking, but a recommendation from
> > our security review). Do you think we can get a+ on this if we put a test
> > together in the next day or two?
> 
> So, no, the perf win is not large.

Even if the perf win is not objectively large, it's something. And, more importantly, the user would subjectively think that it's larger, because the tabs that are actually visible would load first, would they not?

I would say that it'd be worth the risk of taking it late for the UX improvements it would afford. But that's just my two cents.
Comment on attachment 511423 [details] [diff] [review]
v9

After skimming the patch and checking with mitcho, I think this is ok to take.
Attachment #511423 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

8 years ago
Blocks: 632280
(Assignee)

Updated

8 years ago
No longer blocks: 632280
(Assignee)

Updated

8 years ago
Blocks: 628701
(Assignee)

Comment 59

8 years ago
Attachment #511423 - Attachment is obsolete: true
(In reply to comment #59)
> Created attachment 511990 [details] [diff] [review]
> v10: runrotted

Is there a reason you didn't call this "patch for checkin" and flagged it as such?
(Assignee)

Updated

8 years ago
Attachment #511990 - Attachment description: v10: runrotted → v10: patch for checkin
(Assignee)

Comment 61

8 years ago
Because I forgot. Labeled and flagged.
Keywords: checkin-needed

Comment 62

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6b7306e19327
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.