Bug 24114 – ESMP(ECMAScript Mobile Profile) not supported

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Charles Wei]

The ESMP(ECMAScript Mobile Profile) is the OMA standard for Wireless markup scripting language known as ECMAScript Mobile Profile. It's strongly based upon ECMAScript Release 3 (ECMA262), While has some extensions and customizations . This bug report is to make JavascriptCore to support ESMP .

[Charles Wei]

Created attachment 27913 [details]
patch to enable ESMP support : MemoryError  , and Error.code

This is the first patch to enable ESMP support,  this specifically enables a new native error type :  MemoryError , and an new error property:  Error.code. For more detail,  please refer: 

Section 6.12.2 Native Error Types(Constants) 

of

OMA-WAP-ESMP-V1_0-20050614-C

[Charles Wei]

Created attachment 28069 [details]
This patch is to throw URIError for invalid location URI

This patch depends on the last patch. 

This patch is to throw URIError for invalid location URLs, to be compliance with ESMP (OMA-WAP-ESMP-V1_0-20050614-C) , section 9.4, Location Object.

[Charles Wei]

Created attachment 28215 [details]
add an optional parameter "bool caseSensitive" to Element.getAttribute(name), Element.setAttribute(name, value) and Element.removeAttribute(name)

OMA-WAP-ESMP-V1_0-20050614-C has some extension to ECMA-262 , this patch includes the following extensions:

Element.getAttribute(name) ==> Element.getAttribute(name, [Optional] caseSensitive)
Element.setAttribute(name, value) ==>Element.setAttribute(name, value, [Optional] caseSensitive)
Element.removeAttribute(name) ==> Element.removeAttribute(name, [Optional] caseSensitive)

[George Staikos]

*** Bug 23724 has been marked as a duplicate of this bug. ***

[yichao.yin]

Created attachment 30500 [details]
Updated patch for new ESMP error support


As the replacer of Charles' patch(#27913), this patch adds some addtional messages into Charles' patch(id=27913), such as copyright information, bug# for tracing, etc

[yichao.yin]

Created attachment 30501 [details]
Updated patch for ESMP URIError handling

This patch is a replacement for patch(id=28069), includs adding some copyright information, bug# information, and code style change

[yichao.yin]

Created attachment 30502 [details]
Updated patch for ESMP to support casecensitive attribute of element

This patch is a replacement of the patch(id=28215), including some addtional code changes to get suitable for updated WebKit, adding copyright messages, bug# information, etc.

[Eric Seidel (no email)]

Comment on attachment 27913 [details]
patch to enable ESMP support : MemoryError  , and Error.code

Please obsolete old patches when posting new ones.

[yichao.yin]

