143918 - IMAGE_CheckForPermission is 0.5% of page load time

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

IMAGE_CheckForPermission is 0.5% of page load time:

84001   0       59 IMAGE_CheckForPermission(char const*, char const*, int*)
                 27 CKutil_Localize(unsigned short const*)
                 16 nsTextFormatter::smprintf(unsigned short const*, ...)
                  9 nsPref::GetBoolPref(char const*, int*)
                  5 nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&,
nsID const&)
                  1 nsStringBundleService::CreateBundle(char const*,
nsIStringBundle**)
                  1 PR_Free


Where we have:

 84023   0       27 CKutil_Localize(unsigned short const*)
                 10 nsStringBundle::GetStringFromName(unsigned short const*,
unsigned short**)
                  8 nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&,
nsID const&)
                  7 nsStringBundleService::CreateBundle(char const*,
nsIStringBundle**)
                  2 nsAutoString::nsAutoString()

Some minor patches coming up...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I will attach a fix that:

1. stores the pref and sets up a callback (as well as removing some set/get
functions around globals that really seem unnecessary (in terms of code size and
performance) since the globals are accessible anyway), and
2. changes Check_Permission so that the string to access the string bundle is
passed in so that we only need to mess with string bundles if we actually need
to prompt.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: proposed fix

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, if I want to keep things exactly as the old code did, I should make
kBlockerPrefDefault PR_FALSE, which I'll do in my tree now, but I won't bother
with a new patch.

Comment 4

[Stephen P. Morse]

Comment on attachment 83270 [details] [diff] [review]
proposed fix

r=morse with change described in comment 3

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tested this patch by:
 * testing the three variants of the image pref on http://www.cnn.com/  "Accept
only images from the same site" didn't seem to be working quite right, but it
was accepting some images and not others, and I didn't change anything related
to that.  Other than that, the three prefs worked fin.
 * Tested image blocking, which still works.
 * Testing that the pref to turn off image blocking makes the blocked images no
longer be blocked.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 83270 [details] [diff] [review]
proposed fix

Oops, I just tested the cookie confirmation dialogs, and realized that I missed
the |count| parameter for the PermissionToSetSecondCookie warning.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 2

This just passes the count argument whether or not it's needed.

Comment 8

[Stephen P. Morse]

Comment on attachment 83297 [details] [diff] [review]
patch, v. 2

r=morse

Comment 9

[Alec Flett]

Comment on attachment 83297 [details] [diff] [review]
patch, v. 2

excellent, except I don't understand why you're dropping the &rv part in
do_GetService 
for prefs - instead of dropping it, can we at least do some basic error
checking instead?

Here:

>-  nsresult rv;
>-  nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));
>+  nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID));
>   if (NS_FAILED(prefs->GetIntPref(image_behaviorPref, &n))) {

just make this if (NS_SUCCEEDED(rv) && ...)

similar thing here:

>-  nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));
>+  nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID));
>   if (NS_FAILED(prefs->GetBoolPref(image_warningPref, &x))) {


and while we're adding new code:

>+
>+PR_STATIC_CALLBACK(int)
>+image_BlockerPrefChanged(const char * newpref, void * data) {
>+  PRBool x;
>+  nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID));
>+  if (NS_FAILED(prefs->GetBoolPref(image_blockerPref, &x))) {


>+  Recycle(message_fmt);

Ack, I thought Recycle() was deprecated?

sr=alecf with the do_GetService's error checked, let me know what you end up
doing with Recycle()

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, v. 3

This changes |Recycle| to |nsMemory::Free| (which is what Recycle does,
although it seems like CKutil_Localize might be using a different allocator in
its failure case) and it null-checks the pref service pointers (since the
|nsCOMPtr_helper| for |do_GetService| will ensure null on failure, and there's
no point getting the rv if we can't propagate it).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2002-05-16 11:00 PDT.

Comment 12

[Adam D. Moss]

Nice work.