Closed Bug 143918 Opened 22 years ago Closed 22 years ago

IMAGE_CheckForPermission is 0.5% of page load time

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.1alpha

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

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...
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
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.
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 on attachment 83270 [details] [diff] [review]
proposed fix

r=morse with change described in comment 3
Attachment #83270 - Flags: review+
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.
Attachment #83270 - Attachment description: proposed fix (not yet tested) → proposed fix
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.
Attachment #83270 - Flags: needs-work+
Attached patch patch, v. 2 (obsolete) — Splinter Review
This just passes the count argument whether or not it's needed.
Attachment #83270 - Attachment is obsolete: true
Comment on attachment 83297 [details] [diff] [review]
patch, v. 2

r=morse
Attachment #83297 - Flags: review+
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()
Attachment #83297 - Flags: superreview+
Attached patch patch, v. 3Splinter Review
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).
Attachment #83297 - Attachment is obsolete: true
Fix checked in 2002-05-16 11:00 PDT.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Nice work.
Blocks: 71668
QA Contact: tever → nobody
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: