Bug 179114

Summary: First letter text renderer should be anonymous
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, darin, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 179112    
Attachments:
Description Flags
patch
none
patch
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
patch
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
patch none

Description Antti Koivisto 2017-11-01 06:29:15 PDT
Currently both RenderTextFragment and RenderText for first letter point to the same Text node. There should only be one non-anonymous renderer per node.
Comment 1 Antti Koivisto 2017-11-01 06:38:23 PDT
Created attachment 325569 [details]
patch
Comment 2 Build Bot 2017-11-01 06:40:26 PDT
Attachment 325569 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBoxModelObject.cpp:2553:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antti Koivisto 2017-11-01 06:52:44 PDT
Created attachment 325570 [details]
patch
Comment 4 Antti Koivisto 2017-11-01 06:56:33 PDT
Created attachment 325571 [details]
patch
Comment 5 Build Bot 2017-11-01 07:51:40 PDT
Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5062417

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2017-11-01 07:51:41 PDT
Created attachment 325573 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2017-11-01 08:02:29 PDT
Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5062479

New failing tests:
fast/css-generated-content/initial-letter-border-padding.html
fast/css-generated-content/initial-letter-basic.html
fast/css-generated-content/initial-letter-raised.html
fast/css-generated-content/initial-letter-sunken.html
ietestcenter/css3/text/textshadow-004.htm
Comment 8 Build Bot 2017-11-01 08:02:30 PDT
Created attachment 325574 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-11-01 08:13:35 PDT
Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5062493

New failing tests:
ietestcenter/css3/text/textshadow-004.htm
fast/css-generated-content/initial-letter-basic.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html
svg/wicd/test-rightsizing-a.xhtml
fast/css-generated-content/initial-letter-raised.html
fast/css-generated-content/initial-letter-border-padding.html
fast/css-generated-content/initial-letter-sunken.html
Comment 10 Build Bot 2017-11-01 08:13:36 PDT
Created attachment 325575 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Antti Koivisto 2017-11-01 08:19:05 PDT
Created attachment 325576 [details]
patch
Comment 12 Darin Adler 2017-11-01 09:37:01 PDT
Comment on attachment 325576 [details]
patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:131
> +typedef HashMap<const RenderBoxModelObject*, WeakPtr<RenderTextFragment>> FirstLetterRemainingTextMap;

In new code it’s nice to use "using" instead of typedef.
Comment 13 zalan 2017-11-01 09:51:08 PDT
Comment on attachment 325576 [details]
patch

We should tighten the first letter type here. It's odd that we set it on the RenderElement but use it in RenderBoxModelObject for cleanup (I know it's not a new code). I also believe we could even use a more concrete renderer type for all this.
Comment 14 Build Bot 2017-11-01 09:52:44 PDT
Comment on attachment 325576 [details]
patch

Attachment 325576 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5062977

New failing tests:
ietestcenter/css3/text/textshadow-004.htm
fast/selectors/166a.html
fast/text/firstline/003.html
fast/css/first-letter-first-line-hover.html
fast/css/first-letter-hover.html
fast/css/first-letter-detach.html
fast/css/first-letter-float-after-float.html
fast/css/first-letter-visibility.html
fast/css/first-letter-capitalized.html
fast/css/first-letter-recalculation.html
fast/css/first-letter-float.html
fast/selectors/039b.html
fast/multicol/shadow-breaking.html
fast/text/firstline/002.html
fast/css/first-letter-punctuation.html
fast/selectors/039.html
Comment 15 Build Bot 2017-11-01 09:52:46 PDT
Created attachment 325581 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 16 Antti Koivisto 2017-11-01 10:10:49 PDT
(In reply to zalan from comment #13)
> Comment on attachment 325576 [details]
> patch
> 
> We should tighten the first letter type here. It's odd that we set it on the
> RenderElement but use it in RenderBoxModelObject for cleanup (I know it's
> not a new code). I also believe we could even use a more concrete renderer
> type for all this.

RenderBoxModelObject is the most concrete type of the first letter renderer as it can be either RenderInline or RenderBlock. 

RenderElement is used for bits simply because that's where we currently keep all the bits.
Comment 17 Antti Koivisto 2017-11-01 10:14:19 PDT
> RenderElement is used for bits simply because that's where we currently keep
> all the bits.

We could make the setters available only in RBMO though.
Comment 18 Simon Fraser (smfr) 2017-11-01 10:56:55 PDT
Comment on attachment 325576 [details]
patch

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

> Source/WebCore/rendering/RenderElement.h:344
> +    unsigned m_isFirstLetter : 1;

Is it worth using a RenderElement bit here? I think first letter is rare enough that you could use RenderObject::rareData().
Comment 19 Antti Koivisto 2017-11-01 13:16:52 PDT
> Is it worth using a RenderElement bit here? I think first letter is rare
> enough that you could use RenderObject::rareData().

I don't think so. There are plenty of free bits in RenderElement and we could probably just add more too (renderer size doesn't appear to be a huge concern in memory or performance).

I'm also unconvinced that RareData is a good pattern in general. As you pile up stuff there you'll end up with a data structure that is large and not rare. See ElementRareData.

The traditional feature-bit-and-side-hashmap approach used all over render tree seems strictly better to me.
Comment 20 Antti Koivisto 2017-11-01 13:49:14 PDT
Created attachment 325626 [details]
patch
Comment 21 Build Bot 2017-11-01 15:15:33 PDT
Comment on attachment 325626 [details]
patch

Attachment 325626 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5066822

New failing tests:
fast/selectors/039b.html
fast/selectors/039.html
Comment 22 Build Bot 2017-11-01 15:15:34 PDT
Created attachment 325639 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 23 Antti Koivisto 2017-11-02 00:44:12 PDT
Created attachment 325696 [details]
patch
Comment 24 WebKit Commit Bot 2017-11-02 01:05:40 PDT
Comment on attachment 325696 [details]
patch

Clearing flags on attachment: 325696

Committed r224324: <https://trac.webkit.org/changeset/224324>
Comment 25 WebKit Commit Bot 2017-11-02 01:05:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2017-11-15 12:31:51 PST
<rdar://problem/35567715>