Bug 35926 - Web Inspector: Hangup when expanding elements with enormous text node content in Elements panel
Summary: Web Inspector: Hangup when expanding elements with enormous text node content...
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: Alexander Pavlov (apavlov)
URL: https://zlibdroidtest.appspot.com/
Keywords:
: 35764 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-03-09 09:38 PST by Alexander Pavlov (apavlov)
Modified: 2010-06-23 19:04 PDT (History)
14 users (show)

See Also:


Attachments
[PATCH] Suggested solution (3.33 KB, patch)
2010-03-10 05:59 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Bots fixed, explanation added (2.50 KB, patch)
2010-03-10 07:30 PST, Alexander Pavlov (apavlov)
eric: review-
Details | Formatted Diff | Diff
[PATCH] Rebaselined. (2.25 KB, patch)
2010-06-11 07:02 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-03-09 09:38:24 PST
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
Comment 1 Alexander Pavlov (apavlov) 2010-03-10 05:59:04 PST
Created attachment 50395 [details]
[PATCH] Suggested solution
Comment 2 WebKit Review Bot 2010-03-10 06:02:00 PST
Attachment 50395 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/530014
Comment 3 WebKit Review Bot 2010-03-10 06:02:55 PST
Attachment 50395 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/566018
Comment 4 Eric Seidel (no email) 2010-03-10 06:04:01 PST
Attachment 50395 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/578012
Comment 5 Pavel Feldman 2010-03-10 06:11:58 PST
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).
Comment 6 Alexander Pavlov (apavlov) 2010-03-10 07:30:11 PST
Created attachment 50403 [details]
[PATCH] Bots fixed, explanation added
Comment 7 Alexander Pavlov (apavlov) 2010-03-10 07:38:47 PST
(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).
Comment 8 Eric Seidel (no email) 2010-03-15 15:30:46 PDT
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.
Comment 9 Pavel Feldman 2010-03-15 15:37:14 PDT
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.
Comment 10 Eric Seidel (no email) 2010-03-15 15:44:39 PDT
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. ;)
Comment 11 Pavel Feldman 2010-03-15 15:52:42 PDT
(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.
Comment 12 Alexander Pavlov (apavlov) 2010-03-23 10:03:54 PDT
*** Bug 35764 has been marked as a duplicate of this bug. ***
Comment 13 Pavel Feldman 2010-06-11 07:02:08 PDT
Created attachment 58469 [details]
[PATCH] Rebaselined.
Comment 14 Pavel Feldman 2010-06-11 07:03:26 PDT
This now looks sane to me. Dave, could you please review this tiny change?
Comment 15 Eric Seidel (no email) 2010-06-11 08:18:25 PDT
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.
Comment 16 Dave Hyatt 2010-06-23 11:37:13 PDT
Comment on attachment 58469 [details]
[PATCH] Rebaselined.

Good change. r=me.
Comment 17 WebKit Commit Bot 2010-06-23 18:33:32 PDT
Comment on attachment 58469 [details]
[PATCH] Rebaselined.

Clearing flags on attachment: 58469

Committed r61724: <http://trac.webkit.org/changeset/61724>
Comment 18 WebKit Commit Bot 2010-06-23 18:33:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2010-06-23 19:04:35 PDT
http://trac.webkit.org/changeset/61724 might have broken GTK Linux 64-bit Debug