Bug 31293 - Web Inspector: Enable 'console.profile()' and 'console.profileEnd()' regardless of JAVASCRIPT_DEBUGGER
Summary: Web Inspector: Enable 'console.profile()' and 'console.profileEnd()' regardle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-10 07:21 PST by Mikhail Naganov
Modified: 2009-11-17 10:37 PST (History)
9 users (show)

See Also:


Attachments
proposed change (37.75 KB, patch)
2009-11-10 07:25 PST, Mikhail Naganov
no flags Details | Formatted Diff | Diff
proposed change (37.77 KB, patch)
2009-11-10 07:56 PST, Mikhail Naganov
no flags Details | Formatted Diff | Diff
another approach (6.06 KB, patch)
2009-11-12 14:35 PST, Mikhail Naganov
abarth: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
fixed argument conversion in JSC binding (6.19 KB, patch)
2009-11-13 01:01 PST, Mikhail Naganov
pfeldman: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2009-11-10 07:21:57 PST
Currently 'console.profile()' and 'console.profileEnd()' are only available when building against JSC. I want to make them available regardless of engine being used.
Comment 1 Mikhail Naganov 2009-11-10 07:25:10 PST
Created attachment 42864 [details]
proposed change
Comment 2 Mikhail Naganov 2009-11-10 07:56:00 PST
Created attachment 42866 [details]
proposed change

Fixed a small issue
Comment 3 Timothy Hatcher 2009-11-10 13:38:20 PST
Comment on attachment 42866 [details]
proposed change

> +#else
> +    bool profilerEnabled() const { return enabled(); }
>  #endif

This should likely return false, sicne there is no profiler.


>  #if ENABLE(JAVASCRIPT_DEBUGGER)
>      typedef HashMap<unsigned int, RefPtr<JSC::Profile> > ProfilesMap;
>  
>      void startUserInitiatedProfilingSoon();
> -    void toggleRecordButton(bool);
>      void enableDebuggerFromFrontend(bool always);
>      void getProfileHeaders(long callId);
>      void getProfile(long callId, unsigned uid);

