Bug 119963 - Use TextNodeTraversal for getting sheet text in StyleElement
Summary: Use TextNodeTraversal for getting sheet text in StyleElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-17 15:27 PDT by Antti Koivisto
Modified: 2013-08-20 15:51 PDT (History)
4 users (show)

See Also:


Attachments
oatch (3.11 KB, patch)
2013-08-17 15:34 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff
remove now crashing test (2.83 KB, patch)
2013-08-18 02:32 PDT, Antti Koivisto
kling: review-
Details | Formatted Diff | Diff
mark as Crash (1.56 KB, patch)
2013-08-18 03:36 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-08-17 15:27:39 PDT
foo
Comment 1 Antti Koivisto 2013-08-17 15:34:59 PDT
Created attachment 209012 [details]
oatch
Comment 2 Andreas Kling 2013-08-17 15:37:39 PDT
Comment on attachment 209012 [details]
oatch

View in context: https://bugs.webkit.org/attachment.cgi?id=209012&action=review

Gorgeous!

> Source/WebCore/ChangeLog:11
> +            (which is used by contentsAsString) does that itself. The behavior in case of overflow chances from empty 

chances -> changes
Comment 3 Antti Koivisto 2013-08-17 15:47:20 PDT
https://trac.webkit.org/r154242
Comment 4 Antti Koivisto 2013-08-18 02:32:32 PDT
Created attachment 209022 [details]
remove now crashing test
Comment 5 Andreas Kling 2013-08-18 03:08:06 PDT
Comment on attachment 209022 [details]
remove now crashing test

I think we should have a way for layout tests to PASS when doing a controlled CRASH().
Comment 6 Andreas Kling 2013-08-18 03:14:01 PDT
Comment on attachment 209022 [details]
remove now crashing test

We should just expect this test to [ Crash ] for now.
Comment 7 Antti Koivisto 2013-08-18 03:30:56 PDT
Filed bug 119976 for that.
Comment 8 Antti Koivisto 2013-08-18 03:36:57 PDT
Created attachment 209023 [details]
mark as Crash
Comment 9 Antti Koivisto 2013-08-18 04:06:09 PDT
https://trac.webkit.org/r154246 for TestExpectations
Comment 10 Darin Adler 2013-08-18 19:39:15 PDT
Comment on attachment 209012 [details]
oatch

View in context: https://bugs.webkit.org/attachment.cgi?id=209012&action=review

> Source/WebCore/dom/StyleElement.cpp:114
> +    String sheetText = TextNodeTraversal::contentsAsString(element);
>  
> -    createSheet(e, m_startLineNumber, sheetText.toString());
> +    createSheet(element, m_startLineNumber, sheetText);

For me this would read even better without a local variable.
Comment 11 Ryosuke Niwa 2013-08-20 12:38:14 PDT
CRASH expectation in this case doesn't work well in that the test sometimes times off, trying to allocate a very large memory space.
Comment 12 Ryosuke Niwa 2013-08-20 12:40:17 PDT
Marked the test slow in http://trac.webkit.org/changeset/154352.
Comment 13 Antti Koivisto 2013-08-20 14:30:45 PDT
Maybe we should just get rid of the test? There are many ways you can allocate unreasonable amounts of memory and it is bit silly to be testing this particular one.
Comment 14 Ryosuke Niwa 2013-08-20 14:50:14 PDT
(In reply to comment #13)
> Maybe we should just get rid of the test? There are many ways you can allocate unreasonable amounts of memory and it is bit silly to be testing this particular one.

That might make sense after all.
Comment 15 Ryosuke Niwa 2013-08-20 15:51:21 PDT
Filed webkit.org/b/120095 to remove the test.