311965 - Refactor nsUpdateService.js to load less code at startup

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian Ryner (not reading)]

Compile time of JS components is a pretty big chunk of our startup time, and
there's no reason the full update service needs to initialize right away (except
perhaps in the case of a pending update).  If we're just going to do a ping or
background download, all of that code can be on a delayed load.

Comment 1

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

*** Bug 311966 has been marked as a duplicate of this bug. ***

Comment 2

[Darin Fisher]

It may also make sense to move the update timer code into C++ so that we only
need to load the update service when either we get a timer expiring or the user
makes an explicit request to check for updates.

Comment 3

[Benjamin Smedberg]

Why can't we just fix it so that we fastload JS components with everything else?

Comment 4

[Brian Ryner (not reading)]

We can, but it's still a lot of code, and fastload doesn't make it
instantaneous... there are still a lot of memory allocations, corresponding
directly to the size of the script being loaded.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Simply delaying the load of nsUpdateService isn't an option because nsExtensionManager relies on the TimerManager component defined by the update service. The code for the TimerManager isn't all that hefty, though, so maybe we could resolve this by splitting the Timer and Update code and then delay-loading the Update code by five seconds or so. Thoughts?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

After tinkering with the code for a while I'm starting to have doubts about whether the potential gain we could see from a change here is worth the effort to produce it.

XPCOM initializes and churns through *every* file in the 'bin/components' folder looking for components/services to register. It picks a component loader based only on the file extension and then immediately loads the file. This is problematic because there is no way for a component to keep itself from being loaded. I mean, you can easily defer XPCOM registration by throwing the NS_ERROR_FACTORY_REGISTER_AGAIN code, but if the loader can get that code from the module then it has already done the parsing work and it's too late to save many cycles.

My original thought was to just split the code into two modules - the stuff that has to get loaded immediately (timers) and the rest of the update service. Due to the 'load-it-all' approach that XPCOM takes, however, it seems that splitting the file won't make any difference.

In order to really fix this I think we'd have to create a new component loader that keys on a different file extension (*.js.delay ?) and then modify XPCOM's initialization to include the loader. Then we'd have to figure out a new file format that could pass a timer value for the delay (or use a blanket value for all delay-load js components?) without having to actually parse the javascript... An alternative approach (but less elegant) would be to simply hardwire some filenames as delayed load so that the loaders will just skip the file entirely. Then a timer could fire and complete the load.

Anyway, this seems like a ton of work for what may turn out to be a small perf gain. Fastload might be our best option unless I'm totally missing something. 

Comment 7

[Benjamin Smedberg]

BenT: please note that autoregistration only loads all the components on *first* startup (or in debug builds). In release builds, once the first autoregistration has completed, we only load components on an as-needed basis.

But yes, I'm concerned that we're overoptimizing here, especially with bryner's recent commit which fastloads JS components.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this WONTFIX?

Comment 9

[(dormant account)]

(In reply to comment #7)
> BenT: please note that autoregistration only loads all the components on
> *first* startup (or in debug builds). In release builds, once the first
> autoregistration has completed, we only load components on an as-needed basis.
> 
> But yes, I'm concerned that we're overoptimizing here, especially with bryner's
> recent commit which fastloads JS components.

js_Execute of nsUpdateService takes ~130ms on n810. So we are definetly not
over-optimizing here. That's roughly 10x slower than other components(except
nsExtensionManager)

Comment 10

[Dietrich Ayala (:dietrich)]

We could put all non-timer code in a js module, which is imported on-demand inside the component.

This approach would require no additional component/idl crap (ie: more registration work, more xpconnect traversals), while allowing the bulk of the update code to be loaded post-startup in most cases.

Comment 11

[Robert Strong (they/them - no direct email)]

Agreed but my understanding is that this would not have a significant impact on startup due to the components being fastloaded though I would love to be proven wrong on this. I do think the work done at startup can be optimized and started down this path several months ago though results were sketchy using local talos.

Comment 12

[(dormant account)]

106ms  CallHook (anonymous) file:///home/taras/builds/ff-build.fennec.arm/mobile/dist/bin/xulrunner/modules/XPCOMUtils.jsm:118 11ms
112ms  CallHook (anonymous) file:///home/taras/builds/ff-build.fennec.arm/mobile/dist/bin/xulrunner/modules/XPCOMUtils.jsm:118 3ms
116ms  CallHook (anonymous) file:///home/taras/builds/ff-build.fennec.arm/mobile/dist/bin/xulrunner/modules/XPCOMUtils.jsm:118 2ms
116ms js_Execute file:///home/taras/builds/ff-build.fennec.arm/mobile/dist/bin/xulrunner/components/nsUpdateService.js 109ms

