Closed Bug 650494 Opened 13 years ago Closed 13 years ago

Remove nsIXULPrototypeCache

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: khuey, Assigned: marco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

54 /**
55  * This interface lets code from outside gklayout access the prototype cache.
56  */

That's no longer necessary.
Assignee: nobody → 46b
Attached patch Removed nsIXULPrototypeCache (obsolete) — Splinter Review
Attachment #550266 - Flags: review?(khuey)
Comment on attachment 550266 [details] [diff] [review]
Removed nsIXULPrototypeCache

Review of attachment 550266 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good.  A few comments:

::: content/xul/document/public/nsIXULPrototypeCache.h
@@ -84,5 @@
> -// bytecode version changes.
> -#define XUL_FASTLOAD_FILE_VERSION       (0xfeedbeef - 25)
> -
> -#define XUL_SERIALIZATION_BUFFER_SIZE   (64 * 1024)
> -#define XUL_DESERIALIZATION_BUFFER_SIZE (8 * 1024)

Are these constants not used anywhere?

::: content/xul/document/src/nsXULPrototypeCache.cpp
@@ +107,5 @@
>  }
>  
>  
>  NS_IMPL_THREADSAFE_ISUPPORTS2(nsXULPrototypeCache,
> +                              nsISupports,

You don't need to explicitly list nsISupports.  You should change this to

NS_IMPL_THREADSAFE_ISUPPORTS1(nsXULPrototypeCache, nsIObserver)

We could probably drop the THREADSAFE bit too, but there's no need to do that here.

@@ +119,5 @@
>      if (aOuter)
>          return NS_ERROR_NO_AGGREGATION;
>  
>      nsRefPtr<nsXULPrototypeCache> result = new nsXULPrototypeCache();
> +    if (!result)

Don't add in these irrelevant whitespace changes here.

@@ +162,2 @@
>      if (!sInstance) {
> +        sInstance = new nsXULPrototypeCache();

You will need to AddRef this, and Release it somewhere appropriate (from nsLayoutStatics::Shutdown, perhaps).

::: content/xul/document/src/nsXULPrototypeCache.h
@@ +72,5 @@
>   * The cache has two levels:
>   *  1. In-memory hashtables
>   *  2. The on-disk cache file.
>   */
> +class nsXULPrototypeCache : public nsISupports,

Again, no need to explicitly list nsISupports.
Assignee: 46b → mar.castelluccio
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch Removed nsIXULPrototypeCache v2 (obsolete) — Splinter Review
Added AddRef in nsXULPrototypeCache::GetInstance.
Release is yet done in nsXULPrototypeCache::ReleaseGlobals, called by nsLayoutStatics::Shutdown.
The constant weren't used anywhere (at least this is what mxr says).

If you could, please review this today or tomorrow, because Sunday I'll set off on a journey.
Attachment #550266 - Attachment is obsolete: true
Attachment #550266 - Flags: review?(khuey)
Attachment #551085 - Flags: review?(khuey)
khuey if the patch I've presented is good, could you checkin it? I won't be here until 14 August, I wouldn't like in the meantime it will be bitrotted
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

>+        sInstance->AddRef();

This should be NS_ADDREF(sInstance);

Other than that this looks good to me.  I'm not a peer of this stuff though, so I'm going to ask smaug to do the official review.
Attachment #551085 - Flags: review?(khuey)
Attachment #551085 - Flags: review?(Olli.Pettay)
Attachment #551085 - Flags: feedback+
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

> nsXULPrototypeCache::GetInstance()
> {
>-    // Theoretically this can return nsnull and callers should handle that.
>     if (!sInstance) {
>-        nsIXULPrototypeCache* cache;
>-
>-        CallGetService(kXULPrototypeCacheCID, &cache);
>-
>-        sInstance = static_cast<nsXULPrototypeCache*>(cache);
>+        sInstance = new nsXULPrototypeCache();
>+        sInstance->AddRef();
NS_ADDREF(sInstance = new nsXULPrototypeCache());


>-class nsXULPrototypeCache : public nsIXULPrototypeCache,
>-                                   nsIObserver
>+class nsXULPrototypeCache : public nsIObserver
> {
> public:
>     // nsISupports
>     NS_DECL_ISUPPORTS
>     NS_DECL_NSIOBSERVER
> 
>     // nsIXULPrototypeCache
Drop this comment


>     virtual PRBool IsCached(nsIURI* aURI) {
You can make the methods which were defined in nsIXULPrototypeCache non-virtual.
Attachment #551085 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

Actually, this patch has a very serious bug.  There's a lot of logic in NS_NewXULPrototypeCache which is no longer getting run.  The browser won't start with this patch ...
Attachment #551085 - Flags: feedback+ → review-
NS_NewXULPrototypeCache seems unused. It was used only in nsLayoutModule.cpp, but with the patch we've removed this use.
(In reply to Marco Castelluccio from comment #8)
> NS_NewXULPrototypeCache seems unused. It was used only in
> nsLayoutModule.cpp, but with the patch we've removed this use.

Yes, and that's the problem.  Before your patch, the prototype cache was created through XPCOM, which called NS_NewXULPrototypeCache.  That runs the code starting at http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULPrototypeCache.cpp#128 which initializes the prototype cache, sets up observers, etc.  With your patch, we don't create the prototype cache through that function, so this code never gets run.

You need to move that logic in NS_NewXULPrototypeCache into the construction case nsXULPrototypeCache::GetInstance.  It'd actually probably neater to split the hashtable initializations into nsXULPrototypeCache's constructor and put the rest in NS_NewXULPrototypeCache, but that's the minimum that's necessary.

You can test this pretty easily, with your patch as is the browser doesn't even start.
Attached patch Removed nsIXULPrototypeCache v3 (obsolete) — Splinter Review
Moved the XULPrototypeCache creation from NS_NewXULPrototypeCache to the constructor. Removed NS_NewXULPrototypeCache.

I haven't done the checks for the memory, because the cache is created at the start, so they should be useless.
Attachment #551085 - Attachment is obsolete: true
Attachment #552916 - Flags: review?(khuey)
Comment on attachment 552916 [details] [diff] [review]
Removed nsIXULPrototypeCache v3

This looks good.  I'll throw it at the tryserver before landing.
Attachment #552916 - Flags: review?(khuey) → review+
Try run for 1297abb8c3c6 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=1297abb8c3c6
Results (out of 167 total builds):
    exception: 2
    success: 124
    warnings: 41
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/khuey@mozilla.com-1297abb8c3c6
So, there's unfortunately still a problem here.

The addons manager tries to get the prototype cache service at:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#964

It does this just to ensure that the prototype cache is running before it flushes.  I don't think we need this, because if the cache is not alive then there's nothing to flush.  CCing mwu to see if he knows.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> I don't think we need this, because if the cache is not alive then
> there's nothing to flush.  CCing mwu to see if he knows.

Sounds about right to me.

This looks like a relic from when the prototype cache used fastload. Starting the prototype cache was necessary to clear fastload. Now that the prototype cache uses startupcache, we can clear everything the prototype cache uses without starting the prototype cache.
Attached patch Remove nsIXULPrototypeCache v4 (obsolete) — Splinter Review
Added the needed change. I've also sent the patch to try, it will comment here.
Attachment #552916 - Attachment is obsolete: true
Try run for cf2ed2ac2e2d is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=cf2ed2ac2e2d
Results (out of 170 total builds):
    exception: 2
    success: 153
    warnings: 15
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mar.castelluccio@studenti.unina.it-cf2ed2ac2e2d
Corrected commit message. Now this could be checked in, as the try build was successful.
Attachment #555922 - Attachment is obsolete: true
Keywords: checkin-needed
In my queue, pushing to inbound once confirmed everything builds locally.
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/728cf3529cba
Flags: in-testsuite-
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/728cf3529cba
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: