245619 - Don't bother converting result of scripts into a string when the caller doesn't care about the value.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

In some cases when we execute script we don't care about the value of the
evaluation, as is the case for execution of top-level scripts, to name one case.
Patch coming up that makes us only spend the time to do the conversion when the
caller really wants the resulting string value.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Don't convert the result to a string if the caller doesn't care about the return value.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 150056 [details] [diff] [review]
Don't convert the result to a string if the caller doesn't care about the return value.

>@@ -931,26 +936,28 @@ nsJSContext::EvaluateString(const nsAStr
>       }
>     }
>   }
> 
>   // Whew!  Finally done with these manually ref-counted things.
>   JSPRINCIPALS_DROP(mContext, jsprin);
> 
>   // If all went well, convert val to a string (XXXbe unless undefined?).
>   if (ok) {
>-    rv = JSValueToAString(mContext, val, &aRetValue, aIsUndefined);
>+    rv = JSValueToAString(mContext, val, aRetValue, aIsUndefined);
>   }

Don't you need to test ok && aRetValue in the if condition?  Wait, strike that
-- you want an inner if around the call to JSValueToAtring.  Unless that
null-checks(?!).

/be

Comment 3

[Johnny Stenback (:jst)]

JSValueToAtring() already does null-check, nsJSContext::ExecuteScript() uses it
too, and it gets a pointer (which can be null) to a string for the retval...

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 150056 [details] [diff] [review]
Don't convert the result to a string if the caller doesn't care about the return value.

Duh, thanks.  r+sr=me.

/be

Comment 5

[Johnny Stenback (:jst)]

Fixed.

Comment 6

[Makoto Kato [:m_kato]]

This check in causes build error in Windows build.

uilding deps for XPCDocument.cpp
/cygdrive/c/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/build/cygwin-wrapper
cl -FoXPCDocument.obj -c -DMOZ_ACTIVEX_PLUGIN_XPCONNECT -DXPC_IDISPATCH_SUPPORT
-D_IMPL_NS_GFX -D_IMPL_NS_MSG_BASE -D_IMPL_NS_WIDGET  -DOSTYPE=\"WINNT5.0\"
-DOSARCH=\"WINNT\"
-I/cygdrive/c/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/embedding/browser/activex/src/plugin
-I/cygdrive/c/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/embedding/browser/activex/src/plugin/../common
-I_xpidlgen -I../../../../../dist/include/xpcom
-I../../../../../dist/include/java -I../../../../../dist/include/plugin
-I../../../../../dist/include/string -I../../../../../dist/include/dom
-I../../../../../dist/include/content -I../../../../../dist/include/widget
-I../../../../../dist/include/gfx -I../../../../../dist/include/js
-I../../../../../dist/include/pref -I../../../../../dist/include/xpconnect
-I../../../../../dist/include/docshell -I../../../../../dist/include/webshell
-I../../../../../dist/include/necko -I../
../../../../dist/include/windowwatcher -I../../../../../dist/include/ax_common
-I../../../../../dist/include/caps -I../../../../../dist/include/string
-I../../../../../dist/include/npmozax -I../../../../../dist/include
-I../../../../../dist/include/nspr          -TP -nologo -W3 -Gy -Fdnpmozax.pdb 
-DNDEBUG -DTRIMMED -O1 -GX -MD            -DX_DISPLAY_MISSING=1
-DMOZILLA_VERSION=\"1.8a2\" -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1
-DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWINVER=0x400 -DSTDC_HEADERS=1
-DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_X86_=1 -DD_INO=d_ino
-DMOZ_DEFAULT_TOOLKIT=\"windows\" -DMOZ_PHOENIX=1 -DMOZ_XUL_APP=1
-DMOZ_APP_NAME=\"firefox\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1
-DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DMOZ_MATHML=1
-DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1
-DMOZ_BYPASS_PROFILE_AT_STARTUP=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1
-DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 -DMOZILLA_LOCALE_VERSION=\"1.8a\"
 -DMOZILLA_REGION_VERSION=\"1.8a\" -DMOZILLA_SKIN_VERSION=\"1.5\" 
-D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT
/cygdrive/c/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/embedding/browser/activex/src/plugin/XPCDocument.cpp
XPCDocument.cpp
c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/embedding/browser/activex/src/plugin/XPCDocument.cpp(848)
: error C2664: 'EvaluateString' : cannot convert parameter 7 from 'class
nsAutoString' to 'class nsAString *'
        No user-defined-conversion operator available that can perform this
conversion, or the operator cannot be called
make[6]: *** [XPCDocument.obj] Error 2

Comment 7

[Makoto Kato [:m_kato]]

Attachment: patch for build error

Comment 8

[Makoto Kato [:m_kato]]

Attachment: patch for build error partt2 (for webshell/tests/viewer)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 150529 [details] [diff] [review]
patch for build error

d'oh!

/be

Comment 10

[Makoto Kato [:m_kato]]

checked in

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 150529 [details] [diff] [review]
patch for build error

Index: XPCDocument.cpp
...
					    url.get(),	 // url
					    1,		 // line no
					    nsnull,
-					    result,
+					    &result,

Thanks for fixing my bustage! Though "result" is no longer needed at all here
now, so I just landed a change that removes it, and passes in nsnull in stead
(since noone cares about the result, which was the whole point with this
change...).