Bug 120953 - Remove unnecessary sibling text renderers after attach
Summary: Remove unnecessary sibling text renderers after attach
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 120685
  Show dependency tree / graph
 
Reported: 2013-09-07 08:27 PDT by Antti Koivisto
Modified: 2013-09-07 11:26 PDT (History)
1 user (show)

See Also:

Attachments
patch (46.80 KB, patch)
2013-09-07 08:45 PDT, Antti Koivisto 		no flags Details | Formatted Diff | Diff
fix a few bad test result updates (45.89 KB, patch)
2013-09-07 08:54 PDT, Antti Koivisto 		andersca: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-09-07 08:27:12 PDT 
The current createTextRenderersForSiblingsAfterAttachIfNeeded code only adds text renderers, never removes them. Style changes can however go either way.
Comment 1 Antti Koivisto 2013-09-07 08:45:26 PDT 
Created attachment 210904 [details]
patch
Comment 2 Antti Koivisto 2013-09-07 08:54:38 PDT 
Created attachment 210906 [details]
fix a few bad test result updates
Comment 3 Anders Carlsson 2013-09-07 10:58:45 PDT 
Comment on attachment 210906 [details]
fix a few bad test result updates

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

> Source/WebCore/ChangeLog:8
> +        The current createTextRenderersForSiblingsAfterAttachIfNeeded code only adds text renderers, never removes them. Style changes can  go either way.

Extra space before "go".

> Source/WebCore/style/StyleResolveTree.cpp:305
> +        bool hadRenderer = !!whitespaceTextSibling.renderer();

Do you really need the !! here?

> LayoutTests/editing/selection/focus-crash-expected.txt:1
> -To run this test manually, click the "Crash me" button. 
> +To run this test manually, click the "Crash me" button.  

What's with this test? Looks like it adds whitespace?

> LayoutTests/userscripts/user-script-all-frames-expected.txt:1
> +If any thingers appear on this page, the test has failed.  

Same here.

> LayoutTests/userscripts/user-script-top-frame-only-expected.txt:1
> +Two thingers should appear on this page. Otherwise the test has failed.  

And here!
Comment 4 Antti Koivisto 2013-09-07 11:01:23 PDT 
(In reply to comment #3)
> What's with this test? Looks like it adds whitespace?

Yeah. The new code is also less buggy to other direction. It found a few places where logic dictates we actually need a renderer but failed to create one before.
Comment 5 Antti Koivisto 2013-09-07 11:26:43 PDT 
https://trac.webkit.org/r155253