Closed Bug 566736 Opened 10 years ago Closed 10 years ago

Lazily initialize the find toolbar

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a5

People

(Reporter: mano, Assigned: mano)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [ts])

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #446082 - Flags: review?(dietrich)
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: [ts]
OS: Mac OS X → All
Hardware: x86 → All
perf key word
Attached patch v2 (obsolete) — Splinter Review
Attachment #446089 - Flags: review?(gavin.sharp)
Attachment #446082 - Attachment is obsolete: true
Attachment #446082 - Flags: review?(dietrich)
Comment on attachment 446089 [details] [diff] [review]
v2

Looks like this will break http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#391 . I guess you should fix it to just use browserBottomBox.insertBefore as you do for the findbar. It would be nice to ensure there's a test that would catch that bustage, too.

>diff -r 90d627f2471e browser/base/content/browser.js

>+var gFindBarInitialized = false;
>+window.__defineGetter__("gFindBar", function() {

XPCOMUtils.defineLazyGetter(window, "gFindBar", function(){}) would be slightly simpler (don't need to delete yourself).

>+    let openerFindBarInitialized = window.opener.gFindBarInitialized;
>+    if (openerFindBarInitialized) {
>+      let openerFindBar = window.opener.gFindBar;
>+      if (!openerFindBar.hidden &&
>+          openerFindBar.findMode == openerFindBar.FIND_NORMAL) {
>+        gFindBar.open();
>+      }
>+    }

I think this is a bit easier to read as:

let openerFindbar = opener.gFindBarInitialized && opener.gFindBar;
(or openerFindbar = opener.gFindBarInitialized ? opener.gFindBar : null)
if (openerFindbar && !openerFindbar.hidden &&
    openerFindbar.findMode == openerFindbar.FIND_NORMAL)

r=me with those addressed.
Attachment #446089 - Flags: review?(gavin.sharp) → review+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #446089 - Attachment is obsolete: true
a111c9f6a71b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Backed out due to a11y test failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The first part of comment 3 also wasn't addressed.
Attached patch For checkin - another attempt (obsolete) — Splinter Review
I filed bug 566849 for fixing view-source the right way.
Attachment #446097 - Attachment is obsolete: true
6fc5d661ca55
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Ah. Well, then, whatever it's trying to do is clearly failing on Windows, because that test failed on both the opt and debug test runs.
I backed this patch out to make the tree green again.

http://hg.mozilla.org/mozilla-central/rev/855b42fbc47e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry everyone, and thanks. On that now.
Watching closely.

046c2d2acd3b
Attachment #446209 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
TBPL results showing 2% win for linux, 4% win for mac 32, 1% regression for mac 64, and no results yet for windows. Mano, please check mac 64 ts on the graph server of the next day to see if it's noise or not.
Ts, Cold MED Dirty Profile decrease 2.70% on XP Firefox
Depends on: 567306
Depends on: 567309
This makes TAF well-nigh unusable.  See bug 567309.
Blocks: 97023
Target Milestone: --- → Firefox 3.7a5
Version: unspecified → Trunk
Depends on: 591639
Depends on: 619297
You need to log in before you can comment on or make changes to this bug.