Web Inspector: do not re-create RawSourceCode when toggling pretty-print mode. * implement RawSourceCode.setFormatted that allows toggling pretty-print mode on the fly without recreating everything * add RawSourceCode unit tests * remove source mapping listeners and console messages from presentation model
Created attachment 106423 [details] Patch
Attachment 106423 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Last 3072 characters of output: ://src.chromium.org/svn/trunk/src/media@99393 parsed_url: http://src.chromium.org/svn/trunk/src/media@99393 should_process: True processed: True requirements: set(['./']) name: build url: http://src.chromium.org/svn/trunk/src/build@99393 parsed_url: http://src.chromium.org/svn/trunk/src/build@99393 should_process: True processed: True requirements: set(['./']) name: net url: http://src.chromium.org/svn/trunk/src/net@99393 should_process: True requirements: set(['./']) name: tools/data_pack url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 should_process: True requirements: set(['./']) name: third_party/ffmpeg url: From('chromium_deps', 'src/third_party/ffmpeg') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/generate_stubs url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 should_process: True requirements: set(['./']) name: third_party/libjpeg_turbo url: From('chromium_deps', 'src/third_party/libjpeg_turbo') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/v8-i18n url: From('chromium_deps', 'src/third_party/v8-i18n') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/grit url: http://src.chromium.org/svn/trunk/src/tools/grit@99393 should_process: True requirements: set(['./']) name: base url: http://src.chromium.org/svn/trunk/src/base@99393 should_process: True requirements: set(['./']) name: sql url: http://src.chromium.org/svn/trunk/src/sql@99393 should_process: True requirements: set(['./']) name: v8 url: From('chromium_deps', 'src/v8') should_process: True requirements: set(['chromium_deps', './']) name: testing/gtest url: From('chromium_deps', 'src/testing/gtest') should_process: True requirements: set(['testing', 'chromium_deps', './']) name: third_party/libwebp url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393 should_process: True requirements: set(['third_party', './']) name: googleurl url: From('chromium_deps', 'src/googleurl') should_process: True requirements: set(['chromium_deps', './']) name: third_party/skia/include url: From('chromium_deps', 'src/third_party/skia/include') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/ots url: From('chromium_deps', 'src/third_party/ots') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/snappy/src url: From('chromium_deps', 'src/third_party/snappy/src') should_process: True requirements: set(['third_party', 'chromium_deps', './']) Died at Tools/Scripts/update-webkit-chromium line 80. No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 106423 [details] Patch Attachment 106423 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9590989
Comment on attachment 106423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106423&action=review > LayoutTests/inspector/debugger/raw-source-code.html:9 > + var sourceMapping = { Can this test be split into a few smaller ones? > LayoutTests/inspector/debugger/raw-source-code.html:83 > + var timerId = originalSetTimeout(invokeCallback, timeout) It should be originalSetTimeout.call(window,...) > LayoutTests/inspector/debugger/raw-source-code.html:93 > + for (var i = 0; i < 3; ++i) { Why do you do it exactly thrice? Should it loop until timers is empty? If you assume that firing timers may modify "timers" map it should be cloned before the iteration. > LayoutTests/inspector/debugger/raw-source-code.html:105 > + var script = createScript("foo.js", [0, 0, 20, 80], true, "<script source>"); Here and below script content and start/end line/column should be in sync, otherwise the test uses invalid scripts. r- for this.
Created attachment 106575 [details] Patch
Comment on attachment 106575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106575&action=review > LayoutTests/inspector/debugger/raw-source-code.html:9 > + function createScriptMock(url, range, isContentScript, source) Can this method derive end coordinates from the start and source?
(In reply to comment #4) > (From update of attachment 106423 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106423&action=review > > > LayoutTests/inspector/debugger/raw-source-code.html:9 > > + var sourceMapping = { > > Can this test be split into a few smaller ones? This is a set of unit tests for a single class, I think it's nice to have them defined in the same file. Also, it's faster than loading multiple test documents. > > > LayoutTests/inspector/debugger/raw-source-code.html:83 > > + var timerId = originalSetTimeout(invokeCallback, timeout) > > It should be originalSetTimeout.call(window,...) Done. > > > LayoutTests/inspector/debugger/raw-source-code.html:93 > > + for (var i = 0; i < 3; ++i) { > > Why do you do it exactly thrice? Should it loop until timers is empty? If you assume that firing timers may modify "timers" map it should be cloned before the iteration. I don't want this test to start timing out if there is a setTimeout chain somewhere. Three times is just enough for the test purpose, and there shouldn't be any problems with it. Keys list is cloned by VM, no need to do it manually (items added during enumeration are guaranteed not to be visited. > > > LayoutTests/inspector/debugger/raw-source-code.html:105 > > + var script = createScript("foo.js", [0, 0, 20, 80], true, "<script source>"); > > Here and below script content and start/end line/column should be in sync, otherwise the test uses invalid scripts. r- for this. Done.
Created attachment 106576 [details] Patch
Comment on attachment 106576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106576&action=review > LayoutTests/inspector/debugger/raw-source-code.html:96 > + function fireTimers() Are there real setTimeout chains that can't be handled with listeners/sniffers like in other inspector tests? We don't intervene in the timer in other tests I don't see why we should here.
(In reply to comment #7) > (In reply to comment #4) > > (From update of attachment 106423 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106423&action=review > Keys list is cloned by VM, no need to do it manually (items added during enumeration are guaranteed not to be visited. > Good to know.
(In reply to comment #9) > (From update of attachment 106576 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106576&action=review > > > LayoutTests/inspector/debugger/raw-source-code.html:96 > > + function fireTimers() > > Are there real setTimeout chains that can't be handled with listeners/sniffers like in other inspector tests? We don't intervene in the timer in other tests I don't see why we should here. Removed use of timers from RawSourceCode._createSourceMapping and the test.
Created attachment 106586 [details] Patch
Committed r94674: <http://trac.webkit.org/changeset/94674>
inspector/debugger/script-formatter.html is failing on Qt after this patch: http://build.webkit.org/results/Qt%20Linux%20Release/r94677%20(37240)/results.html
This is causing a failure on Mac: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94675%20(32889)/inspector/debugger/script-formatter-pretty-diff.html
(In reply to comment #15) > This is causing a failure on Mac: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r94675%20(32889)/inspector/debugger/script-formatter-pretty-diff.html I think it's fixed in http://trac.webkit.org/changeset/94689.
Mn... the test still appears to be flaky on Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r94694%20(37244)/results.html http://build.webkit.org/results/Qt%20Linux%20Release/r94690%20(37243)/results.html
This test is also failing on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r94736%20(2047)/inspector/debugger/script-formatter-pretty-diff.html
Here's failure on Windows 7: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r94740%20(16122)/inspector/debugger/script-formatter-pretty-diff.html
I'm going to rollout the patch for now since I don't understand what the test is testing or what this change is about, and nobody appears to be actively working on it.
Committed r94760: <http://trac.webkit.org/changeset/94760>