Why isn't everything profile related moved out of the if ENABLE(JAVASCRIPT_DEBUGGER) blocks? (Not just this file, other places too.)
Comment 4 Mikhail Naganov 2009-11-11 07:39:36 PST
(In reply to comment #3)
> (From update of attachment 42866 [details])
> > +#else
> > +    bool profilerEnabled() const { return enabled(); }
> >  #endif
> 
> This should likely return false, sicne there is no profiler.
> 

No. Since in this case '{start,stop}UserInitiatedProfiling' will never run.

> 
> >  #if ENABLE(JAVASCRIPT_DEBUGGER)
> >      typedef HashMap<unsigned int, RefPtr<JSC::Profile> > ProfilesMap;
> >  
> >      void startUserInitiatedProfilingSoon();
> > -    void toggleRecordButton(bool);
> >      void enableDebuggerFromFrontend(bool always);
> >      void getProfileHeaders(long callId);
> >      void getProfile(long callId, unsigned uid);
> 
> Why isn't everything profile related moved out of the if
> ENABLE(JAVASCRIPT_DEBUGGER) blocks? (Not just this file, other places too.)

I've tried to make more functions to be available, but this requires more additional changes making patch even more complicated, because there are things like 'toJS', 'toJSDOMWindow', which also need to be done in engine-agnostic way.

As for the 'console.profiles' property, I'm a bit confused, because it doesn't conform to injected script approach at all---it needs to be re-done in 'getProfiles' / 'didGetProfiles' async way, but this breaks the "standard" interface of the 'console' object. This is a problem, actually.

So I would like not to submit this patch as is, and continue with profiling interface generalization in small steps. Also, mind that some of these changes will become two-sided for DevTools, so it's better to keep them simple.
Comment 5 Timothy Hatcher 2009-11-11 08:59:00 PST
What do you mean by "two-sided for DevTools"?

console.profiles is used by layout tests, not by the Inspector.
Comment 6 Mikhail Naganov 2009-11-11 12:08:29 PST
(In reply to comment #5)
> What do you mean by "two-sided for DevTools"?
>

We call "two-sided" patches that consist of a patch to WebKit and a patch to Chromium. The main hassle is that a patch to WK Inspector can break DevTools because it changes some interface, so it may require a "preventive" patch into Chromium which ensures that on WebKit rollout we will not break, and possibly a "cleanup" patch later.

As an example, if I make "InspectorController.profilerEnabled" and friends available regardless of JAVASCRIPT_DEBUGGER, it will break DevTools' assumption that profiler is always enabled, so proper adjustments will be needed to made in our code.

This is the main motivating factor for upstreaming stuff into WebKit. 
 
> console.profiles is used by layout tests, not by the Inspector.

Ah, yes, sorry. I forgot that console is actually accessed from a page, and commands are only passed from Inspector console in order to be evaluated in page's context. So making them accessible will not cause a trouble, right.

But can we make them always available as a next step? Because this requires V8 binding for ScriptProfile to be something more that a simple stub.
Comment 7 Pavel Feldman 2009-11-12 07:07:47 PST
Comment on attachment 42866 [details]
proposed change

We agreed on a simple approach to this. Clearing review flag.
Comment 8 Mikhail Naganov 2009-11-12 14:35:26 PST
Created attachment 43100 [details]
another approach

Pavel, Timothy,

I found that much easier way is to fork 'profile' and 'profileEnd' methods of console. How do you like it?
Comment 9 Timothy Hatcher 2009-11-12 14:54:08 PST
Comment on attachment 43100 [details]
another approach

This seems fine to me.
Comment 10 WebKit Commit Bot 2009-11-12 17:30:32 PST
Comment on attachment 43100 [details]
another approach

Rejecting patch 43100 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11616 test cases.
inspector/console-dir.html -> crashed

Exiting early after 1 failures. 9311 tests run.
490.22s total testing time

9310 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 11 Adam Barth 2009-11-12 18:16:47 PST
Comment on attachment 43100 [details]
another approach

Crash looks like it might be real.
Comment 12 Pavel Feldman 2009-11-12 23:09:15 PST
Comment on attachment 43100 [details]
another approach

> +JSValue JSConsole::profile(ExecState* exec, const ArgList& args)
> +{
> +    ScriptCallStack callStack(exec, args, 1);
> +    impl()->profile(args.at(0).getString(), &callStack);
> +    return jsUndefined();
> +}

I don't think that test crash is relevant, but I think you should have borrowed all the logic from the generated bindings:

ScriptCallStack callStack(exec, args, 1);
const UString& title = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
imp->profile(title, &callStack);

Btw, why do v8 customs not use profile names?
Comment 13 Brian Weinstein 2009-11-12 23:15:21 PST
I've seen console-dir and console-dirxml crash on my machine (on Windows, using run-webkit-tests or just straight DumpRenderTree). I'm on a different one right now, but can post a backtrace to this or a different bug if there is one tracking.
Comment 14 Mikhail Naganov 2009-11-13 01:01:54 PST
Created attachment 43146 [details]
fixed argument conversion in JSC binding

I think 'console dir' test crash is completely unrelated. I ran 'inspector' tests for several times w/o any troubles.

I fixed arguments conversion in JSC binding. As for V8, it currently doesn't afford an ability to tag profiles with custom labels.
Comment 15 WebKit Commit Bot 2009-11-13 01:22:23 PST
Comment on attachment 43146 [details]
fixed argument conversion in JSC binding

Rejecting patch 43146 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11618 test cases.
http/tests/xmlhttprequest/abort-crash.html -> crashed

Exiting early after 1 failures. 9146 tests run.
436.30s total testing time

9145 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
Comment 16 Pavel Feldman 2009-11-13 01:52:04 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/bindings/js/JSConsoleCustom.cpp
	A	WebCore/bindings/v8/custom/V8ConsoleCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/page/Console.idl
Committed r50933
Comment 17 Eric Seidel (no email) 2009-11-13 11:39:36 PST
Filed bug 31480 about the crash.
Comment 18 Laszlo Gombos 2009-11-17 08:31:06 PST
This change broke the JSC builds (e.g. QtWebKit) with JAVASCRIPT_DEBUGGER turned off (this was a supported configuration).

Building QtWebKit with --minimal will result in the following linker error:

WebKitBuildQt/Release/lib/libQtWebKit.so: undefined reference to
`WebCore::JSConsole::profile(JSC::ExecState*, JSC::ArgList const&)'
WebKitBuildQt/Release/lib/libQtWebKit.so: undefined reference to
`WebCore::JSConsole::profileEnd(JSC::ExecState*, JSC::ArgList const&)'


I'm not sure how to resolve this problem.
 - we could either change the semantics of JAVASCRIPT_DEBUGGER so that it only disables the debugger and not the profiler (myself would not prefer this option)
 - revert this change and Chromium can maybe define JAVASCRIPT_DEBUGGER (instead of removing the flags). This is probably not an option either as Chromium will not build in this configuration. 

Maybe we should use the CHROMIUM test for this cases instead of changing the meaning of the JAVASCRIPT_DEBUGGER flag.
Comment 19 Mikhail Naganov 2009-11-17 08:46:24 PST
I think, I can make more fine-grained #ifdefs in JSConsoleCustom, so functions will be there, but they will be empty.

The only drawback is that 'console.profile' and 'console.profileEnd' will present even if 'JAVASCRIPT_DEBUGGER' is turned off. But I think this isn't a problem.
Comment 20 Laszlo Gombos 2009-11-17 09:09:14 PST
I would change the test in Console.idl to 
#if defined(ENABLE_JAVASCRIPT_DEBUGGER) && ENABLE_JAVASCRIPT_DEBUGGER || (defined(PLATFORM_CHROMIUM))

and revert the change made in WebCore/bindings/js/JSConsoleCustom.cpp.
Comment 21 Laszlo Gombos 2009-11-17 09:10:26 PST
We should either REOPEN this bug (I do not seems to have permission to do so) or use this new bug (https://bugs.webkit.org/show_bug.cgi?id=31575) to track this work.
Comment 22 Mikhail Naganov 2009-11-17 10:37:55 PST
Laszlo, your proposal for introducing platform check makes sense. And it will actually suffice---no need to change JSConsoleCustom, because:
 - in case of JSC w/debugger there will be both declaration and definition;
 - in case of JSC w/o debugger there will be no declaration and no definition;
 - in case of Chromium, there will be declaration and definition in V8 bindings (JSC bindings will be ignored);

I will make a patch and test it with Chromium, could you then test it with Qt (as I heard building Qt version of WK is tricky due to a need of bleeding edge glibc)?