566736 - Lazily initialize the find toolbar

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Asaf Romano (gone)]

Attachment: patch

Comment 1

[Worcester12345]

perf key word

Comment 2

[Asaf Romano (gone)]

Attachment: v2

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[Asaf Romano (gone)]

Attachment: for checkin

Comment 5

[Asaf Romano (gone)]

a111c9f6a71b

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out due to a11y test failures.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

The first part of comment 3 also wasn't addressed.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

The test in question is:

http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/name_nsRootAcc_wnd.xul

Comment 9

[Asaf Romano (gone)]

Attachment: For checkin - another attempt

I filed bug 566849 for fixing view-source the right way.

Comment 10

[Asaf Romano (gone)]

6fc5d661ca55

Comment 11

[(not currently active) Ted Mielczarek]

I think this broke test_name_nsRootAcc.xul on Windows:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274273683.1274274700.8647.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274273351.1274275486.12162.gz

Probably because the test fiddles with gFindBar:
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/name_nsRootAcc_wnd.xul#35

Can you either fix the test or back out?

Comment 12

[(no longer active)]

(In reply to comment #11)
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/name_nsRootAcc_wnd.xul#35

Actually this line was landed as part of this bug:
http://hg.mozilla.org/mozilla-central/rev/6fc5d661ca55#l1.8

Comment 13

[(not currently active) Ted Mielczarek]

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.

Comment 14

[(no longer active)]

I backed this patch out to make the tree green again.

http://hg.mozilla.org/mozilla-central/rev/855b42fbc47e

Comment 15

[Asaf Romano (gone)]

Sorry everyone, and thanks. On that now.

Comment 16

[Asaf Romano (gone)]

Attachment: Last round for now.

Watching closely.

046c2d2acd3b

Comment 17

[Dietrich Ayala (:dietrich)]

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.

Comment 18

[Asaf Romano (gone)]

windows: -1%.

Comment 19

[Asaf Romano (gone)]

Ts, Cold MED Dirty Profile decrease 2.70% on XP Firefox

Comment 20

[Boris Zbarsky [:bzbarsky]]

This makes TAF well-nigh unusable.  See bug 567309.