593743 - (about:startup) about:startup page showing historical startup timings

Status: NEW

(Firefox :: General, enhancement)

Description

[Daniel Brooks [:db48x]]

Attachment: screenshot of progress so far

There's probably some documentation on the wiki, but Firefox will be recording the time elapsed between when the app is launched and the session is restored and presenting this on a graph at about:startup.

So far I've implemented the unix and windows platform specific code, storage database code, the about redirector entry, and started the about page. There's no graph yet; it just displays a chart.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: just the strings

Comment 2

[Dave Garrett]

Is there any reason this couldn't live in Toolkit instead?

Comment 3

[Johnathan Nightingale [:johnath]]

Comment on attachment 473456 [details] [diff] [review]
just the strings

>diff --git a/browser/locales/en-US/chrome/browser/aboutStartup.dtd b/browser/locales/en-US/chrome/browser/aboutStartup.dtd
>new file mode 100644
>--- /dev/null
>+++ b/browser/locales/en-US/chrome/browser/aboutStartup.dtd
>@@ -0,0 +1,12 @@
>+<!ENTITY title "Application Startup Timing">

Maybe source brand.dtd and change this string to "&brandShortName; Startup History"?

>+<!ENTITY chart "Chart">
>+<!ENTITY graph "Graph">

Chart is intended to mean "text based, tabular data", right? Some people use "chart" as a synonym for graph, I think "Table" works better here?

>+++ b/browser/locales/en-US/chrome/browser/aboutStartup.properties

>+appUpgrade=%1$S %2$S (%3$S)
>+extensionInstalled=%1$S %1$2 installed
>+extensionUpgraded=%1$S upgraded to %1$2
>+extensionEnabled=%1$S %1$2 enabled
>+extensionDisabled=%1$S %1$2 disabled

These should have localization notes ( https://developer.mozilla.org/en/Localization_notes ) explaining what the variables are likely to contain, so that localizers know what to do with them.

Also, Dave's question is an interesting one - can this live in toolkit? Not a deal breaker if you are currently relying on Firefox-specific code, but I think this should be pretty toolkit-generic?

Comment 4

[Johnathan Nightingale [:johnath]]

Pike/l10n: This is work Daniel's doing on an about:startup page to let users see their startup-time history. It should be basically user-invisible, you'd have to know to go to the URL, but it's potentially helpful for troubleshooting.

It's not a blocker, just an opportunistic landing that should be well isolated from other things, but I wanted to make you aware regardless.

Comment 5

[Daniel Brooks [:db48x]]

(In reply to comment #2)
> Is there any reason this couldn't live in Toolkit instead?

Nope. In fact the meat of the code is in toolkit; I'll move the ui code there as well. I am using the browser's AboutRedirector though.

> >+<!ENTITY title "Application Startup Timing">
> 
> Maybe source brand.dtd and change this string to "&brandShortName; Startup
> History"?

Good idea.

> 
> >+<!ENTITY chart "Chart">
> >+<!ENTITY graph "Graph">
> 
> Chart is intended to mean "text based, tabular data", right? Some people use
> "chart" as a synonym for graph, I think "Table" works better here?

Done, although I wasn't going to make the table very prominent. Graphs are prettier :)

> These should have localization notes (
> https://developer.mozilla.org/en/Localization_notes ) explaining what the
> variables are likely to contain, so that localizers know what to do with them.

Ooh, I knew I was forgetting something. Thanks.

Also, I'll throw in the changes to the jar manifest while I'm in here. :)

Comment 6

[Daniel Brooks [:db48x]]

Attachment: improved strings

Comment 7

[Guillermo López :willyaranda (probably SLOW response)]

Is there a reason to use microseconds instead of milliseconds? I think it's not really useful to have that precision in the data.

Comment 8

[Daniel Brooks [:db48x]]

NSPR timestamps are always in microseconds, even if the timer functions such as PR_Now only ever store milliseconds in them. I intend for the graph to show milliseconds. Actually, on linux the data won't even be precise enough for that, since the launch timestamp is only known to within one fiftieth of a second.

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 473528 [details] [diff] [review]
improved strings

<!ENTITY chart "Table">
is generally not good, use good variable names?

On the overall concept, I don't think we should pre-land strings. Less so for things that aren't blockers. We'll just end up with un-used strings on stable branches that then don't match what's actually used in the upcoming release because things evolved.

Comment 10

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

Axel: we're pretty close to having this work come in after beta 6, it's extremely low risk and extremely high value, so while I've been professing the same thing as you in comment 9, this is a case that I think warrants an exception.

Comment 11

[Axel Hecht [:Pike]]

If you want to make this a betaN blocker, you could twist my arm.

Re the patch, the %1 etc in the localization notes should be %1$S etc, to avoid confusion with other variable formats.

Comment 12

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

Comment on attachment 473528 [details] [diff] [review]
improved strings

As a coding-style comment, Axel's right; we should be naming the entities a bit more descriptively, such as

<!ENTITY about.startup.title "String goes here">

Design-wise, I think these are the strings we want to use:


Start Time

Most Recent Start                   Time                                 Elapsed Time

&brandShortName Launched
&brandShortName Started
&brandShortName Ready

_Graph_ | _Table_

Past Starts                         Launch Time                  Start Time                Ready Time


note that by default we're going to move away from session restore on startup, which is why I'm renaming that to "ready" and "ready time" - the delta between when the application "starts" and when "startup" is complete. Hopefully that makes sense.

Comment 13

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

(that mockup made more sense when I had more horizontal width in which to play, oops!)

Comment 14

[Daniel Brooks [:db48x]]

Attachment: improved strings

(In reply to comment #12)
> <!ENTITY about.startup.title "String goes here">

Repainted 

> note that by default we're going to move away from session restore on startup,
> which is why I'm renaming that to "ready" and "ready time" - the delta between
> when the application "starts" and when "startup" is complete. Hopefully that
> makes sense.

Yes, that helps a lot, thanks.

Comment 15

[Johnathan Nightingale [:johnath]]

Thanks Daniel,

Can we get a version of this patch that is absolute, instead of relative-to-your-last-patch? These should all be net-new strings (and files, iinm) so it's odd (and maybe hg-import-unfriendly) to see +/- diffs.

Comment 16

[Daniel Brooks [:db48x]]

Attachment: improved strings (combined diff)

Sure, here's a combined diff. The other file is just an hg export of the three changes I made, as I revised the patch based on suggestions in the bug; hg import will handle it just fine.

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 474184 [details] [diff] [review]
improved strings (combined diff)

We'll need localization notes in aboutStartup.dtd to explain the subtle differences between launched, started, ready, launch time, start time, elapsed time. They're all similar concepts and can easily be confused.

Also, it'd be nice to have an introduction comment in the file that mentions that this is about:startup, so that once people can test, they can test.

Comment 18

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

(not a blocker, though we'll approve the patch once reviewed)

It would be nice, Axel, if you could give an example of a localization comment, as it's pretty clear that this is new territory for the patch author.

Daniel: here's an example: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/aboutSupport.dtd

Comment 19

[Axel Hecht [:Pike]]

The docs for localization notes are https://developer.mozilla.org/en/Localization_notes, too.

Comment 20

[Daniel Brooks [:db48x]]

Attachment: improved the localization notes

Comment 21

[Axel Hecht [:Pike]]

Comment on attachment 474262 [details] [diff] [review]
improved the localization notes

As per IRC, feedback+ from me, thanks for the cycles.

Comment 22

[Daniel Brooks [:db48x]]

Attachment: screenshot of progress so far

I'll push it to the try server momentarily, to get some testable builds

Comment 23

[Daniel Brooks [:db48x]]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c5f2255205bf/

Comment 24

[Daniel Brooks [:db48x]]

Naturally, those builds ended up with a tiny glitch that renders them all useless. See if you can spot it in the patch.

Comment 25

[Rob Campbell [:rc] (:robcee)]

displaying µs in the numerical values and ms on the graph is a little confusing. Maybe just default to ms for all values since that's likely going to be the highest usable resolution anyway?

Comment 26

[Daniel Brooks [:db48x]]

(In reply to comment #25)
> displaying µs in the numerical values and ms on the graph is a little
> confusing. Maybe just default to ms for all values since that's likely going to
> be the highest usable resolution anyway?

Yep. As it happens, I fixed that yesterday. I've been a bit remiss and haven't pushed this to my user repository, but I'll do that today.

Comment 27

[Daniel Brooks [:db48x]]

Attachment: hg bundle containing the work so far

This is very nearly complete; it lacks only a few small things.

Comment 28

[Daniel Brooks [:db48x]]

Attachment: screenshot of progress so far

I welcome all feedback. Are the colors good? I'm not very happy with the table/graph toggler.

Comment 29

[Daniel Brooks [:db48x]]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2dc5f4af2c2c

Also, looking at it with a fresh profile (instead of the one I've been testing in for two weeks), the graph looks very empty. Perhaps it shouldn't try to show 7 days worth if there isn't enough data.

Comment 30

[u88484]

(In reply to comment #28)
> Created attachment 474876 [details]
> screenshot of progress so far
> 
> I welcome all feedback. Are the colors good? I'm not very happy with the
> table/graph toggler.

1) What's the difference between launch time and startup time?
2) I think about 1000 ms should be scaled to in seconds in the graph and seconds and minutes in the table
3) I think there is way too much detail in the timestamp.  I don't think the day or the week, -400 GMT and the time zone are needed.  Why would the user need to know (or care) about any of that?
4) I know this is way out of scope but looking at that big spike gave me the idea that maybe the user can be notified of what happened that caused a huge spike (huge sessionstore, recently installed extension, etc.)

Comment 31

[Daniel Brooks [:db48x]]

(In reply to comment #30)
> (In reply to comment #28)
> > Created attachment 474876 [details] [details]
> > screenshot of progress so far
> > 
> > I welcome all feedback. Are the colors good? I'm not very happy with the
> > table/graph toggler.
> 
> 1) What's the difference between launch time and startup time?

Launch time is the time that you clicked on the Firefox icon and your OS started loading the executable. Startup time is when the OS handed control over to Firefox. I don't think a help bubble explaining that would get in the way, do you?

> 2) I think about 1000 ms should be scaled to in seconds in the graph and
> seconds and minutes in the table

Yes, that's a good idea. I think the table will stay in milliseconds, since the idea is that the table would be most useful for direct comparisons of the values, and rounding would hurt that.

> 3) I think there is way too much detail in the timestamp.  I don't think the
> day or the week, -400 GMT and the time zone are needed.  Why would the user
> need to know (or care) about any of that?

Ah, true. We should show the date in the user's locale in any case.

> 4) I know this is way out of scope but looking at that big spike gave me the
> idea that maybe the user can be notified of what happened that caused a huge
> spike (huge sessionstore, recently installed extension, etc.)

Yes, it would be nice if we could always tell. Currently it'll show a vertical bar whenever the app version number changes, and tomorrow it'll show vertical bars for when you install or uninstall extensions. You can see numerous grey bars that mark minor upgrades, changes only to the build id that happened each time I rebuilt my Firefox. Each of those is accompanied by a large spike, although not every such spike is the result of a rebuild. You can see that spike of 200,000 ms (not a typo, that's 200 seconds) because I had bogged my system down a bit for testing purposes.

Comment 32

[Axel Hecht [:Pike]]

Generally, I'm not a friend to show linear graphs for stuff that is not, do the lines really help, compared to just data dots? It'd also make it easier to strip boring times from the graphs, and zoom in to times that had more restarts.

Comment 33

[Daniel Brooks [:db48x]]

Attachment: implementation

Here's a combined patch. If you want the history you can pull from my user repository (url field).

Comment 34

[Daniel Brooks [:db48x]]

Oh, and if you've already run a build with this feature in it, you'll need to delete the old startup.sqlite from your profile. The schema changed slightly, and as yet there is no code to handle a schema upgrade (doesn't seem necessary at this point.)

Comment 35

[Daniel Brooks [:db48x]]

builds are starting to show up at http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4e82cb73ec34/ as well.

Comment 36

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #35)
> builds are starting to show up at
> http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4e82cb73ec34/
> as well.

They crash for me on OS X 64-bit. The only crash info I get is:

Add-ons: {972ce4c6-7e08-4474-a285-3208198ce6fd}:4.0b7pre,[e-mail]:1
BuildID: 20100921125120
CrashTime: 1285107495
EMCheckCompatibility: true
Email: [e-mail]
FramePoisonBase: 7ffffffff0dea000
FramePoisonSize: 4096
InstallTime: 1285107463
ProductName: Firefox
ReleaseChannel: nightly
SecondsSinceLastCrash: 14
StartupTime: 1285107493
Theme: classic/1.0
Throttleable: 1
Vendor: Mozilla
Version: 4.0b7pre

This report also contains technical information about the state of the application when it crashed.

Comment 37

[Benjamin Smedberg]

Comment on attachment 477274 [details] [diff] [review]
implementation

I am not going to have time to review this before Firefox 4.

Comment 38

[Blake Cutler]

This data would be extremely valuable for our Test Pilot analysis.  Is about:startup definitely going to miss Firefox 4?

Comment 39

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

bsmedberg: the vast majority of this patch is tests, fwiw. Also, startup time is one of our key metrics and competitive differentiators. I'll finish the ui-r tonight, but I think we should try to find someone to look over the 200 or so lines of code in the startup path here.

Daniel: I'm assuming you've passed this through tryserver to ensure it doesn't regress Ts, right? ;)

Comment 40

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

Comment on attachment 474262 [details] [diff] [review]
improved the localization notes

a=beltzner for default and the b7 relbranch

Comment 41

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

Comment on attachment 477274 [details] [diff] [review]
implementation

This is great. I'm not really convinced in there being a ton of value in the graph, but there's no real reason to remove it at this point (unless the code reviewer feels that it would simplify their task, I suppose). Really glad you got the event entries for add-on install and session restore. I can't tell from inspection: are those also being added to the table of events, or just the graph?

An added bonus would be an export data option, but not worth adding the strings.

Comment 42

[Reed Loden [:reed]]

Comment on attachment 474262 [details] [diff] [review]
improved the localization notes

>+<!-- LOCALIZATION NOTE (about.startup.app.ready): The timestamp for
>+when the Firefox startup process finished, and when Firefox was first
>+ready for the user to use. -->
>+<!ENTITY about.startup.app.ready"&brandShortName; Ready">

Seems like you need an extra space or something there?

Comment 43

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

Vlad: I know you have a beta8 blocker and a few betaN blockers, but any chance you'd be able or willing to steal this review from Benjamin or suggest another reviewer?

Comment 44

[Axel Hecht [:Pike]]

(In reply to comment #42)
> Comment on attachment 474262 [details] [diff] [review]
> improved the localization notes
> 
> >+<!-- LOCALIZATION NOTE (about.startup.app.ready): The timestamp for
> >+when the Firefox startup process finished, and when Firefox was first
> >+ready for the user to use. -->
> >+<!ENTITY about.startup.app.ready"&brandShortName; Ready">
> 
> Seems like you need an extra space or something there?

Yes, and that seems to be fixed in the implementation patch, gladly.

Comment 45

[Jono Xia]

To clarify Blake's comment:  It would be extremely valuable for our Test Pilot analysis to be able to query nsIXULRuntime.launchTimestamp, .startupTimestamp, and .restoredTimestamp.  So I strongly hope that at least the recording of those three timestamps will make it in, whether or not the database and the about:startup page make it in.

Comment 46

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

Can we split the patch out into two parts for quicker review?

Part 1: Strings & nsIXULRuntime.* timestamps
Part 2: DB and about:startup implementation

Comment 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 477274 [details] [diff] [review]
implementation

This looks fine, though don't we already have jquery etc. in the tree?  Can you take a look, and if so file a bug to stick them all in one place that we can reference via a chrome URI or something?

Also, nsIAppStartup.idl and nsIXULRuntime.idl both need IID bumps.

Comment 48

[Daniel Brooks [:db48x]]

(In reply to comment #47)
> Comment on attachment 477274 [details] [diff] [review]
> implementation
> 
> This looks fine, though don't we already have jquery etc. in the tree?  Can you
> take a look, and if so file a bug to stick them all in one place that we can
> reference via a chrome URI or something?
> 
> Also, nsIAppStartup.idl and nsIXULRuntime.idl both need IID bumps.

Thanks Vlad! I'll look again to see if we already have jquery in the tree; so far I've only seen it in the Test Pilot extension and some tests. At any rate, this patch does make a separate chrome package for it (see the chrome://jquery/content/ urls.) The files still live in toolkit.jar though.

Comment 49

[Axel Hecht [:Pike]]

panorama is using some stripped/tailored clone called iq,
browser/base/content/tabview/iq.js.

Comment 50

[Daniel Brooks [:db48x]]

(In reply to comment #49)
> panorama is using some stripped/tailored clone called iq,
> browser/base/content/tabview/iq.js.

Huh, that's an odd thing to do. I suppose it's faster that way?

Comment 51

[Daniel Brooks [:db48x]]

Attachment: implementation

includes the iid bumps, and a simple cache in nsXULAppInfo::GetLaunchTimestamp (no reason to do that work more than once.)

Also, I apparently never requested approval for this one. Moving the reviews forward.

Comment 52

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

Comment on attachment 485625 [details] [diff] [review]
implementation

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/public/nsIAppStartup.idl

> interface nsIAppStartup : nsISupports

>+    readonly attribute PRUint64 restoredTimestamp;

This is currently really a browser-specific event, so I'm not sure it makes sense to put it on nsIAppStartup...

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/src/nsAppStartup.cpp

>+  } else if ((!strcmp(aTopic, "sessionstore-browser-state-restored")) ||
>+             (!strcmp(aTopic, "sessionstore-windows-restored"))) {
>+    RecordStartupDuration();

I think you only want to observe sessionstore-windows-restored. browser-state-restored can fire in non-startup cases (e.g. for private browsing transitions, IIRC).

>+nsAppStartup::GetRestoredTimestamp(PRUint64 *aResult)
>+{
>+  *aResult = (PRTime)mRestoredTimestamp;
>+  return NS_OK;
>+}

This will return 0 if called before the restore has completed, right? That should be documented, at least... Looks like it can also affect about:startup loaded before the restore is completed.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/content/aboutStartup.js

Most of the services used in here can be accessed via Services.jsm.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/content/aboutStartup.xhtml

>+   - The Initial Developer of the Original Code is
>+   - the Mozilla Corporation.

Should be "Mozilla Foundation" (applies to all license headers, see http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html )

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/content/jquery/LICENSE.txt

Need to make changes to license.html accordingly (there's already mention of jQuery there, so just add this path to it).

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/mozapps/extensions/AddonManager.jsm

>+  _addNotificationListeners: function()
>+  {
>+    const svc = Cc["@mozilla.org/observer-service;1"]
>+                  .getService(Ci.nsIObserverService);

This should use Services.obs. But this method doesn't seem to be called? Given that, I think you should split it off to a followup bug (along with the relevant nsAppStartup code) - Mossop should review changes to the addons manager.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/themes/gnomestripe/global/aboutStartup.css
>diff -r df51ba69a839 -r 8ad67554aadd toolkit/themes/pinstripe/global/aboutStartup.css
>diff -r df51ba69a839 -r 8ad67554aadd toolkit/themes/winstripe/global/aboutStartup.css

These should have license headers.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/xre/nsAppRunner.cpp

>+NS_IMETHODIMP nsXULAppInfo::GetLaunchTimestamp(PRUint64 *aTimestamp)

>+#ifdef XP_UNIX

This is completely broken on Mac... In fact it seems to cause a crash on startup. You probably want to #ifndef XP_MACOSX it?

>diff -r df51ba69a839 -r 8ad67554aadd xpcom/system/nsIXULRuntime.idl

> interface nsIXULRuntime : nsISupports

>+  readonly attribute unsigned long long launchTimestamp;
>+  readonly attribute unsigned long long startupTimestamp;

Add some comments? It's not at all obvious how they differ.

Comment 53

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

(In reply to comment #52)
> >+  *aResult = (PRTime)mRestoredTimestamp;
> >+  return NS_OK;
> >+}
> 
> This will return 0 if called before the restore has completed, right?

Oh, actually it looks like it will return uninitialized memory.

Comment 54

[Daniel Brooks [:db48x]]

Attachment: fixes review comments

Thanks Gavin

(In reply to comment #52)
> Comment on attachment 485625 [details] [diff] [review]
> implementation
> 
> >diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/public/nsIAppStartup.idl
> 
> > interface nsIAppStartup : nsISupports
> 
> >+    readonly attribute PRUint64 restoredTimestamp;
> 
> This is currently really a browser-specific event, so I'm not sure it makes
> sense to put it on nsIAppStartup...

Well, the only thing browser-specific about it is the name of the observer notification that we listen for. Every app will have some way of determining that startup has ended and the app is ready to use. I guess I could rename it to readyTimestamp if you want.

> 
> >diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/src/nsAppStartup.cpp
> 
> >+  } else if ((!strcmp(aTopic, "sessionstore-browser-state-restored")) ||
> >+             (!strcmp(aTopic, "sessionstore-windows-restored"))) {
> >+    RecordStartupDuration();
> 
> I think you only want to observe sessionstore-windows-restored.
> browser-state-restored can fire in non-startup cases (e.g. for private browsing
> transitions, IIRC).

An excellent point, thank you for clarifying that.

> 
> >+nsAppStartup::GetRestoredTimestamp(PRUint64 *aResult)
> >+{
> >+  *aResult = (PRTime)mRestoredTimestamp;
> >+  return NS_OK;
> >+}
> 
> This will return 0 if called before the restore has completed, right? That
> should be documented, at least... Looks like it can also affect about:startup
> loaded before the restore is completed.

It doesn't get called before the timestamp has been set, I've tried it.

> >+NS_IMETHODIMP nsXULAppInfo::GetLaunchTimestamp(PRUint64 *aTimestamp)
> 
> >+#ifdef XP_UNIX
> 
> This is completely broken on Mac... In fact it seems to cause a crash on
> startup. You probably want to #ifndef XP_MACOSX it?

Ah, yes. Last night as I was going to sleep I thought I might've forgotten to pull this change in.

Comment 55

[Marco Bonardo [:mak]]

Comment on attachment 485730 [details] [diff] [review]
fixes review comments

This needs superreview for idl changes.
Also I think you should move jQuery to a separate patch, since it does not need code review but it's a large part  of this patch

I was asked to skim through the database stuff, this I'm just looking at it. If anybody needs me to do a global pass, let me know.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/src/nsAppStartup.cpp

>+nsresult nsAppStartup::RecordStartupDuration()
>+{
>+  nsresult rv;

should define at first use

>+  PRTime launched = 0, started = 0;
>+  mRestoredTimestamp = PR_Now();
>+
>+  nsCOMPtr<mozIStorageConnection> db;
>+  rv = OpenStartupDatabase(getter_AddRefs(db));
>+  NS_ENSURE_SUCCESS(rv, rv);

this can be simplified, see below.
  
>+  nsCOMPtr<mozIStorageStatement> statement;
>+  rv = db->CreateStatement(NS_LITERAL_CSTRING("INSERT INTO duration VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)"),
>+                           getter_AddRefs(statement));
>+  NS_ENSURE_SUCCESS(rv, rv);

when doing INSERTs always specify fields please,
  INSERT INTO table (field1, field2, ...) VALUES (?1, ?2, ...)
this makes easier future ALTER and also increases a lot the readabiity of the insert (preventing binding errors)
You want to use CreateAsyncStatement here
this call seems to be too high in the method, move it just before using the statement.

>+  nsCOMPtr<nsIXULRuntime> runtime = do_GetService(XULRUNTIME_SERVICE_CONTRACTID);
>+  nsCOMPtr<nsIXULAppInfo> appinfo = do_QueryInterface(runtime);
>+
>+  runtime->GetLaunchTimestamp((PRUint64*)&launched);
>+  runtime->GetStartupTimestamp((PRUint64*)&started);
>+
>+  if (!launched)
>+    launched = started;

brace ifs (also single line ones) per coding style
use static_cast<> rather than old style casts

>+  nsCAutoString appVersion, appBuild, platformVersion, platformBuild;
>+  appinfo->GetVersion(appVersion);
>+  appinfo->GetAppBuildID(appBuild);
>+  appinfo->GetPlatformVersion(platformVersion);
>+  appinfo->GetPlatformBuildID(platformBuild);

cast to (void) if you don't care about errors
  
>+  rv = statement->BindInt64Parameter(0, launched);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = statement->BindInt64Parameter(1, started - launched);
>+  NS_ENSURE_SUCCESS(rv, rv);
...

these are deprecated you should use BindStringByIndex or give them a name and BindStringByName (by naming each param like :name, :timestamp, etc

>+  rv = statement->Execute();
>+  NS_ENSURE_SUCCESS(rv, rv);

No, you should never ever use synchronous statements, especially in the startup path.
This must use executeAsync.

>+nsresult nsAppStartup::RecordAddonEvent(const PRUnichar *event, nsISupports *details)
>+{
>+  PRTime now = PR_Now();
>+  nsresult rv;
>+
>+  nsCOMPtr<mozIStorageConnection> db;
>+  rv = OpenStartupDatabase(getter_AddRefs(db));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<mozIStorageStatement> statement;
>+  rv = db->CreateStatement(NS_LITERAL_CSTRING("INSERT INTO events VALUES (?1, ?2, ?3, ?4, ?5)"),
>+                           getter_AddRefs(statement));
>+  NS_ENSURE_SUCCESS(rv, rv);

same comments as above

>+
>+  nsCOMPtr<nsIPropertyBag2> bag = do_QueryInterface(details);

you willing to NS_ENSURE_STATE(bag) here
  
>+  nsAutoString id, name, version;
>+  bag->GetPropertyAsAString(NS_LITERAL_STRING("id"), id);
>+  bag->GetPropertyAsAString(NS_LITERAL_STRING("name"), name);
>+  bag->GetPropertyAsAString(NS_LITERAL_STRING("version"), version);

cst to (void) if you explicitly ignore errors

>+  rv = statement->BindInt64Parameter(0, now);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = statement->BindStringParameter(1, id);
>+  NS_ENSURE_SUCCESS(rv, rv); 
...

same as above, deprecated stuff

>+  rv = statement->Execute();
>+  NS_ENSURE_SUCCESS(rv, rv);

same as above

>+nsresult OpenStartupDatabase(mozIStorageConnection **db)

I'd rather make this mozIStorageConnection* GetStartupDBConn(), save the connection in a local property mDBConn and NS_ENSURE_STATE result of this getter.
You most likely want to open only one connection and to close it when done, for performance reasons.

This will require some refactoring though.
The better way to handle this for performance would be:
1. collect information
2. open db connection
3. executeAsync all statements at once
4. close db connection

while you currently open a db connection for each statement, execute statement (synchronously :( ) and let connection die.
Probably (3) is hard to do for addon events, since looks like they can come at any time during app life, but you could close dbConn at profile teardown.

>+{
>+  nsresult rv;

define at first use

>+  nsCOMPtr<nsIFile> file;
>+  rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(file));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = file->Append(NS_LITERAL_STRING("startup.sqlite"));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<mozIStorageService> svc = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = svc->OpenDatabase(file, db);
>+  if (NS_ERROR_FILE_CORRUPTED == rv)
>+  {
>+    svc->BackupDatabaseFile(file, NS_LITERAL_STRING("startup.sqlite.backup"),
>+                            nsnull, nsnull);

you should cast to (void) anything that you explicitly don't error check.
Btw, will you really find useful the corrupt backup? From my experience once you get the corrupt db, it is just unreadable and with low value.
Places got issues like backups created in loop when the file was corrupt in such a way that it was un-movable (due to antivirus often), thus you keep backing up and opening creating tens of backups a day.
We workarounded the issue by checking to not save more than 1 backup per day, but actually data in this startup database don't seem sensible enough to require a backup at all, I'd just delete the corrupt db and make a new one.

>+    rv = svc->OpenDatabase(file, db);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  NS_ENSURE_SUCCESS(rv, rv);

the first NS_ENSURE_SUCCESS is useless, you have replaced rv already, the external one is the only one needed.

>+  rv = EnsureTable(*db,
>+                   NS_LITERAL_CSTRING("duration"),
>+                   NS_LITERAL_CSTRING("timestamp INTEGER, launch INTEGER, startup INTEGER, appVersion TEXT, appBuild TEXT, platformVersion TEXT, platformBuild TEXT"));
>+  NS_ENSURE_SUCCESS(rv, rv);

see above, btw this is not exposed to XPCOM thus it would be much more readable returning the connection pointer rather than a nsresult.

>+  rv = EnsureTable(*db,
>+                   NS_LITERAL_CSTRING("events"),
>+                   NS_LITERAL_CSTRING("timestamp INTEGER, id TEXT, name TEXT, version TEXT, action TEXT"));
>+  NS_ENSURE_SUCCESS(rv, rv);

actually if one table exists it's most likely that the other one exists as well, the EnsureTable helper seems to make everything less readable, and the tables strings should be formatted properly, e.g.
  timestamp INTEGER
, id TEXT
, name TEXT
...
I'd just make this include all needed code for tables check and creation, you probably want to use SetSchemaVersion too to make easier future changes.
I'd not name a column "ID" since it's most likely to be confusing (usually id is considered the primary key of the table, that is the alias for ROWID)

>+nsresult EnsureTable(mozIStorageConnection *db, const nsACString &table,
>+                     const nsACString &schema)
>+{
>+  nsresult rv;
>+  PRBool exists = false;
>+  rv = db->TableExists(table, &exists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!exists)
>+    rv = db->CreateTable(PromiseFlatCString(table).get(),
>+                         PromiseFlatCString(schema).get());

always brace ifs please, per code style

Notice that having a global dbConn will require you to call asyncClose() on it before shutdown.

As a side note, I don't know how often you could get an add-on event, if that's often you could want to have a cached prepared statement too, but could be a followup in case it's found needed.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/components/startup/src/nsAppStartup.h
>--- a/toolkit/components/startup/src/nsAppStartup.h	Sun Oct 24 16:26:19 2010 -0400
>+++ b/toolkit/components/startup/src/nsAppStartup.h	Sun Oct 24 16:24:58 2010 -0500
>@@ -85,6 +85,11 @@
>   PRPackedBool mShuttingDown;   // Quit method reentrancy check
>   PRPackedBool mAttemptingQuit; // Quit(eAttemptQuit) still trying
>   PRPackedBool mRestart;        // Quit(eRestart)
>+
>+  PRTime mRestoredTimestamp;
>+
>+  nsresult RecordStartupDuration();
>+  nsresult RecordAddonEvent(const PRUnichar *event, nsISupports *details);

each one of these needs a javadoc, and properties should be inited in the constructor.

>diff -r df51ba69a839 -r 8ad67554aadd toolkit/content/aboutStartup.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/toolkit/content/aboutStartup.js	Sun Oct 24 16:24:58 2010 -0500
>@@ -0,0 +1,326 @@
>+/* ***** BEGIN LICENSE BLOCK *****

>+ * ***** END LICENSE BLOCK ***** */

take boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ not from other files

>+function displayTimestamp(id, µs) document.getElementById(id).textContent = formatstamp(µs);

there are various entries with this microsecs utf8 text, actually I think you want to rename them to "usecs" and stay ascii


>+function populateMeasurements()
>+{
>+  let s = "SELECT timestamp, launch, startup, appVersion, appBuild FROM duration";
>+  var query = db.createStatement(s);
>+  var lastver, lastbuild;
>+  query.executeAsync({
>+    handleResult: function(results)
>+    {

>+        else
>+          if (lastbuild != build)
>+            options.grid.markings.push(minorMark(stamp, appVersion(version,
>+                                                                   build)));

you should brace stuff like this one

>+    handleCompletion: function()
>+    {

you want to handle the input argument here, that is "reason" for the completion, it's possible execution  fails and you are not handling it

>+function populateEvents()
>+{

>+    handleCompletion: function()
>+    {

ditto

f- because of the sync stuff we are running to populate the tables, those are going to hurt due to fsyncs on the main-thread.
Sorry if I've been a bit verbose :(

Comment 56

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

Just wanted to thank Vlad, Gavin and Marco for their feedback and reviews on this patch.

Daniel: what's the jQuery stuff being used for? Do we really need it?

Comment 57

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

(In reply to comment #54)
> Well, the only thing browser-specific about it is the name of the observer
> notification that we listen for. Every app will have some way of determining
> that startup has ended and the app is ready to use. I guess I could rename it
> to readyTimestamp if you want.

I think that would be better, yeah. Also, it would probably be better to have the app call RecordStartupDuration() itself, rather than having the core code watch for a browser-specific topic.

> It doesn't get called before the timestamp has been set, I've tried it.

It was for me, when about:startup was part of a session being restored.

I still think it would be best to split the addons change into a followup.

Comment 58

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

(In reply to comment #56)
> Daniel: what's the jQuery stuff being used for? Do we really need it?

It's used for the graph.

Comment 59

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

Comment on attachment 485730 [details] [diff] [review]
fixes review comments

Looks like there are a bunch more comments to address, so cancelling the reviews for now.

Comment 60

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 485730 [details] [diff] [review]
fixes review comments

>diff -r bbec3799beb9 -r c6e28dd6326c toolkit/components/startup/src/nsAppStartup.cpp
>@@ -125,6 +136,9 @@
>   NS_TIME_FUNCTION_MARK("Got Observer service");
> 
>   os->AddObserver(this, "quit-application-forced", PR_TRUE);
>+  os->AddObserver(this, "sessionstore-browser-state-restored", PR_TRUE);
>+  os->AddObserver(this, "sessionstore-windows-restored", PR_TRUE);
>+  os->AddObserver(this, "AddonManager-event", PR_TRUE);
>   os->AddObserver(this, "profile-change-teardown", PR_TRUE);
>   os->AddObserver(this, "xul-window-registered", PR_TRUE);
>   os->AddObserver(this, "xul-window-destroyed", PR_TRUE);

I saw that you took out sessionstore-browser-state-restored from the condition, but the observer is still being added.

Comment 61

[Johnathan Nightingale [:johnath]]

Daniel - can we get tryserver builds of the latest code? It's hard to understand what the experience feels like right now based on the screenshot (which is several patch revisions old).

Comment 62

[Daniel Brooks [:db48x]]

(In reply to comment #61)
> Daniel - can we get tryserver builds of the latest code? It's hard to
> understand what the experience feels like right now based on the screenshot
> (which is several patch revisions old).

Certainly. I've run it through the try server twice this week. Use builds from http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c6e28dd6326c/ if you're on a mac, otherwise use http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8ad67554aadd/. Visually, however, not much has changed :)

(In reply to comment #57)
> (In reply to comment #54)
> > Well, the only thing browser-specific about it is the name of the observer
> > notification that we listen for. Every app will have some way of determining
> > that startup has ended and the app is ready to use. I guess I could rename it
> > to readyTimestamp if you want.
> 
> I think that would be better, yeah. Also, it would probably be better to have
> the app call RecordStartupDuration() itself, rather than having the core code
> watch for a browser-specific topic.

I'll gladly change it to a different observer topic, if there is a suitable one. Otherwise, I had been planning on a followup bug specifically to add an app-neutral topic and make Firefox, Thunderbird and Seamonkey all use it.

> > It doesn't get called before the timestamp has been set, I've tried it.
> 
> It was for me, when about:startup was part of a session being restored.

Odd, I can't reproduce this. What is the value of browser.startup.page in that profile?

> I still think it would be best to split the addons change into a followup.

Perhaps, but Marco was able to review the database code without having a separate bug, so I don't think it'll be necessary for the much smaller change to AddonManager.jsm.

Comment 63

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

(In reply to comment #62)
> I'll gladly change it to a different observer topic, if there is a suitable
> one.

The second part of my comment was meant to suggest not using an observer topic at all, but rather having Firefox code call RecordStartupDuration directly (perhaps from one of its browser-state-restored observers). IOW, apps would be required to call RecordStartupDuration themselves to opt-in to the tracking.

I know that it's _possible_ to review everything in this bug, but I don't think it's ideal, and splitting things up just helps get the core parts landed sooner. I won't belabor the point, but (core changes)/(about:startup content+strings)/(graphing+jQuery)/(add-on integration) all seem like separate steps that would be easier to review separately, and only the first two really need to get in to b7.

Comment 64

[u60234]

Comment on attachment 485730 [details] [diff] [review]
fixes review comments

>diff -r bbec3799beb9 -r c6e28dd6326c toolkit/content/aboutStartup.js

>+var series = [{ label: "Launch Time",
>+                data: []
>+              },
>+              { label: "Startup Time",
>+                data: []

These labels needs to be localizable.

>diff -r bbec3799beb9 -r c6e28dd6326c toolkit/locales/en-US/chrome/global/aboutStartup.dtd

>+<!-- LOCALIZATION NOTE (about.startup.duration.ready): The total time
>+that the user had to wait between when they lauched firefox and it was
>+ready to use. -->

lauched -> launched

>diff -r bbec3799beb9 -r c6e28dd6326c toolkit/locales/en-US/chrome/global/aboutStartup.properties

>+# LOCALIZATION NOTE (about.startup.appVersion): %1$S will be
>+# &brandShortName;, %2$S will be the version number, and %$S3 will be

%$S3 -> %3$S

Dates and decimal separators in time stamps are also not using my system locale. Shouldn't they?

Comment 65

[Daniel Brooks [:db48x]]

Attachment: fixed review comments

I don't think I left any of the commends unaddressed.

> take boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ not from
> other files

I did copy it from the boilerplate. I'm not quite sure what the problem could have been. Could you elaborate?

> Dates and decimal separators in time stamps are also not using my system
> locale. Shouldn't they?

Yes, thank you for reminding me. I've put in the code to localize the dates that show up in the tables, but haven't done the same for the ones in the graph. Because of the way the graph automatically shortens the dates based on the extent of the axis, I'm not sure it's currently possible to do. Perhaps Thunderbird has some code that I can reuse, or perhaps I'll have to extend nsIScriptableDateFormat, but either way it can wait for a followup bug.

Comment 66

[Daniel Brooks [:db48x]]

Fresh builds will be showing up in http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-852178715a8c/ over the next few hours. Make sure you delete your old startup.sqlite; I changed a column name.

Comment 67

[Marco Bonardo [:mak]]

(In reply to comment #65)
> > take boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ not from
> > other files
> 
> I did copy it from the boilerplate. I'm not quite sure what the problem could
> have been. Could you elaborate?

Sorry I got confused with another bug, I was just meaning the s/corporation/foundation thing.

Comment 68

[Marco Bonardo [:mak]]

Comment on attachment 487002 [details] [diff] [review]
fixed review comments

Yay! Globally the database situation looks much better.
As the first time, I'll just check database parts and skip all the rest.

>diff -r b0132c16f574 -r 852178715a8c toolkit/components/startup/src/nsAppStartup.cpp

>+  NS_NAMED_LITERAL_CSTRING(insert,
>+                           "INSERT INTO duration                                       \
>+                                        (timestamp, launch, startup, appVersion,       \
>+                                         appBuild, platformVersion, platformBuild)     \
>+                                   VALUES (:timestamp, :launch, :startup, :appVersion, \
>+                                           :appBuild, :platformVersion, :platformBuild)");
>+  nsCOMPtr<mozIStorageAsyncStatement> statement;
>+  rv = db->CreateAsyncStatement(insert, getter_AddRefs(statement));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

nit: I think you can avoid the trailing \ and stay in 80 chars, actually the common style we use is:
rv = db->CreateAsyncStatement(NS_LITERAL_CSTRING(
  "SELECT something "
  "FROM somewhere "
  "WHERE xyz = 1 "
), getter_AddRefs(statement));

>+  nsCOMPtr<mozIStorageBindingParamsArray> parametersArray;
>+  statement->NewBindingParamsArray(getter_AddRefs(parametersArray));
>+  nsCOMPtr<mozIStorageBindingParams> parameters;
>+  parametersArray->NewBindingParams(getter_AddRefs(parameters));
>+
>+  parameters->BindInt64ByName(NS_LITERAL_CSTRING("timestamp"),
>+                              launched);
>+  parameters->BindInt64ByName(NS_LITERAL_CSTRING("launch"),
>+                              started - launched);
...
>+
>+  parametersArray->AddParams(parameters);
>+  statement->BindParameters(parametersArray);

looks like you miss a bunch of error checking here. I know it's boring but probably you want it.
I don't recall if creating binding params is infallible, you could try asking sdwilsh or asuth on irc

>+  nsCOMPtr<mozIStoragePendingStatement> pending;
>+  rv = statement->ExecuteAsync(nsnull, getter_AddRefs(pending));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = statement->Finalize();
>+  NS_ENSURE_SUCCESS(rv, rv);

you should not need to finalize the statement, it will be finalized by executeAsync when it is destroyed

>+  db->Close();

you can't close this way since you have an async statement running, use AsyncClose() instead.
also, error check.


the same comments are valid for RecordAddonEvent, thus I'm not going to repeat them again.


>+nsresult OpenStartupDatabase(mozIStorageConnection **db)
>+{

>+  rv = svc->OpenDatabase(file, db);
>+  if (NS_ERROR_FILE_CORRUPTED == rv)
>+  {
>+    svc->BackupDatabaseFile(file, NS_LITERAL_STRING("startup.sqlite.backup"),
>+                            nsnull, nsnull);
>+    rv = svc->OpenDatabase(file, db);

I still think making a backup of a corrupt database is a useless overkill here.
Plus looks like you are doing a copy of the corrupt db but not removing it, thus the next call to OpenDatabase will again try to open the corrupt one.
Indeed backup executes a copy, not a move.

>+  }
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  (*db)->SetSchemaVersion(1);

make the schema version a STATUP_DATABASE_SCHEMA_VERSION define

>+  rv = EnsureTable(*db,
>+                   NS_LITERAL_CSTRING("duration"),
...
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = EnsureTable(*db,
>+                   NS_LITERAL_CSTRING("events"),
...
>+  NS_ENSURE_SUCCESS(rv, rv);

you probably want to get schema version, and if < 1 ensure tables.
And so on, for any schema change, should be faster then checking each table.
finally set schema version to the current one.
Also, wrapping these 2 in a transaction will save you a fsync on startup and increase safety.


>diff -r b0132c16f574 -r 852178715a8c toolkit/content/aboutStartup.js

>+function displayTimestamp(id, µs) document.getElementById(id).textContent = formatstamp(µs);
>+function displayDuration(id, µs) document.getElementById(id).nextSibling.textContent = formatms(msFromµs(µs));

I still think using non-ascii var names can create issues to future contributions to the same file (bad text editors, bad configs and so on)

>+    handleCompletion: function()
>+    {

you're still ignoring the argument that is aReason, and tells you if the statement execution finished correctly, was canceled or failed.

>+    handleCompletion: function()
>+    {

same here, but maybe that's by design and you're not interested in knowing that?

I think I don't have other comments on the db part, unless any issue comes out later.

f+ with these addressed/answered.

Comment 69

[Daniel Brooks [:db48x]]

(In reply to comment #68)
> >+    handleCompletion: function()
> >+    {
> 
> you're still ignoring the argument that is aReason, and tells you if the
> statement execution finished correctly, was canceled or failed.

Oh, I had meant to comment. I still need to size the graph and set up the axes and so on even when there's a failure, so I'm just putting a message in the table when there is one. That's in handleError() though.

> f+ with these addressed/answered.

Thanks. Gavin, what do you say?

Comment 70

[Daniel Brooks [:db48x]]

whoops, out of time... I'll post the updated patch after I've gotten back from dinner and had time to test it.

Comment 71

[Daniel Brooks [:db48x]]

Attachment: added a transaction, etc

In the end I decided that using a non-ascii identifier (as allowed by the javascript spec) should be fine. No editor I've ever used has had any real problems with utf8 (although it does mess up the syntax highlighting provided by js2-mode in emacs), and well, this is supposed to be the future. I like the result enough that I'm willing to stick by it, accepting the future burden of maintenance it may or may not cause. I hope that this does not upset anyone. I hope also, Marco, that I can still claim your feedback+ even though I've kept it.

Comment 72

[Dave Townsend [:mossop]]

Comment on attachment 488629 [details] [diff] [review]
added a transaction, etc

Shipping jQuery like this makes me a little uncomfortable, marking this so I remember to look into it more on Monday.

Comment 73

[Dave Townsend [:mossop]]

Comment on attachment 488629 [details] [diff] [review]
added a transaction, etc

I'm a little ambivalent on the subject but shaver has said that we shouldn't be shipping jQuery such that extensions can start to rely on it. The problem is that it adds an additional maintenance and compatibility burden on us, for example we wouldn't be able to make stability/security fixes to about:startup that required a new jQuery version unless we were willing perhaps to port the appropriate jQuery fixes to the version we were already using. Perhaps this is a rare case but it's going to be fairly easy to change this to either put jQuery somewhere less exposed (where extension authors shouldn't rely on it) or to perhaps preprocess its code into the appropriate files.

Comment 74

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So I know the Tab Candy team did some porting of jQuery to iQ (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/iq.js)

It's not a drop-in replacement though so there would have to be more work done if that was the path to go down.

Comment 75

[Daniel Brooks [:db48x]]

Hrm. Thunderbird ships a copy of jquery as chrome://messenger/content/jquery.js, but it doesn't seem to have caused them any problems.

Comment 76

[Dave Townsend [:mossop]]

(In reply to comment #75)
> Hrm. Thunderbird ships a copy of jquery as
> chrome://messenger/content/jquery.js, but it doesn't seem to have caused them
> any problems.

Thunderbird are allowed to ship whatever they are comfortable with in their single application, we have to make choices about what ships in every Mozilla based application.

Note that as we are after API freeze you aren't going to be able to modify nsIXULRuntime so you'll have to create a nsIXULRuntime_MOZILLA_2_0 horror.

Who is going to be organising the security review for this feature?

Comment 77

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ugh.  (Not directed at Mossop, just a general ugh.)

1 - If you need to, just copy and paste all of jquery into your JS.  It's ugly, but honestly I'd rather have this data than not have it due to figuring out where jquery lives.  You can probably get away with moving jquery.js into your own content dir; we don't make any promises about any compat there.  Better yet, make it chrome://unstable-mozilla-private/content/jquery.js.  If this needs to go into browser instead of toolkit, that's fine as well.

2 - Let's not create nsIXULRuntime_MOZILLA_2_0.  Put those changes at the end, ask for sr, and let's just get this done.

I understand the need for API freezes and the like; but at the same time, let's ship the product that we actually want to ship, and not the one that we end up shipping because we enforce semi-arbitrary external constraints on ourselves.

Comment 78

[Daniel Brooks [:db48x]]

Comment on attachment 488629 [details] [diff] [review]
added a transaction, etc

Heh, I like that package name. http://hg.mozilla.org/users/db48x_yahoo.com/aboutstartup/rev/97a4aa8ca939

Comment 79

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

can you attach the new chunk as a patch?

Comment 80

[Daniel Brooks [:db48x]]

I'm getting an error when I try to attach anything. I haven't merged this change yet, so all I can attach anyway is the one change anyway. I only have access to my computer via my cellphone at he moment, so trying to do the merge now might be more comedic than useful.

Comment 81

[Daniel Brooks [:db48x]]

Attachment: with vlad's suggestions

sorry, had forgotten to post this

Comment 82

[Dave Townsend [:mossop]]

Comment on attachment 492656 [details] [diff] [review]
with vlad's suggestions

Not a full review (the previous reviews stand) but yes this looks good to me now too, however regarding the changes to the IDLs I have to insist that we follow the current tree rules which means you need to email [e-mail] with risk/reward/probability of extensions using the interface (both JS and C++ extensions) so they can agree to making the changes without creating new interfaces.

Comment 83

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 492656 [details] [diff] [review]
with vlad's suggestions

fwm; mailing release-drivers sounds like a good thing to do.

Comment 84

[Benjamin Smedberg]

Comment on attachment 492656 [details] [diff] [review]
with vlad's suggestions

As noted in r-d emails, we cannot accept .idl changes to nsIAppStartup or nsIXULRuntime now. But since the changes are additive, you should be able to add a new nsIXULRuntime_MOZILLA_2_0.

Comment 85

[Daniel Brooks [:db48x]]

Attachment: change the interface names

Comment 86

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A number of files got the wrong mode, 755 instead of 644; for example <http://hg.mozilla.org/mozilla-central/rev/7ab2c445f7ed> (there's more). Could you please file and fix a followup?

Comment 87

[Timothy Nikkel (:tnikkel)]

When this was backed out we seem to have gotten a 14% improvement on 4 different startup tests. So looks like this probably caused a regression that no email was generated for.

Comment 88

[Daniel Brooks [:db48x]]

(In reply to comment #87)
> When this was backed out we seem to have gotten a 14% improvement on 4
> different startup tests. So looks like this probably caused a regression that
> no email was generated for.

That's bad news. Care to share a link? I don't see anything on the graphs myself... perhaps I'm looking in the wrong place.

(In reply to comment #86)
> A number of files got the wrong mode, 755 instead of 644; for example
> <http://hg.mozilla.org/mozilla-central/rev/7ab2c445f7ed> (there's more). Could
> you please file and fix a followup?

Ah, I didn't notice that, thanks for bringing it up. I've fixed it locally so that it shouldn't be a problem next time around.

Comment 89

[Timothy Nikkel (:tnikkel)]

(In reply to comment #88)
> (In reply to comment #87)
> > When this was backed out we seem to have gotten a 14% improvement on 4
> > different startup tests. So looks like this probably caused a regression that
> > no email was generated for.
> 
> That's bad news. Care to share a link? I don't see anything on the graphs
> myself... perhaps I'm looking in the wrong place.

I was going by the automated emails
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d1cb77606afcda4d#

Comment 90

[Daniel Brooks [:db48x]]

(In reply to comment #89)
> I was going by the automated emails
> http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d1cb77606afcda4d#

Ah, now I see. That is interesting. There's a clear 14% drop on a bunch of machines, but no corresponding jump earlier when I first checked in. One machine dropped 14%, then jumped back to the original level then dropped 14% again. Since my first push didn't cause a regression, I can't take credit for the improvement. Perhaps it was a fluke? Are there any better explanations?

Comment 91

[Dave Townsend [:mossop]]

Have you done a tryserver run of this and used the comparetalos tool to look at the performance numbers? If not might be worth doing that and then also maybe requesting a short tree closure when this lands to ensure clean numbers

Comment 92

[Daniel Brooks [:db48x]]

(In reply to comment #91)
> Have you done a tryserver run of this and used the comparetalos tool to look at
> the performance numbers? If not might be worth doing that and then also maybe
> requesting a short tree closure when this lands to ensure clean numbers

Yes, here's the latest one: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=0fdfcad343c6&newRev=fbb6749d4bdd&tests=ts,ts_cold,ts_shutdown&submit=true

It shows a 30% penalty in the startup test and 9% in ts_cold.


Then I did another test: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=9e71d94198c9&newRev=fbb6749d4bdd&tests=ts,ts_cold,ts_shutdown&submit=true

This compares about:startup to about:startup with an empty startup.sqlite database in the profile. ts is now unaffected (yay!), but ts_cold is still slower by 9%. Unfortunately, that 9% is 700 milliseconds, which is insane. How can reading a 96kb database file take 700 milliseconds?

Also, I haven't been able to reproduce locally the crash (or crashes?) that mochitest-other triggers, so I don't know what that problem is.

Comment 93

[Dave Townsend [:mossop]]

Looks like loading the database happens during the first page loads, maybe it would be better to delay that until a few seconds or even minutes after startup.

Comment 94

[Marco Bonardo [:mak]]

notice that ts_cold has lots of noise, on Places branch we have noticed it is often changing with unrelated changes or even just merges. I'd care more on Ts regression. Also there is probably not much difference between what the db has to do in ts and ts_cold to startup.

Comment 95

[Daniel Brooks [:db48x]]

(In reply to comment #93)
> Looks like loading the database happens during the first page loads, maybe it
> would be better to delay that until a few seconds or even minutes after
> startup.

Does it matter much?

(In reply to comment #94)
> notice that ts_cold has lots of noise, on Places branch we have noticed it is
> often changing with unrelated changes or even just merges. I'd care more on Ts
> regression. Also there is probably not much difference between what the db has
> to do in ts and ts_cold to startup.

Yea, I asked for some extra runs to see if that was the case. It doesn't look like I regressed that at all, now. I'll just add the empty file to the tinderboxes before I check in.

That only leaves the crash(es). Any ideas there? See http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292300353.1292301811.14773.gz&fulltext=1#err6

Comment 96

[Dave Townsend [:mossop]]

(In reply to comment #95)
> (In reply to comment #93)
> > Looks like loading the database happens during the first page loads, maybe it
> > would be better to delay that until a few seconds or even minutes after
> > startup.
> 
> Does it matter much?

It impacts the speed of loading the first pages by however long opening the database takes. Have we any measurements on how long that takes after it has a bunch of data in it?

Comment 97

[Timothy Nikkel (:tnikkel)]

(In reply to comment #95)
> (In reply to comment #94)
> > notice that ts_cold has lots of noise, on Places branch we have noticed it is
> > often changing with unrelated changes or even just merges. I'd care more on Ts
> > regression. Also there is probably not much difference between what the db has
> > to do in ts and ts_cold to startup.
> 
> Yea, I asked for some extra runs to see if that was the case. It doesn't look
> like I regressed that at all, now. I'll just add the empty file to the
> tinderboxes before I check in.

I find it unlikely that four separate tests all improving on the same changeset (the back out of this bug) by about the same percentage (see comment 89) can be blamed on noise. There just aren't that many mails to tree-management about random increases and decreases in these tests for that to seem plausible.

Comment 98

[Mike Hommey [:glandium]]

Comment on attachment 492656 [details] [diff] [review]
with vlad's suggestions

A few comments on the GetLaunchTimestamp function:

+#if defined(XP_UNIX) && !defined(XP_MACOSX)

I doubt AIX and Solaris have /proc/$pid/stat files, not even sure about *BSD. I'd go with a defined(LINUX)

<snip>
+  uptime = fopen("/proc/uptime", "r");
+  fscanf(uptime, "%lld.%lld", &sec, &ssec);
+  PRTime boottime = PR_Now() - (((sec * PR_MSEC_PER_SEC) + (ssec * 10)) * PR_USEC_PER_MSEC);
+  fclose(uptime);
+
+  FILE *pidstat;
+  pid_t pid = getpid();
+  char *statpath = PR_smprintf("/proc/%d/stat", pid);

You can use /proc/self/stat

+  pidstat = fopen(statpath, "r");
+  if (!pidstat)
+    return NS_ERROR_FAILURE;
+
+  char stat[512];
+  memset(&stat, 0, 512);
+  int n = fread(&stat, 1, 511, pidstat);
+  if (n <= 0)
+    return NS_ERROR_FAILURE;
+  fclose(pidstat);
+  PR_smprintf_free(statpath);
+
+  long starttime = 0;
+  sscanf(strrchr(stat, ')') + 2,
+         "%*c %*d %*d %*d %*d %*d %*u %*u %*u %*u "
+         "%*u %*u %*u %*u %*u %*d %*d %*d %*d %lu",
+         &starttime);
+
+  *aTimestamp = cached = boottime + ((starttime / tickspersecond) * PR_USEC_PER_SEC);
+  return NS_OK;

Taras tried something similar and got results sometimes off by hundreds of milliseconds, sometimes even more than half a second.

Comment 99

[(dormant account)]

(In reply to comment #98)
> +  *aTimestamp = cached = boottime + ((starttime / tickspersecond) *
> PR_USEC_PER_SEC);
> +  return NS_OK;
> 
> Taras tried something similar and got results sometimes off by hundreds of
> milliseconds, sometimes even more than half a second.

Yeah on systems that I tried uptime was only precise to ~1second which threw off results a bit. glandium's approach is to track thread/process creation and use new /proc/.../stat relative the old one. That is extremely precise, but is unpleasantly cumbersome. Can be done in a followup.

nothing in /proc/* can be relied on on non-linux.

Comment 100

[(dormant account)]

Daniel thanks a lot for taking on this project. I wasn't aware of this work and was merely planning to reimplement most of this for bug 585196. Really glad to see how far you got.

However there are a few issues with the patch, in addition to points raised by glandium.

(In reply to comment #95)
> (In reply to comment #93)
> > Looks like loading the database happens during the first page loads, maybe it
> > would be better to delay that until a few seconds or even minutes after
> > startup.
> 
> Does it matter much?

This is the first useful ts_cold catch I've seen so far :) Lets avoid the ironic problem of startup-measurement code affecting startup. 
Sqlite is a robust, but expensive way to store flat lists. IO like this should happen on own thread. It should also be delayed by a minute or so after startup. I don't think this can land as is.

I'm also a little concerned about adding a full copy of jquery to the tree. It's great to have startup-graphing functionality(especially on mobile), but would it be possible to write a simple graphing script or use a minified copy of jquery? Perhaps in a followup bug?

Comment 101

[Mark Finkle (:mfinkle) (use needinfo?)]

I am concerned that this could more negatively affect mobile startup and I'd like to not take such a large addition (codesize) too.

Why do we want to save data in sqlite? Why do we need to chart the data? Can't we just display the data in a table? Why not use an add-on to make fancy graphs?

Comment 102

[Daniel Brooks [:db48x]]

Clearly I haven't been checking my email as often as I should over the holidays.

(In reply to comment #98)
> Comment on attachment 492656 [details] [diff] [review]
> with vlad's suggestions
> 
> A few comments on the GetLaunchTimestamp function:
> 
> +#if defined(XP_UNIX) && !defined(XP_MACOSX)
> 
> I doubt AIX and Solaris have /proc/$pid/stat files, not even sure about *BSD.
> I'd go with a defined(LINUX)

Fair enough

(In reply to comment #98)
> Taras tried something similar and got results sometimes off by hundreds of
> milliseconds, sometimes even more than half a second.

Hmm. The worst I've seen so far was about 100ms. Anything that depends on jiffies is going to be pretty imprecise, but at least I figured that nobody would be adjusting their HZ very often, so at least the imprecision should be constant. 


(In reply to comment #99)
> Yeah on systems that I tried uptime was only precise to ~1second which threw
> off results a bit. glandium's approach is to track thread/process creation and
> use new /proc/.../stat relative the old one. That is extremely precise, but is
> unpleasantly cumbersome. Can be done in a followup.

Is that code available somewhere? It sounds like it'd be more likely to work on OSX, where I tried and failed several times to implement this.


(In reply to comment #100)
> Daniel thanks a lot for taking on this project. I wasn't aware of this work and
> was merely planning to reimplement most of this for bug 585196. Really glad to
> see how far you got.

I'm glad I could help. 

(In reply to comment #100)
> (In reply to comment #95)
> > (In reply to comment #93)
> > > Looks like loading the database happens during the first page loads, maybe it
> > > would be better to delay that until a few seconds or even minutes after
> > > startup.
> > 
> > Does it matter much?
> 
> This is the first useful ts_cold catch I've seen so far :) Lets avoid the
> ironic problem of startup-measurement code affecting startup. 

Yes, admittedly that would be ironic. The test just seems so noisy, swinging up and down by the same amount as the 'regression' over multiple runs with the same changeset. If it's a real regression, what's the best way to go about finding the cause?

(In reply to comment #100)
> Sqlite is a robust, but expensive way to store flat lists. IO like this should
> happen on own thread. It should also be delayed by a minute or so after
> startup. I don't think this can land as is.

The only reason I used sqlite is that parsing and serializing JSON takes longer the larger data is, and the third option was to implement a CSV parser. Or perhaps I could have stuffed the data into an ini file... I can always delay the IO, that shouldn't be too difficult. Plus, it's so easy to add more data to the same database in the future.

(In reply to comment #100)
> I'm also a little concerned about adding a full copy of jquery to the tree.
> It's great to have startup-graphing functionality(especially on mobile), but
> would it be possible to write a simple graphing script or use a minified copy
> of jquery? Perhaps in a followup bug?

We've always resisted shipping minified javascript in the past. Writing another graphing script does seem like more work than it ought to be, especially considering the features that this one has. (The way it handles the time axis automatically is really nice, for example.) Perhaps a follow up bug for producing a version of jQuery customized specifically for Firefox, that doesn't do any feature detection, would help?


(In reply to comment #101)
> I am concerned that this could more negatively affect mobile startup and I'd
> like to not take such a large addition (codesize) too.
> 
> Why do we want to save data in sqlite? Why do we need to chart the data? Can't
> we just display the data in a table? Why not use an add-on to make fancy
> graphs?

Well, you know that aphorism as well as I; a picture is worth a thousand words.

There's no technical reason why we can't ship the graphing as an extension, of course, but imagine how sweet it will be when someone asks why their Firefox has become slow, and you can have them visit about:startup and see the shape of the graph. Are there any large jumps? What extensions did you install that day?


So, to sum up, I'll look into delaying the IO a bit and then I'll do a larger run of ts and ts_cold tests. Bigger sample size means better data, right? I'm also getting ready to borrow a test slave from the pool; I really hope I'll be able to reproduce and debug the crash that sometimes happens during mochitest-other on linux/linux64.

Comment 103

[(dormant account)]

> (In reply to comment #99)
> > Yeah on systems that I tried uptime was only precise to ~1second which threw
> > off results a bit. glandium's approach is to track thread/process creation and
> > use new /proc/.../stat relative the old one. That is extremely precise, but is
> > unpleasantly cumbersome. Can be done in a followup.
> 
> Is that code available somewhere? It sounds like it'd be more likely to work on
> OSX, where I tried and failed several times to implement this.

I reworked your startup-measurement stuff in bug 522375 as that is more likely to land without trouble at this point.

Could you share some pointers on how to measure startup on osx?


> Yes, admittedly that would be ironic. The test just seems so noisy, swinging up
> and down by the same amount as the 'regression' over multiple runs with the
> same changeset. If it's a real regression, what's the best way to go about
> finding the cause?

Commenting out parts of the code :)
I'm pretty sure sqlite io was the cause.


> There's no technical reason why we can't ship the graphing as an extension, of
> course, but imagine how sweet it will be when someone asks why their Firefox
> has become slow, and you can have them visit about:startup and see the shape of
> the graph. Are there any large jumps? What extensions did you install that day?

I think that's cool. But a lot of this can be accomplished by having a link in some about: page or dialog that would install the measurement extension for you.

I think that's the way forward with this bug. An extension can evolve much quicker and with less constraints as long as the platform provides the core hooks.

Comment 104

[Daniel Brooks [:db48x]]

(In reply to comment #103)
> I reworked your startup-measurement stuff in bug 522375 as that is more likely
> to land without trouble at this point.

This is cool. I see it tracks more information as well, which is a good thing.

> Could you share some pointers on how to measure startup on osx?

I started by reading the source of ps, but that was a lot less help than it should have been. Then I wrote an implementation using the api function GetTickCount and the information from the process info structure, but I couldn't find any real documentation on how long a tick is (or even if it's a consistent size, and the startup duration generally coming up either negative or in the dozens of seconds when it should have been 2-3. The functions for getting process information are fairly nice, at least, but nothing else about it was.

> > Yes, admittedly that would be ironic. The test just seems so noisy, swinging up
> > and down by the same amount as the 'regression' over multiple runs with the
> > same changeset. If it's a real regression, what's the best way to go about
> > finding the cause?
> 
> Commenting out parts of the code :)
> I'm pretty sure sqlite io was the cause.

Well, I guess I'll be doing some more try server builds then :)

> > There's no technical reason why we can't ship the graphing as an extension, of
> > course, but imagine how sweet it will be when someone asks why their Firefox
> > has become slow, and you can have them visit about:startup and see the shape of
> > the graph. Are there any large jumps? What extensions did you install that day?
> 
> I think that's cool. But a lot of this can be accomplished by having a link in
> some about: page or dialog that would install the measurement extension for
> you.
> 
> I think that's the way forward with this bug. An extension can evolve much
> quicker and with less constraints as long as the platform provides the core
> hooks.

That may be the case, but I was contracted to provide a built-in webpage, so that's what I'm aiming for. Everything else is pretty flexible though, and I'll help in any way that I can.

Comment 105

[Mike Hommey [:glandium]]

Attachment: Small extension for a _very_ basic about:startup

As I needed this, and for the record, here is an extension using the new API from bug 522375 that can be installed on recent Firefox and Fennec nightlies. It just displays the available timings when you go to about:startup.

Comment 106

[Raul Nicolae Malea]

For my build, about:startup not exist. invalid URL.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331

Comment 107

[Raul Nicolae Malea]

(In reply to comment #106)
> For my build, about:startup not exist. invalid URL.
> 
> Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre
> ID:20110119030331

Fișierul nu a fost găsit
 
          Firefox nu găsește fișierul de la jar:file:///C:/Program Files/Minefield/omni.jar!/chrome/toolkit/content/global/aboutStartup.xhtml.

Comment 108

[Mike Hommey [:glandium]]

(In reply to comment #107)
> (In reply to comment #106)
> > For my build, about:startup not exist. invalid URL.
> > 
> > Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre
> > ID:20110119030331
> 
> Fișierul nu a fost găsit
> 
>           Firefox nu găsește fișierul de la jar:file:///C:/Program
> Files/Minefield/omni.jar!/chrome/toolkit/content/global/aboutStartup.xhtml.

Did you install the extension?

Comment 109

[Raul Nicolae Malea]

(In reply to comment #108)
> (In reply to comment #107)
> > (In reply to comment #106)
> > > For my build, about:startup not exist. invalid URL.
> > > 
> > > Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre
> > > ID:20110119030331
> > 
> > Fișierul nu a fost găsit
> > 
> >           Firefox nu găsește fișierul de la jar:file:///C:/Program
> > Files/Minefield/omni.jar!/chrome/toolkit/content/global/aboutStartup.xhtml.
> 
> Did you install the extension?

Ahh! :)) Is an extension? I thought it was implemented in Firefox 4. What is extension link?

Comment 110

[Mike Hommey [:glandium]]

(In reply to comment #109)
> Ahh! :)) Is an extension? I thought it was implemented in Firefox 4. What is
> extension link?

https://bugzilla.mozilla.org/attachment.cgi?id=505049

Comment 111

[Daniel Brooks [:db48x]]

Attachment: wip, some minor things left to do

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=461b12e362da&newRev=86bb9d2091e3&tests=ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true

Comment 112

[Timothy Nikkel (:tnikkel)]

To get a better picture of if your results are significant or not you probably not to compare against more than 1 base rev, maybe 5 or so.

Comment 113

[Daniel Brooks [:db48x]]

Attachment: should be ready to go; I don't think I've forgotten anything

Comment 114

[Daniel Brooks [:db48x]]

Attachment: ready

This should be an easy review. The only substantive changes were to match up with the patch in bug 522375, which checked in the code required to collect the startup data. I also grabbed jQuery 1.5.

Comment 115

[Steffen Wilberg]

I just posted in mozilla.dev.platform about this.

Comment 116

[(no longer active)]

Is there any reason that this patch is bundling non-minified versions of both jquery and flot, in contrast to, let's say, loading them from the web?

Comment 117

[(dormant account)]

(In reply to comment #116)
> Is there any reason that this patch is bundling non-minified versions of both
> jquery and flot, in contrast to, let's say, loading them from the web?

Ted suggested graphing by sending the data to the google chart tools.

Comment 118

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

Comment on attachment 509832 [details] [diff] [review]
ready

I can't see us approving this for Firefox 4.0, as it has string impact and isn't reviewed at this time.

Sorry, Dan - I recognize that there's a lot of good, hard work in this.

Comment 119

[Daniel C]

Could something be included which was equivalent to the very simple extension that Mike attached in comment 105? The current output is:

main	1622
sessionRestored	4049
firstPaint	4815

None of those strings need translating as they're just event names. There won't be any historic data, but for basic diagnosis of horrendous startup times it could help. For example I'm sure I remember seeing Taras or someone comment on a blog post saying that a user's time-to-main was very slow, although I can't find it now.

Comment 120

[Steffen Wilberg]

(In reply to comment #117)
> Ted suggested graphing by sending the data to the google chart tools.

Yeah, have a look at http://code.google.com/apis/ajax/playground/?type=visualization#annotated_time_line

Comment 121

[Daniel Brooks [:db48x]]

(In reply to comment #120)
> (In reply to comment #117)
> > Ted suggested graphing by sending the data to the google chart tools.
> 
> Yeah, have a look at
> http://code.google.com/apis/ajax/playground/?type=visualization#annotated_time_line

I'm really wary of sending this data to third parties; it's a list of the exact dates and times that you opened Firefox.

Comment 122

[Mike Hommey [:glandium]]

(In reply to comment #121)
> (In reply to comment #120)
> > (In reply to comment #117)
> > > Ted suggested graphing by sending the data to the google chart tools.
> > 
> > Yeah, have a look at
> > http://code.google.com/apis/ajax/playground/?type=visualization#annotated_time_line
> 
> I'm really wary of sending this data to third parties; it's a list of the exact
> dates and times that you opened Firefox.

You could send deltas instead. We don't need the absolute times.

Comment 123

[Justin Dolske [:Dolske]]

Comment on attachment 509832 [details] [diff] [review]
ready

Quick drive-by: there are a lot of unrelated whitespace-only changes in nsAppStartup. That makes things harder to review, and generally isn't a good idea to do anyway. Seems like this could all just be JS code anyway...

Comment 124

[Daniel Brooks [:db48x]]

Attachment: no ui

Comment 125

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

Comment on attachment 495794 [details] [diff] [review]
change the interface names

(bookkeeping)

Comment 126

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

Comment on attachment 474262 [details] [diff] [review]
improved the localization notes

(bookkeeping)

Comment 127

[Mike Hommey [:glandium]]

For the record my extension is also available on https://addons.mozilla.org/addon/about-startup and has had various improvements since I attached the initial version to this bug.

Comment 128

[Justin Dolske [:Dolske]]

Comment on attachment 512821 [details] [diff] [review]
no ui

Clearing review request on old bug. Is this still relevant with all the other startup work/telemetry that's been done? If not, let's close this. If it is, thne let's refresh the patch and request review from a specific reviewer to wrap this up.

Comment 129

[Daniel C]

(In reply to Justin Dolske [:Dolske] from comment #128)
> Comment on attachment 512821 [details] [diff] [review]
> no ui
> 
> Clearing review request on old bug. Is this still relevant with all the
> other startup work/telemetry that's been done? If not, let's close this. If
> it is, thne let's refresh the patch and request review from a specific
> reviewer to wrap this up.

I think this is still relevant in terms of users being able to see the data that they are sharing with Mozilla. The startup numbers aren't transmitted via Telemetry as far as I know (rather via one of the daily pings I think) so don't show up in about:telemetry.

Comment 130

[Mike Hommey [:glandium]]

(In reply to Daniel Cater from comment #129)
> I think this is still relevant in terms of users being able to see the data
> that they are sharing with Mozilla. The startup numbers aren't transmitted
> via Telemetry as far as I know (rather via one of the daily pings I think)
> so don't show up in about:telemetry.

I don't think about:telemetry shows them, but they are transmitted via telemetry.

Comment 131

[Justin Dolske [:Dolske]]

So, this bug seems to predate the implementation of Telemetry. Seems like that's where we're going to get most of the value from (and if we need to add more data to Telemetry, then ok).

Is having a per-client record with UI is still worth doing? Especially if it's available as an addon (? comment 127). Bug 597282 seems like a more general solution, too.

Comment 132

[Daniel C]

If these are being sent via Telemetry then showing the data in about:telemetry would suffice I think. I thought about:telemetry showed everything that was being sent. I think it's important from a privacy perspective that it does so.

Comment 133

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.