Last Comment Bug 499123 - Coalesce browser about: pages
: Coalesce browser about: pages
Status: RESOLVED FIXED
[TSnappiness][ts]
: perf
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 3.6b1
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
:
Mentors:
Depends on:
Blocks: 347680 about:support 506119
  Show dependency treegraph
 
Reported: 2009-06-18 05:47 PDT by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2009-09-25 17:34 PDT (History)
9 users (show)
rflint: in‑testsuite-
rflint: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
WIP (46.06 KB, patch)
2009-06-18 05:47 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Patch (45.89 KB, patch)
2009-07-27 15:41 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
dietrich: review+
Details | Diff | Splinter Review
Patch v2 (46.09 KB, patch)
2009-07-28 15:25 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
gavin.sharp: review+
Details | Diff | Splinter Review
As checked in (51.09 KB, patch)
2009-08-19 23:22 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review

Description User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-06-18 05:47:53 PDT
Created attachment 383910 [details] [diff] [review]
WIP

This combines the various JS nsIAboutModule implementations we have in browser/ into a single C++ implementation. The goal is to reduce component registration costs during startups backed by the fastload cache and trips to disk for those which aren't.
Still trying to get some reliable perf data for this, but in the warm case it looks like it shaves off about 25-40ms.
Comment 1 User image Dietrich Ayala (:dietrich) 2009-06-18 16:04:15 PDT
nice win. there's a bug about "loading js xpcom is unnecessarily slow", but i can't find it, grr.
Comment 2 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-07-27 15:41:42 PDT
Created attachment 390932 [details] [diff] [review]
Patch
Comment 3 User image Dietrich Ayala (:dietrich) 2009-07-28 12:48:02 PDT
Comment on attachment 390932 [details] [diff] [review]
Patch


>+NS_IMETHODIMP
>+AboutRedirector::NewChannel(nsIURI *aURI, nsIChannel **result) 
>+{
>+  NS_ENSURE_ARG_POINTER(aURI);
>+  NS_ASSERTION(result, "must not be null");
>+
>+  nsresult rv;
>+
>+  nsCAutoString path;
>+  rv = aURI->GetPath(path);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 f = path.FindChar('#');
>+  if (f >= 0)
>+    path.SetLength(f);
>+
>+  PRInt32 g = path.FindChar('?');
>+  if (g >= 0)
>+    path.SetLength(g);

can re-use f here?

ditto for GetURIFlags()

>+
>+  ToLowerCase(path);
>+
>+  nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  for (int i = 0; i < kRedirTotal; i++) {
>+    if (!strcmp(path.get(), kRedirMap[i].id)) {
>+      nsCOMPtr<nsIChannel> tempChannel;
>+      rv = ioService->NewChannel(nsDependentCString(kRedirMap[i].url),
>+                                 nsnull, nsnull, getter_AddRefs(tempChannel));
>+      if (NS_FAILED(rv))
>+        return rv;

NS_ENSURE_SUCCESS(rv, rv); for same result in a single line

and ditto for the other instances following

>diff --git a/browser/components/about/Makefile.in b/browser/components/about/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/about/Makefile.in

>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@

nit: indent prob

>-PROT_Application.prototype.getURIFlags = function(uri) {
>-  // We don't particularly *want* people linking to this from
>-  // untrusted content, but given that bad sites can cause this page
>-  // to appear (e.g. by having an iframe pointing to known malware),
>-  // we should code as though this is explicitly possible.
>-  return Ci.nsIAboutModule.ALLOW_SCRIPT |
>-         Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT;
>-}

don't recall if you did port this comment, but please do. and any other comments in the removed code that might be useful.

first pass looks ok, r=me.
Comment 4 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-07-28 15:25:18 PDT
Created attachment 391203 [details] [diff] [review]
Patch v2
Comment 5 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2009-07-28 18:16:53 PDT
Comment on attachment 391203 [details] [diff] [review]
Patch v2

>diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp

>+  { "blocked", "chrome://browser/content/safebrowsing/blockedSite.xhtml",
>+    // We don't particularly *want* people linking to this from untrusted 
>+    // content, but given that bad sites can cause this page to appear (e.g.
>+    // by having an iframe pointing to known malware), we should code as though
>+    // this is explicitly possible.
>+    nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>+    nsIAboutModule::ALLOW_SCRIPT },

This comment doesn't really make sense... URI_SAFE_FOR_UNTRUSTED_CONTENT at its base determines whether the page is linkable. That it also controls whether we give the pages chrome privs is an implementation detail of this specific nsIAboutModule, so if we wanted it to be unlinkable and also not have chrome privs, we could do that.
Comment 6 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2009-08-19 17:17:09 PDT
Comment on attachment 391203 [details] [diff] [review]
Patch v2

>diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp

I think you should use hg cp and maintain the license headers for this file, since it's largely unchanged from the original (same applies to the header). Probably also worth a comment that points to the original, since it's still around.

>+#include "nsStringAPI.h"

this include doesn't appear to be necessary?

>+AboutRedirector::NewChannel(nsIURI *aURI, nsIChannel **result) 

>+  nsCAutoString path;
>+  rv = aURI->GetPath(path);
>+  NS_ENSURE_SUCCESS(rv, rv);
>
>+  PRInt32 f = path.FindChar('#');
>+  if (f >= 0)
>+    path.SetLength(f);
>+
>+  f = path.FindChar('?');
>+  if (f >= 0)
>+    path.SetLength(f);
>+
>+  ToLowerCase(path);

Probably wouldn't hurt to put this in a static method, rather than duplicating in GetURIFlags.

>diff --git a/browser/components/feeds/src/Makefile.in b/browser/components/feeds/src/Makefile.in

>-REQUIRES = xpcom string necko caps js xpconnect mimetype
>+REQUIRES = xpcom string necko xpconnect mimetype

why still xpconnect?

r=me with those and the comment change from comment 5 adressed. For the sake of simplicity, we should probably just leave things as-is and omit the comment, I think. Can file a followup on making about:blocked non-linkable/non-chrome (would mean adding a separate bool to RedirEntry, I guess).
Comment 7 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-08-19 23:22:39 PDT
Created attachment 395522 [details] [diff] [review]
As checked in
Comment 8 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-08-19 23:28:39 PDT
http://hg.mozilla.org/mozilla-central/rev/3e0d12affe42
Comment 9 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-09-25 14:46:02 PDT
Comment on attachment 395522 [details] [diff] [review]
As checked in

a192=beltzner
Comment 10 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-09-25 17:30:02 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b4fc1fe7163

Note You need to log in before you can comment on or make changes to this bug.