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)
Tracking
()
RESOLVED
FIXED
mozilla1.1alpha
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
10.41 KB,
patch
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 1•22 years ago
|
||
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.
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Comment 3•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 83270 [details] [diff] [review] proposed fix r=morse with change described in comment 3
Updated•22 years ago
|
Attachment #83270 -
Flags: review+
Assignee | ||
Comment 5•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #83270 -
Attachment description: proposed fix (not yet tested) → proposed fix
Assignee | ||
Comment 6•22 years ago
|
||
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+
Assignee | ||
Comment 7•22 years ago
|
||
This just passes the count argument whether or not it's needed.
Attachment #83270 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
Comment on attachment 83297 [details] [diff] [review] patch, v. 2 r=morse
Attachment #83297 -
Flags: review+
Comment 9•22 years ago
|
||
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+
Assignee | ||
Comment 10•22 years ago
|
||
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
Assignee | ||
Comment 11•22 years ago
|
||
Fix checked in 2002-05-16 11:00 PDT.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 12•22 years ago
|
||
Nice work.
You need to log in
before you can comment on or make changes to this bug.
Description
•