(In reply to comment #8)
> (From update of attachment 27913 [details] [review])
> Please obsolete old patches when posting new ones.

Eric, I have no the permission to do it. thanks for your help.

[Maciej Stachowiak]

Comment on attachment 30500 [details]
Updated patch for new ESMP error support

Minor style comment:

+#if ENABLE(ESMP)
+        URIError       = 6,
+        MemoryError    = 7
+#else
         URIError       = 6
+#endif

It's ok to leave a trailing comma after the last enum element, so you don't need to duplicate URIError.I'm not a fan of the other code duplicate in the patch, but I don't see an easy way to avoid it.

r- to address the minor style comment. Please repost a patch with that addressed.

[yichao.yin]

(In reply to comment #10)
> (From update of attachment 30500 [details] [review])
> Minor style comment:
> +#if ENABLE(ESMP)
> +        URIError       = 6,
> +        MemoryError    = 7
> +#else
>          URIError       = 6
> +#endif
> It's ok to leave a trailing comma after the last enum element, so you don't
> need to duplicate URIError.I'm not a fan of the other code duplicate in the
> patch, but I don't see an easy way to avoid it.
> r- to address the minor style comment. Please repost a patch with that
> addressed.

Alright, I will fix it soon. Thanks. :)

[Maciej Stachowiak]

Comment on attachment 30501 [details]
Updated patch for ESMP URIError handling

Typo in the ChangeLog:

+        Through URIError for invalid URIs for location, for compliance with ESMP(OMA-WAP-ESMP-V1_0),

This should say "Throw", not "Through".

throwError(exec, URIError, (String("Invalid URI ") + dstUrl).utf8().data());

The .utf8().data() is not appropriate, this makes a temporary char*. Instead, you can just pass a String and rely on WebCore::String's implicit conversion to UString. This error appears in the code twice.

Please address the above comments and repost.

[Maciej Stachowiak]

Comment on attachment 30502 [details]
Updated patch for ESMP to support casecensitive attribute of element

It's unfortunate to do this by adding completely duplicate versions of removeAttribute/getAttribute/setAttribute. Perhaps at least some of these could just have the extra parameter and use of it as an #ifdef but share most of the body.

Also, I don't understand why custom bindings are needed for getAttribute and removeAttribute. Why was that done? The ChangeLog should explain.

Please address these comments and repost.

[yichao.yin]

(In reply to comment #12)
> (From update of attachment 30501 [details] [review])
> Typo in the ChangeLog:
> +        Through URIError for invalid URIs for location, for compliance with
> ESMP(OMA-WAP-ESMP-V1_0),
> This should say "Throw", not "Through".
> throwError(exec, URIError, (String("Invalid URI ") + dstUrl).utf8().data());
> The .utf8().data() is not appropriate, this makes a temporary char*. Instead,
> you can just pass a String and rely on WebCore::String's implicit conversion to
> UString. This error appears in the code twice.
> Please address the above comments and repost.

ok, I will fix it later.

[yichao.yin]

(In reply to comment #13)
> (From update of attachment 30502 [details] [review])
> It's unfortunate to do this by adding completely duplicate versions of
> removeAttribute/getAttribute/setAttribute. Perhaps at least some of these could
> just have the extra parameter and use of it as an #ifdef but share most of the
> body.
> Also, I don't understand why custom bindings are needed for getAttribute and
> removeAttribute. Why was that done? The ChangeLog should explain.
> Please address these comments and repost.

Without custom bindings for getAttribute/removeAttribute,we can't get or remove attributes according to the manner that ESMP defined through javascript. we need JSElement has the two functions binding with those of WebCore.
Of course, code reuse should be considered as much as possible. I will fix it later. 
Thanks!

[yichao.yin]

Created attachment 30648 [details]
Latest patch for ESMP to support casesensitive attribute of element

reconstructed the implmentation, and added more messages in Changelog

[yichao.yin]

Created attachment 30649 [details]
Latest patch to support addtional ESMP Errro type

Addressed the comments of Maciej for patch 30500

[yichao.yin]

Created attachment 30650 [details]
Latest patch for ESMP URIError support

updated as per Maciej's comments for patch 30501

[Eric Seidel (no email)]

Comment on attachment 30648 [details]
Latest patch for ESMP to support casesensitive attribute of element

I think the case sentitive support can be enabled in Element always, w/o guards.  We don't get much from the guards:
5 #if ENABLE(ESMP)
 56     PassRefPtr<Node> removeNamedItem(const String& name, ExceptionCode& code, bool caseSensitive = false);
 57 #else
5458     PassRefPtr<Node> removeNamedItem(const String& name, ExceptionCode&);
 59 #endif

Basically I would remove the guards on all of the internal plumbing.  The only thing which needs guards are the JS bindings.

Why the copies of JSElment getAttribute and removeAttribute?  Those don't seem needed.

r- for the excessive use of #ifdef guards and the code duplication.

[Eric Seidel (no email)]

Comment on attachment 30649 [details]
Latest patch to support addtional ESMP Errro type

This is the wrong way to add code support:
#if !ENABLE(ESMP)
247249     NativeErrorPrototype* evalErrorPrototype = new (exec) NativeErrorPrototype(exec, nativeErrorPrototypeStructure, "EvalError", "EvalError");

The current copy/paste approach using #ifdefs is very error-prone.

Please instead add a mapping from the various error types to their codes.  That mapping code would only be added in ESMP builds.

Please add a LayoutTest to verify that the codes are properly exposed to JavaScript in ESMP builds.

Please add  a LayoutTest to test that memoryError exists.

r- for lack of testing

Some WebKit devs may disagree with me, but I think that it's better to have a few extra values (like the codes) in the common source base, than have copy/paste code to avoid them. :)

[Eric Seidel (no email)]

Comment on attachment 30650 [details]
Latest patch for ESMP URIError support

This needs LayoutTests.

[Robin Qiu]

Created attachment 41280 [details]
patch for ESMP to support casesensitive attribute of element

1. Removed guards.
2. Added test case.

[Robin Qiu]

Created attachment 41281 [details]
patch for ESMP to support casesensitive attribute of element

Made some small modification.

[Robin Qiu]

Created attachment 41479 [details]
patch for ESMP URIError support

If enable ESMP, test case fast/dom/location-new-window-no-crash.html will faild. So, add it into ignore file list when ESMP is enabled.

[Robin Qiu]

Created attachment 41853 [details]
patch for ESMP exception.code support

[Darin Adler]

Comment on attachment 41853 [details]
patch for ESMP exception.code support

It's not right to use a single bug for all ESMP changes. We need to use separate bug report for each feature.

> +#if ENABLE(ESMP)
> +    int code = 0;
> +    if (name == "RangeError")
> +        code = 101;
> +    else if (name == "ReferenceError")
> +        code = 102;
> +    else if (name == "SyntaxError")
> +        code = 103;
> +    else if (name == "TypeError")
> +        code = 104;
> +    else if (name == "URIError")
> +        code = 105;
> +    else if (name == "EvalError")
> +        code = 106;
> +    else if (name == "MemoryError")
> +        code = 99;
> +    putDirect(Identifier(exec, "code"), jsNumber(exec, code), 0);
> +#endif

This is an inefficient way to do this operation. Doing 7 string comparisons to accomplish this doesn't make sense.

I suggest putting this into Error.cpp's Error::create function instead of doing what you're doing here.

It seems quite strange to make ESMP a compile-time switch. Is this really the right way to do this? Who would use the ESMP version in what context? I'd like to know more before taking more of these patches.

[Robin Qiu]

(In reply to comment #26)
> (From update of attachment 41853 [details])
> It's not right to use a single bug for all ESMP changes. We need to use
> separate bug report for each feature.

  Personally, I think puting all patches for ESMP in one bug is convenient to track them.

> 
> > +#if ENABLE(ESMP)
> > +    int code = 0;
> > +    if (name == "RangeError")
> > +        code = 101;
> > +    else if (name == "ReferenceError")
> > +        code = 102;
> > +    else if (name == "SyntaxError")
> > +        code = 103;
> > +    else if (name == "TypeError")
> > +        code = 104;
> > +    else if (name == "URIError")
> > +        code = 105;
> > +    else if (name == "EvalError")
> > +        code = 106;
> > +    else if (name == "MemoryError")
> > +        code = 99;
> > +    putDirect(Identifier(exec, "code"), jsNumber(exec, code), 0);
> > +#endif
> 
> This is an inefficient way to do this operation. Doing 7 string comparisons to
> accomplish this doesn't make sense.

I admit that this is not an efficient way, but considering this function is called  very few times (called by JSGlobalObject::init() only when initializing JS runtime environment ), therefore, I think its efficiency is acceptable.

> 
> I suggest putting this into Error.cpp's Error::create function instead of doing
> what you're doing here.
> 

 Error::create() finally calls NativeErrorPrototype::NativeErrorPrototype() to constructe an error. We did this in your way in the previous patch, but  Eric Seidel thought it's a wrong way.

> It seems quite strange to make ESMP a compile-time switch. Is this really the
> right way to do this? Who would use the ESMP version in what context? I'd like
> to know more before taking more of these patches.

  ESMP support is added for peoples who want to port webkit to a phone, it is similar to WCSS and WML support.

[Robin Qiu]

Created attachment 41935 [details]
patch for ESMP MemoryError support   

* Before testing, please change "sub hasESMPSupport" in "WebKitTools/Scripts/webkitdirs.pm" first: 
  return 0; ==> return 1;

* The test case is time-consuming.

[Oliver Hunt]

Comment on attachment 41281 [details]
patch for ESMP to support casesensitive attribute of element

Following discussions at the TC39 meeting last week, ESMP is basically to be considered dead, so we won't accept ESMP patches to the main webkit tree

[Oliver Hunt]

Comment on attachment 41479 [details]
patch for ESMP URIError support

Following discussions at the TC39 meeting last week, ESMP is basically to be considered dead, so we won't accept ESMP patches to the main webkit tree

[Oliver Hunt]

Comment on attachment 41935 [details]
patch for ESMP MemoryError support   

Following discussions at the TC39 meeting last week, ESMP is basically to be considered dead, so we won't accept ESMP patches to the main webkit tree

[Oliver Hunt]

Following discussions at the TC39 meeting last week, ESMP is basically to be considered dead, so we won't accept ESMP patches to the main webkit tree

For this reason I am closing out this bug as WONTFIX

[George Staikos]

(In reply to comment #32)
> Following discussions at the TC39 meeting last week, ESMP is basically to be
> considered dead, so we won't accept ESMP patches to the main webkit tree
> 
> For this reason I am closing out this bug as WONTFIX

While I don't necessarily disagree with the status, the reason for closing the bug is a different story.  We're trying to determine now if ESMP is actually part of the hard requirements for certain setups.  If so, it doesn't matter how declared dead it is, it's an engineering problem and needs to be solved.  No different than replicating bugs in MS IE.

[George Staikos]

No comment, so reopen until we can verify that it isn't required for proxy compatibility.

[Charles Wei]

This becomes invalid after 3 years opening.