[webkit-reviews] review denied: [Bug 36839] Web Inspector: Adds the ability to get the function symbol name when looking up call location. : [Attachment 52062] Forgot to update ChangeLog for LayoutTests. New patchset with update.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 36839: Web Inspector: Adds the ability to get the function symbol name when
looking up call location.
https://bugs.webkit.org/show_bug.cgi?id=36839

Attachment 52062: Forgot to update ChangeLog for LayoutTests. New patchset with
update.
https://bugs.webkit.org/attachment.cgi?id=52062&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> -    if (!V8Proxy::sourceName(*sourceName) ||
!V8Proxy::sourceLineNumber(*sourceLineNumber))
> +    if (!V8Proxy::sourceName(*sourceName) ||
!V8Proxy::sourceLineNumber(*sourceLineNumber) || !V8Proxy::funcName(*funcName))


This looks very inefficient - we should try fetching all at a time.

> +    DEFINE_STATIC_LOCAL(const char*, frameSourceLine,
>	   ("function frameSourceLine(exec_state) {"
>	   "  return exec_state.frame(0).sourceLine();"

This looks like a very old WebKit. Recent ones require	checks for frame count.


> +bool V8Proxy::funcName(String& result)
> +{

I think we are going to have a bunch of methods like this and I am not sure we
should put them into V8Proxy. There is a nice new ScriptDebugServer class in
bindings that abstracts us from JS engine. It is nearly blank now, but we are
working on filling it. I think all three methods should be defined there and it
should own the utility context business as a whole. It would give us more
flexibility and we'll be able to fetch all the info you need within single call
(such as top stack frames, etc.). We should move ScriptCallStack::callLocation
there as well.