This is a log from my n810. Callhooks are the calls that I see happening. So js_Execute takes 109ms to process the body of the js file, but only 15ms can be accounted for by function calls.
Either my profiling is missing stuff, or it takes a while to process the body of that js file. From the other stuff I profiled, I don't think it's a bug in my measuring method. I'm using my component from bug 470116 to get this output(slightly newer version).

Comment 13

[(dormant account)]

That didnt paste very well, see the relevant lines in http://people.mozilla.com/~tglek/fennec/startup.txt

Comment 14

[Dietrich Ayala (:dietrich)]

Taras can you confirm that these numbers are from a run that doesn't have the js component fastloaded?

Comment 15

[(dormant account)]

(In reply to comment #14)
> Taras can you confirm that these numbers are from a run that doesn't have the
> js component fastloaded?

it is fastloaded.

Comment 16

[Robert Strong (they/them - no direct email)]

It would be interesting to get new numbers since much of the code that was loaded on startup is no longer loaded on startup.

Comment 17

[(dormant account)]

Attachment: ext manager + update service log on n810

(In reply to comment #16)
> It would be interesting to get new numbers since much of the code that was
> loaded on startup is no longer loaded on startup.

Actually looks like we still load the same code. See the 2 js_Execute calls. We might run less code now, but it's all still loaded early on during startup and still hurts.

Comment 18

[Robert Strong (they/them - no direct email)]

The same methods are called but they do quite a bit less than they used to in that we no longer initialize early when it doesn't have to. Do you know what if any ts improvements there were? I should be able to remove one of the observer notifications during startup in UpdateService at some point - when I initially removed it several leaks started happening that didn't make much since.

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to comment #18)
>...
> removed it several leaks started happening that didn't make much since.
s/since/sense/

Comment 20

[(dormant account)]

(In reply to comment #18)
> The same methods are called but they do quite a bit less than they used to in
> that we no longer initialize early when it doesn't have to. Do you know what if
> any ts improvements there were? I should be able to remove one of the observer
> notifications during startup in UpdateService at some point - when I initially
> removed it several leaks started happening that didn't make much since.

Mobile has been suffering ts regressions as they try to finish up features.

But from my profiling output, I believe your work saved at least 100ms of execution in nsUpdateService.js.

Comment 21

[Robert Strong (they/them - no direct email)]

Is this info available via the Tinderbox and if not what is the status of making it available? Those of us without devices have been shooting in the dark for too long trying to improve this for Mobile.

Comment 22

[(dormant account)]

(In reply to comment #21)
> Is this info available via the Tinderbox and if not what is the status of
> making it available? Those of us without devices have been shooting in the dark
> for too long trying to improve this for Mobile.

no, but I have a patch for spitting out that info that you can run on your desktop builds, it's fairly easy to extrapolate. I'll post my latest version to bug 470116 in a sec.

I'm a bit less swamped than I was this summer, so I should be able to run these checks more often if you find measuring this stuff on the desktop to be insufficient.

Comment 23

[Robert Strong (they/them - no direct email)]

Taras, thanks! My observations via standalone Talos on the desktop as well as Talos on the Tinderbox is that there were no noticeable changes so I suspect we won't get much if any indication of success when trying to improve TS on Mobile.

(In reply to bug 471219 comment #59)
> Taras said 99ms is just to get the JS from that file ready to run. That's after
> being pulled from the fastload cache, and before any JS is actually run.
Thanks, that is good to know!

> (In reply to comment #56)
> > That is used to resume downloading an update when the app quits before the
> > download finishes and cleaning up an active update when its state is STATE_NONE
> > during the final-ui-startup notification. I haven't looked into optimizing that
> > specifically and would appreciate any insight as to why you thought it would be
> > possible to not load it.
> 
> those tasks sound like they could be kicked off after first window is up.
Quite possibly but much of this is also used to notify of a failed update so it would have to happen at final-ui-startup or very soon thereafter.

Comment 24

[Robert Strong (they/them - no direct email)]

Just to verify, am I reading the log totals correctly?
nsUpdateService.js   : 291ms
nsExtensionManager.js: 370ms

Comment 25

[(dormant account)]

(In reply to comment #24)
> nsExtensionManager.js: 370ms

I get 96+1+53+21+35 for extension manager, basically everything at the same indentation level can be summed up.

And yes, Ts is useless for exact numbers on this kind of stuff because you need to measure subsets of what Ts measures, otherwise you get buried in the noise.

Comment 26

[Robert Strong (they/them - no direct email)]

Thanks! I should have seen that but I had regexp'd out the text which removed the indents
nsUpdateService.js    : 138+2+6+9+33  = 188ms
nsExtensionManager.js : 96+1+53+21+35 = 206ms

Comment 27

[Robert Strong (they/them - no direct email)]

Is there a bug for adding a notification to replace delayedStartup so it will just be available to all apps?

Comment 28

[Robert Strong (they/them - no direct email)]

(In reply to comment #26)
> Thanks! I should have seen that but I had regexp'd out the text which removed
> the indents
> nsUpdateService.js    : 138+2+6+9+33  = 188ms
> nsExtensionManager.js : 96+1+53+21+35 = 206ms
Looking at the old report from 07-21 in comment #13 we have
nsUpdateService.js      : 109+2+28+36+25 = 200ms

So, the savings don't seem to be all that much so far and definitely no where near 100 ms.

Looks to me like the only area where we can make a significant dent is by lessening the code loaded by js_Execute probably by creating a small component that just performs the actions needed during startup.

Comment 29

[Dietrich Ayala (:dietrich)]

(In reply to comment #27)
> Is there a bug for adding a notification to replace delayedStartup so it will
> just be available to all apps?

bug 364304

Comment 30

[Dietrich Ayala (:dietrich)]

hm, that's not really all apps, just browser.

Comment 31

[Dietrich Ayala (:dietrich)]

(In reply to comment #28)
> Looks to me like the only area where we can make a significant dent is by
> lessening the code loaded by js_Execute probably by creating a small component
> that just performs the actions needed during startup.

can you point at the stuff required during startup? we can find people to do the surgery.

Comment 32

[Robert Strong (they/them - no direct email)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1172

Comment 33

[Robert Strong (they/them - no direct email)]

Note: according to bug 520837 it looks like delayedStartup can still affect TS

Comment 34

[(dormant account)]

(In reply to comment #33)
> Note: according to bug 520837 it looks like delayedStartup can still affect TS

yeah I'm not a big fan of delaying things post-startup. This causes fun sideeffects like freezing the browser ui once it's up.

Comment 35

[Robert Strong (they/them - no direct email)]

Just ran standalone talos on a normal build and a build with ac_add_options --disable-updater (e.g. without nsIApplicationUpdateService, nsIUpdateManager, etc. components and only with the nsIUpdateTimerManager component) and the TS was not improved. So, I really don't think I'll be able to measure a difference with talos on a desktop. It looks like the latest patch in bug 470116 has already bitrotted as well... I'm going to try to fix the failed hunks.

Comment 36

[Robert Strong (they/them - no direct email)]

Looks like the patch had changes to the following that have already landed so the patch in bug 470116 should be fine.
netwerk/base/src/nsStandardURL.cpp
netwerk/base/src/nsStandardURL.h
netwerk/protocol/res/src/nsResProtocolHandler.cpp
netwerk/protocol/res/src/nsResProtocolHandler.h

Comment 37

[(dormant account)]

(In reply to comment #35)
> Just ran standalone talos on a normal build and a build with ac_add_options
> --disable-updater (e.g. without nsIApplicationUpdateService, nsIUpdateManager,
> etc. components and only with the nsIUpdateTimerManager component) and the TS
> was not improved. So, I really don't think I'll be able to measure a difference
> with talos on a desktop. It looks like the latest patch in bug 470116 has
> already bitrotted as well... I'm going to try to fix the failed hunks.

So I checked on n810. This build option makes nsUpdateService.js shrink to 8K(from 105) and load time goes down to 19ms(ie 10fold).  Of course depending on the speed of your desktop you are looking at 10x of what mobile is, so 100ms/10 is easy to get lost in the noise.

Comment 38

[Robert Strong (they/them - no direct email)]

Thanks Taras! Couple more and hopefully last questions.

Are the numbers from a debug or an optimized build? Are they cold or warm start?

Comment 39

[(dormant account)]

optimized, warm. Same as last set. there is relatively little diffence between warm/cold on n810.

Comment 40

[Robert Strong (they/them - no direct email)]

Taras, could I get numbers with the patches in Bug 520526 (already landed on trunk) and Bug 512651 applied? Thanks

Comment 41

[(dormant account)]

http://people.mozilla.com/~tglek/log.txt

Comment 42

[Robert Strong (they/them - no direct email)]

Thanks Taras! Looks like with the patches from Bug 520526 (already landed on
trunk) and Bug 512651 applied the number is now:
nsUpdateService.js : 78+2+7+2+30    = 119

Prior results are:
from 7/21
nsUpdateService.js : 109+2+28+36+25 = 200ms

from 10/2
nsUpdateService.js : 138+2+6+9+33   = 188ms

So, it looks like Bug 512651 will save about about 70ms on TS for Maemo.

Comment 43

[Robert Strong (they/them - no direct email)]

For this test run the nsExtensionManager.js js_Execute went down from 96ms to 82ms while nsUpdateService.js went down from 138 to 78

I'm going to revise the estimated TS reduction to around 45ms though it is just a more conservative guesstimate. It also looks like the latest log didn't catch all of the nsExtensionManager.js calls.

Comment 44

[(dormant account)]

(In reply to comment #43)
> For this test run the nsExtensionManager.js js_Execute went down from 96ms to
> 82ms while nsUpdateService.js went down from 138 to 78
> 
> I'm going to revise the estimated TS reduction to around 45ms though it is just
> a more conservative guesstimate. It also looks like the latest log didn't catch
> all of the nsExtensionManager.js calls.


It's not supposed to catch all calls, just ones that take > 0.1ms.

Keep in mind. Most recent numbers are also a bit better because of 412796 landing, for some reason that reduced js_execute times by 20%. extensionmanager's js_execute got faster too, as did times for browser.js and other large files.

Comment 45

[Robert Strong (they/them - no direct email)]

Sorry, it looks like the file didn't completely download the first time.
nsExtensionManager.js : 82+1+50+14+41 = 188ms
nsUpdateService.js    : 78+2+7+2+30   = 119ms

The previous number for nsExtensionManager.js was 206ms so it improved by 18ms. From this I'm confident there is a decent win with the patch in Bug 512651

Comment 46

[Robert Strong (they/them - no direct email)]

Still have a bunch of cleanup to do but this is the result so far for just this patch with the other patches I' working on:
Before:
loaded on profile-after-change
nsUpdateService.js -> 103 KB (105,762 bytes)

After:
loaded on profile-after-change
nsUpdateSvcStartup.js -> 12.0 KB (12,289 bytes)
nsUpdateTimerManager.js -> 8.48 KB (8,684 bytes)

only loaded on profile-after-change when there is cleanup from a previous update or a download needs to be resumed.
nsUpdateService.js -> 96.4 KB (98,736 bytes)

Comment 48

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev1

Requires the patches from bug 521452 and bug 522583

Comment 49

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev2

Comment 50

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev3

Comment 51

[Robert Strong (they/them - no direct email)]

Taras, could you provide logs on Fennec prior to and after applying the following patches? I'd like to get an idea of what impact this approach will have for Fennec.
Bug 521452 - attachment #407884 [details] [diff] [review]
Bug 522583 - attachment #407844 [details] [diff] [review]
and this bug attachment #407959 [details] [diff] [review]

Thanks!

Comment 52

[Robert Strong (they/them - no direct email)]

btw: just landed Bug 521452 and Bug 522583 on trunk.

Comment 53

[Robert Strong (they/them - no direct email)]

On trunk I did 6 runs (actually more but I removed the runs that had obvious noise) without and with this patch on the oban WinCE board. The numbers from these runs were very consistent.
average without = 181.6 ms
average with    = 148.5 ms
savings         =  33.1 ms

Comment 54

[Robert Strong (they/them - no direct email)]

Note: the above numbers are by no means the final ones and I think I might be able to knock another 50ms off

Comment 55

[Robert Strong (they/them - no direct email)]

new numbers... better than I expected
average without = 181.6 ms
average with    =  74.0 ms
savings         = 107.6 ms

Comment 56

[Robert Strong (they/them - no direct email)]

ten consecutive runs on Firefox WinCE with the patch without removing noisy runs:
1.  44 ms
2.  46 ms
3.  47 ms
4.  42 ms
5.  47 ms
6.  71 ms
7.  73 ms
8.  44 ms
9.  67 ms
10. 40 ms

Even without removing what appears to be noisy runs this brings the time spent during startup to an average of 52.1 ms. I think this is pretty darn good so I'm going to cleanup the patch and submit it for review.

Comment 57

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 58

[Robert Strong (they/them - no direct email)]

Comment on attachment 409036 [details] [diff] [review]
patch rev1

>diff --git a/toolkit/mozapps/update/src/nsUpdateQuickStartup.js b/toolkit/mozapps/update/src/nsUpdateQuickStartup.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/src/nsUpdateQuickStartup.js
>@@ -0,0 +1,98 @@
>...
>+  let statusFile = getUpdateDirNoCreate([DIR_UPDATES, "0"]);
>+  statusFile.append(FILE_UPDATE_STATUS);
>+  // If the update.status file exists then 
I fixed this comment to read
// If the update.status file exists then initiate post update processing

Instead of calling it update-quick-startup, UpdateServiceStartup, etc. what do you think ofupdate-service-stub, UpdateServiceStub, etc.?

Comment 59

[Robert Strong (they/them - no direct email)]

btw: I pushed the patch to try and all looks good

Comment 60

[Robert Strong (they/them - no direct email)]

Interesting numbers from try talos with this patch... I hope these numbers are accurate and not just a fluke!

WINNT 6.0 try talos dirty
ts: 624.37
ts: 634.63
ts: 641.21
ts: 631.26
ts: 591.26
ts: 604.42
ts: 634.16
ts: 620.63
ts: 593.68
ts: 590.21
ts: 603.95
ts: 598.26
ts: 600.63
ts: 603.74
ts: 599.53
ts: 610.16
ts: 603.74
ts: 598.84
ts: 606.26
ts: 605.05
ts: 607.21
ts: 603.47
ts: 605.32
ts: 619.95
ts: 591.26
ts: 593.11
ts: 603.63
ts: 606.68
ts: 579.42 <--- try build with this patch
ts: 634.21
ts: 624.21
ts: 612.16

Comment 61

[Dave Townsend [:mossop]]

Comment on attachment 409036 [details] [diff] [review]
patch rev1

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in

>@@ -335,14 +425,7 @@ function getStatusTextFromCode(code, def
> function getUpdatesDir() {
>   // Right now, we only support downloading one patch at a time, so we always
>   // use the same target directory.
>-  var updateDir = getUpdateDir([]);
>-  updateDir.append(DIR_UPDATES);
>-  updateDir.append("0");
>-  if (!updateDir.exists()) {
>-    LOG("General", "getUpdatesDir - update directory " + updateDir.path +
>-        " doesn't exist, creating...");
>-    updateDir.create(Ci.nsILocalFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>-  }
>+  var updateDir = getUpdateDirCreate([DIR_UPDATES, "0"]);
>   return updateDir;
> }

Just return getUpdateDirCreate... no need for the updateDir variable.

Comment 62

[Dave Townsend [:mossop]]

Comment on attachment 409036 [details] [diff] [review]
patch rev1

>diff --git a/toolkit/mozapps/update/src/nsUpdateQuickStartup.js b/toolkit/mozapps/update/src/nsUpdateQuickStartup.js

>+function UpdateServiceStartup() {
>+  // This is needed by the xpcshell tests to instantiate this component.
>+  this.wrappedJSObject = this;

This doesn't seem to be the case.

>+
>+  let statusFile = getUpdateDirNoCreate([DIR_UPDATES, "0"]);
>+  statusFile.append(FILE_UPDATE_STATUS);
>+  // If the update.status file exists then 

This comment seems half finished

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in

>+function getObserverService() {
>+  return Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+}

Any reason not to use a lazy getter here (and in nsTimerManager)?

Comment 63

[Robert Strong (they/them - no direct email)]

Fixed all review comments except for the lazy getter for the observer service... since it is only needed once on startup and once on shutdown I didn't see much value in having it hang around for the entire session.

I also discussed the file naming with Vlad and decided to go nsUpdateServiceStub.js, UpdateServiceStub, etc.

Will attach an updated patch later and land. Thanks for the quick review!

Comment 64

[Robert Strong (they/them - no direct email)]

(In reply to comment #63)
>...
> Will attach an updated patch later and land.
land after I perform due diligence to get the package.manifest's updated for SeaMonkey, Thunderbird, and Sunbird (if it builds on trunk).

Comment 65

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

Comment 66

[Robert Strong (they/them - no direct email)]

Attachment: Thunderbird patch

Phil, we're going to need this to prevent breaking app update on Thunderbird trunk and it will need the MOZILLA_1_9_2_BRANCH ifdef's removed if/when this lands on 1.9.2. Thanks!

Comment 67

[Phil Ringnalda (:philor)]

I'm not sure this actually makes it any easier to manage the ifdefs, but we're branched, so you only really need to deal with 1.9.1 for comm-1.9.1, and 1.9.2/1.9.3 for commcentral.

Comment 68

[Robert Strong (they/them - no direct email)]

Comment on attachment 409266 [details] [diff] [review]
Thunderbird patch

I forgot about that... I'll resubmit after I've finished compiling with the changes.

Comment 69

[Robert Strong (they/them - no direct email)]

Attachment: Thunderbird patch rev2

Comment 70

[Robert Strong (they/them - no direct email)]

Attachment: Seamonkey patch

Frank, we're going to need this to prevent breaking app update on Seamonkey
trunk and it will need the MOZILLA_1_9_2_BRANCH ifdef's removed if/when this
lands on 1.9.2. Thanks!

Comment 71

[Robert Strong (they/them - no direct email)]

Attachment: Sunbird patch

Martin, we're going to need this to prevent breaking app update on Sunbird trunk and it will need the MOZILLA_1_9_2_BRANCH ifdef's removed if/when this lands on 1.9.2. Thanks!

Comment 72

[Robert Strong (they/them - no direct email)]

Comment on attachment 409282 [details] [diff] [review]
Seamonkey patch

Robert, I need to land this soon on trunk so it can bake before landing on 1.9.2. Can you review it or get someone else to in case Frank isn't able to soon?

Comment 73

[Robert Kaiser]

I'm pretty sure that Frank will come to it sooner than me, actually.

Comment 74

[Robert Strong (they/them - no direct email)]

Comment on attachment 409282 [details] [diff] [review]
Seamonkey patch

CTho, I need to land this soon on trunk so it can bake before landing on 1.9.2 and if it lands without the Seamonkey patch it will break Seamonkey's app update. Since you are one of the installer owners can you review it and if not could you find someone to review it so I can get this landed without breaking app update for Seamonkey?

Comment 75

[Daniel Veditz [:dveditz]]

Comment on attachment 409282 [details] [diff] [review]
Seamonkey patch

r=dveditz

Comment 76

[Robert Kaiser]

Comment on attachment 409282 [details] [diff] [review]
Seamonkey patch

Chris hasn't been active any more for some time, the SeaMonkey project areas doc badly needs work. Frank is bascially owning the SeaMonkey Winodws installer and integration nowdays, so asking him to review this.

Comment 77

[Phil Ringnalda (:philor)]

Just land it, break them, and fix them when they get around to reviewing. It's not like SM's trunk packages or removed-files are actually up-to-date anyway.

Comment 78

[Robert Strong (they/them - no direct email)]

kairo, I'm going to push this along with the Seamonkey patch using dveditz's r to avoid breaking Seamonkey's app update on trunk Tuesday when the tree is green unless I hear from you that I shouldn't land the Seamonkey patch in which case Seamonkey's app update will be broken.

Comment 79

[Robert Kaiser]

Comment on attachment 409282 [details] [diff] [review]
Seamonkey patch

Rob, that's OK, I trust Dan enough on that :)

Comment 80

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/a82e3f58698a

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/21d65f4a4439
http://hg.mozilla.org/comm-central/rev/b1383e748fc7
http://hg.mozilla.org/comm-central/rev/95694ec14a49

Comment 81

[Robert Strong (they/them - no direct email)]

Note: filed bug 526333 to try to make it easier to make these types of changes for toolkit apps.

Comment 82

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409264 [details] [diff] [review]
patch for checkin

a192=beltzner, big perf win on mobile devices

Comment 83

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1d6e61283c22

Pushed removal of ifdef MOZILLA_1_9_2_BRANCH to comm-central now that this has landed on 1.9.2
http://hg.mozilla.org/comm-central/rev/ba3ba62897f4
http://hg.mozilla.org/comm-central/rev/81306b648d1f
http://hg.mozilla.org/comm-central/rev/d9e3d3ec7849