Closed Bug 146116 Opened 22 years ago Closed 22 years ago

Image.prototype.foo not delegated to by (new Image).foo

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.2alpha

People

(Reporter: brendan, Assigned: peterv)

References

Details

Attachments

(2 files, 2 obsolete files)

We need Image.prototype to refer to HTMLImageElement.prototype.

/be
Keywords: 4xp, mozilla1.1
Peterv might be taking this bug sometime soon... We should do the same for
Option.prototype too.
Target Milestone: --- → mozilla1.1alpha
Taking.
Assignee: jst → peterv
Priority: -- → P3
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
I added a new category (JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_CATEGORY) that
allows to specify the prototype that a global constructor should share.
Comment on attachment 93405 [details] [diff] [review]
v1

+#define JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_CATEGORY \
+  "JavaScript global constructor prototype"

Wouldn't it make things clearer if the word "alias" was somewhere in that macro
name?

+    } else if (name_struct->mType ==
nsGlobalNameStruct::eTypeConstructorAlias) {
+      nsGlobalNameStruct* alias_struct =
+	 gNameSpaceManager->GetConstructorProto(name_struct);
+      if (alias_struct) {
+	 if (alias_struct->mType == nsGlobalNameStruct::eTypeClassConstructor)
{
...

Here you make sure GetConstructorProto doesn't return null.

@@ -3519,11 +3534,15 @@

     JSObject *dot_prototype = nsnull;

+    if (name_struct->mType == nsGlobalNameStruct::eTypeConstructorAlias) {
+      name_struct = gNameSpaceManager->GetConstructorProto(name_struct);
+    }
+
     if (name_struct->mType == nsGlobalNameStruct::eTypeClassConstructor) {
...

but here you don't. Wouldn't you want a if (!name_struct) check inside that
first if check?

- In nsScriptNamespaceManager.cpp

...
+	   s->mAlias = new nsGlobalNameStruct::ConstructorAlias;
+	   if (!s->mAlias) {
+	     // Free entry
+	     (void) PL_DHashTableOperate(&mGlobalNames,
+					 &NS_ConvertASCIItoUCS2(categoryEntry),
+					 PL_DHASH_REMOVE);
+	     return NS_ERROR_OUT_OF_MEMORY;
+	   }

What's up with that silly (void) before the function call? :-) That's not the
norm in the dom code :-)

Other than that, sr=jst
Attachment #93405 - Flags: superreview+
Attached patch v1.1 (obsolete) — Splinter Review
*** Bug 138736 has been marked as a duplicate of this bug. ***
Comment on attachment 95990 [details] [diff] [review]
v1.1

>@@ -201,6 +224,35 @@
>       rv = NS_OK;
> 
>       continue;
>+    }
>+
>+    if (aType == nsGlobalNameStruct::eTypeExternalConstructor) {

This is probably me not fully understanding everything, but why do you only
want to do this for aType == nsGlobalNameStruct::eTypeExternalConstructor?

>+      nsXPIDLCString constructorProto;
>+      rv = aCategoryManager->GetCategoryEntry(JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY,
>+                                              categoryEntry,
>+                                              getter_Copies(constructorProto));
>+      if (NS_SUCCEEDED(rv)) {
>+        nsGlobalNameStruct *s = AddToHash(NS_ConvertASCIItoUCS2(categoryEntry));
>+        NS_ENSURE_TRUE(s, NS_ERROR_OUT_OF_MEMORY);
>+
>+        if (s->mType == nsGlobalNameStruct::eTypeNotInitialized) {
>+          s->mAlias = new nsGlobalNameStruct::ConstructorAlias;
>+          if (!s->mAlias) {
>+            // Free entry
>+            PL_DHashTableOperate(&mGlobalNames,
>+                                 &NS_ConvertASCIItoUCS2(categoryEntry),
>+                                 PL_DHASH_REMOVE);
>+            return NS_ERROR_OUT_OF_MEMORY;
>+          }
>+          s->mType = nsGlobalNameStruct::eTypeConstructorAlias;
>+          s->mAlias->mCID = cid;
>+          s->mAlias->mProtoName.AssignWithConversion(constructorProto);
>+          s->mAlias->mProto = nsnull;
>+        } else {
>+          NS_WARNING("Global script name not overwritten!");
>+        }
>+        return NS_OK;
>+      }

Do you really want to return here? shouldn't you keep looping?



Still have some parts of the patch to go...
make sure that the code around
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3591

doesn't need touching (i don't think it does). A better name for the new enum
would probably be eTypeExternalConstructorAlias or eTypeExternalAliasedConstructor

Other then that it looks fine
Comment on attachment 95990 [details] [diff] [review]
v1.1

>-  return catman->AddCategoryEntry(JAVASCRIPT_GLOBAL_CONSTRUCTOR_CATEGORY,
>-                                  "Image", NS_HTMLIMGELEMENT_CONTRACTID,
>+  nsresult rv = catman->AddCategoryEntry(JAVASCRIPT_GLOBAL_CONSTRUCTOR_CATEGORY,
>+                                         "Image", NS_HTMLIMGELEMENT_CONTRACTID,
>+                                         PR_TRUE, PR_TRUE, getter_Copies(previous));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return catman->AddCategoryEntry(JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_CATEGORY,
>+                                  "Image", "HTMLImageElement",
>                                   PR_TRUE, PR_TRUE, getter_Copies(previous));

This should be JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY (same of
option)
Attached file testcase
> make sure that the code around
> http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3591

I doesn't because of

+    if (name_struct->mType == nsGlobalNameStruct::eTypeConstructorAlias) {
+      name_struct = alias_struct;
+    }
Attached patch v1.2Splinter Review
Use eTypeExternalConstructorAlias instead of eTypeConstructorAlias, use
JAVASCRIPT_GLOBAL_CONSTRUCTOR_PROTO_ALIAS_CATEGORY everywhere, continue instead
of return in nsScriptNameSpaceManager::FillHash, always return NS_OK at the end
of nsScriptNameSpaceManager::FillHash instead of returning rv and resetting it
where the errorcode shouldn't leak to the caller.
Attachment #95990 - Attachment is obsolete: true
Comment on attachment 97237 [details] [diff] [review]
v1.2

Carry over sr=jst.
Attachment #97237 - Flags: review+ → superreview+
Comment on attachment 97237 [details] [diff] [review]
v1.2

[collision] r=sicking
Attachment #97237 - Flags: review+
this broke the SunOS port:

"/builds/tinderbox/SeaMonkey/SunOS_5.7_Depend/mozilla/dom/src/build/nsScriptNameSpaceManager.cpp",
line 240: Error: The "&" operator can only be applied to a variable or other
l-value.
Checked in. (bustage on SunOS fixed too)
Status: ASSIGNED → RESOLVED
Closed: 22 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: