Bug 67647 – Web Inspector: do not re-create RawSourceCode when toggling pretty-print mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

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

[Pavel Podivilov]

Created attachment 106423 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 106423 [details]
Patch

Attachment 106423 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9590989

[Yury Semikhatsky]

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.

[Pavel Podivilov]

Created attachment 106575 [details]
Patch

[Yury Semikhatsky]

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?

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Created attachment 106576 [details]
Patch

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

(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.

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Created attachment 106586 [details]
Patch

[Pavel Podivilov]

Committed r94674: <http://trac.webkit.org/changeset/94674>

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

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

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Pavel Podivilov]

Committed r94760: <http://trac.webkit.org/changeset/94760>