Bug 35926 – Web Inspector: Hangup when expanding elements with enormous text node content in Elements panel

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

[Alexander Pavlov (apavlov)]

Steps to reproduce:

1. Visit https://zlibdroidtest.appspot.com/
2. Bring up the Web Inspector
3. Try switching to the Elements panel

A hangup occurs.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=31832

[Alexander Pavlov (apavlov)]

Created attachment 50395 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

Attachment 50395 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/530014

[WebKit Review Bot]

Attachment 50395 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/566018

[Eric Seidel (no email)]

Attachment 50395 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/578012

[Pavel Feldman]

Comment on attachment 50395 [details]
[PATCH] Suggested solution

You are optimizing for the corner-case. Do you think it improves average case as well?
(r- for bots failures).

[Alexander Pavlov (apavlov)]

Created attachment 50403 [details]
[PATCH] Bots fixed, explanation added

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (From update of attachment 50395 [details])
> You are optimizing for the corner-case. Do you think it improves average case
> as well?

At least, it does not regress an average case (since inRenderedText() is called for isText() renderers which usually have at least one InlineTextBox).

[Eric Seidel (no email)]

Comment on attachment 50403 [details]
[PATCH] Bots fixed, explanation added

Looks good.  However, you should also add a comment next to inRenderedText in Position.h explaining the runtime of the function, or at least warning that it's slow.

[Pavel Feldman]

Eric, I think this could regress average case since we check potentially invisible nodes for being selectable. I would not be landing this - it optimizes for the inspector's corner case only.

[Eric Seidel (no email)]

Comment on attachment 50403 [details]
[PATCH] Bots fixed, explanation added

Pavel is obviously attempting to r- through comment...  Not sure why he didn't just r- it himself. ;)

[Pavel Feldman]

(In reply to comment #10)
> (From update of attachment 50403 [details])
> Pavel is obviously attempting to r- through comment...  Not sure why he didn't
> just r- it himself. ;)

Thing is I am not sure. My gut tells me that optimizing for the corner case described in the bug must have trade-offs :). But we don't have real world metrics that would prove it either so or otherwise. So am just being cautious.

If you think that doing an inexpensive extra check that might be unnecessary prior to doing necessary expensive check is fine in this context - I am all for it.

[Alexander Pavlov (apavlov)]

*** Bug 35764 has been marked as a duplicate of this bug. ***

[Pavel Feldman]

Created attachment 58469 [details]
[PATCH] Rebaselined.

[Pavel Feldman]

This now looks sane to me. Dave, could you please review this tiny change?

[Eric Seidel (no email)]

Can't we make a test for this?  I seem to remember Enrica recently made some Editor perf tests, or at least talked about doing so.

[Dave Hyatt]

Comment on attachment 58469 [details]
[PATCH] Rebaselined.

Good change. r=me.

[WebKit Commit Bot]

Comment on attachment 58469 [details]
[PATCH] Rebaselined.

Clearing flags on attachment: 58469

Committed r61724: <http://trac.webkit.org/changeset/61724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61724 might have broken GTK Linux 64-bit Debug