Summary: | [JSC] Web Inspector: implement breaking from native callback | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, charles.wei, dglazkov, efidler, haraken, japhet, jochen, joepeck, kpiascik, loislo, ossy, PeterHWang, pfeldman, webkit.review.bot, yurys | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | 88780 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Pavel Podivilov
2010-08-02 02:18:42 PDT
JSC's ScriptDebugServer::breakProgram() needs implementation. The original bug for DOM Breakpoints was: https://bugs.webkit.org/show_bug.cgi?id=42886 "This method should emulate stopping on a breakpoint, run the message loop and deliver backtrace to the front-end. Basically repeat the original breakpoint loop routine." How is this used in the Inspector? (So the people implementing it can test it.) (In reply to comment #4) > How is this used in the Inspector? (So the people implementing it can test it.) 1. In Settings.js set Preferences.nativeInstrumentationEnabled = true (currently is false). 2. In Elements panel, invoke context menu on some node and enable DOM breakpoints. 3. Do something that makes events fire Expected: JS is stopped, Scripts panel shown, stack shows JS stack that led to the DOM mutation. Created attachment 145036 [details]
Patch
Attachment 145036 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 145039 [details]
Patch
Comment on attachment 145039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145039&action=review > Source/WebCore/ChangeLog:10 > + No test. No impact on others interface of JSC. There is a bunch of layout tests in LayouTests/inspector/debugger disabled for JSC because of this bug. You should enable them if the functionality is now fixed. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:158 > + m_paused = true; This is not what we expect this method to do, you should run message loop here until the execution is resumed like in case if a regular JS breakpoint. Probably the following code will work: m_pauseOnNextStatement = true; pauseIfNeeded(m_currentCallFrame->dynamicGlobalObject()); > Source/WebCore/bindings/js/ScriptDebugServer.h:88 > + bool supportsNativeBreakpoints() { return true; } This method now can be removed from both Source/WebCore/bindings/{js,v8}/ScriptDebugServer.h as it returns the same value. Created attachment 145547 [details]
Patch
Comment on attachment 145547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145547&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:155 > + if (isPaused() || !m_currentCallFrame) please use m_paused like the rest of the class instead of isPaused() > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:121 > void InspectorDebuggerAgent::supportsNativeBreakpoints(ErrorString*, bool* result) can we not get rid of this method alltogether? Is it used for something else? Comment on attachment 145547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145547&action=review >> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:121 >> void InspectorDebuggerAgent::supportsNativeBreakpoints(ErrorString*, bool* result) > > can we not get rid of this method alltogether? Is it used for something else? Now that we consider it supported by all ports, it should go away along with its declaration in the protocol (Inspector.json) and its usage in the front-end. Make sure inspector people take a look at this change prior to landing once all the usages are removed. Comment on attachment 145547 [details] Patch Attachment 145547 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12900220 New failing tests: http/tests/media/media-source/video-media-source-event-attributes.html Created attachment 145583 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 145547 [details]
Patch
r- per comments above
Created attachment 145688 [details]
Patch
Yury, Konrad, and Pavel, can we do it by two steps? Firstly, make sure things of JSC Ok in this bug. Secondly, open a new bug to remove the common code in v8 and inspector. (In reply to comment #17) > Yury, Konrad, and Pavel, can we do it by two steps? > Firstly, make sure things of JSC Ok in this bug. Secondly, open a new bug to remove the common code in v8 and inspector. Sounds good to me, just make sure you file the new bug first and refer to it from the change via // FIXME: <bug link> Remove this method. Comment on attachment 145688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145688&action=review > Source/WebCore/bindings/js/ScriptDebugServer.h:88 > + bool supportsNativeBreakpoints() { return true; } Please put fixme here. Created attachment 146796 [details]
Patch
Comment on attachment 146796 [details] Patch Clearing flags on attachment: 146796 Committed r119962: <http://trac.webkit.org/changeset/119962> All reviewed patches have been landed. Closing bug. These tests still fail on Qt: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119968%20%2838549%29/results.html (In reply to comment #23) > These tests still fail on Qt: > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119968%20%2838549%29/results.html I made a little digging. This patch only enabled this tests on Qt, they are still skipped on other platforms: - wincairo/Skipped - mac/Skipped - win/Skipped - gtk/TestExpectations (but here all inspector tests are skipped because of an another bug) inspector/debugger/dom-breakpoints.html ---------------------------------------- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/dom-breakpoints-pretty-diff.html The results seems correct, but the platform independent (JSC specific expected file wasn't updated long time ago, only the chromium specific) inspector/debugger/event-listener-breakpoints.html --------------------------------------------------- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/event-listener-breakpoints-pretty-diff.html It seems correct, but we need JSC and V8 specific expected files. inspector/debugger/step-through-event-listeners.html ----------------------------------------------------- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/step-through-event-listeners-pretty-diff.html It seems correct, but we need JSC and V8 specific expected files. inspector/debugger/xhr-breakpoints.html ---------------------------------------- http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/xhr-breakpoints-pretty-diff.html It is absolutely wrong on Qt. I filed a new bug report to track the failures (because I don't want to confuse anybody with discussing in this bug)- https://bugs.webkit.org/show_bug.cgi?id=88780 (In reply to comment #25) > I filed a new bug report to track the failures (because I don't want to confuse anybody with discussing in this bug)- https://bugs.webkit.org/show_bug.cgi?id=88780 I've investigated the test cases, it's just because expectation file is old. I'll resolve https://bugs.webkit.org/show_bug.cgi?id=88780 as soon as possible. Now that this bug is resolved we should unskip some of the tests that used to fail because of it, see for example: # https://bugs.webkit.org/show_bug.cgi?id=43332 inspector/debugger/dom-breakpoints.html inspector/debugger/event-listener-breakpoints.html inspector/debugger/step-through-event-listeners.html inspector/debugger/xhr-breakpoints.html http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/TestExpectations#L247 |