Bug 119963

Summary:

Use TextNodeTraversal for getting sheet text in StyleElement

Product:

WebKit

Reporter:

Antti Koivisto <koivisto>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, esprehn+autocc, kangil.han, rniwa

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
oatch	kling: review+
remove now crashing test	kling: review-
mark as Crash	kling: review+

Antti Koivisto

Reported 2013-08-17 15:27:39 PDT

foo

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Antti Koivisto

Comment 1 2013-08-17 15:34:59 PDT

Created attachment 209012 [details] oatch

Andreas Kling

Comment 2 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

Antti Koivisto

Comment 3 2013-08-17 15:47:20 PDT

https://trac.webkit.org/r154242

Antti Koivisto

Comment 4 2013-08-18 02:32:32 PDT

Created attachment 209022 [details] remove now crashing test

Andreas Kling

Comment 5 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().

Andreas Kling

Comment 6 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.

Antti Koivisto

Comment 7 2013-08-18 03:30:56 PDT

Filed bug 119976 for that.

Antti Koivisto

Comment 8 2013-08-18 03:36:57 PDT

Created attachment 209023 [details] mark as Crash

Antti Koivisto

Comment 9 2013-08-18 04:06:09 PDT

https://trac.webkit.org/r154246 for TestExpectations

Darin Adler

Comment 10 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.

Ryosuke Niwa

Comment 11 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.

Ryosuke Niwa

Comment 12 2013-08-20 12:40:17 PDT

Marked the test slow in http://trac.webkit.org/changeset/154352.

Antti Koivisto

Comment 13 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.

Ryosuke Niwa

Comment 14 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.

Ryosuke Niwa

Comment 15 2013-08-20 15:51:21 PDT

Filed webkit.org/b/120095 to remove the test.

Note You need to log in before you can comment on or make changes to this bug.