Bug 164503 - Move RenderNamedFlowThread nextRendererForElement logic to RenderTreeUpdater.
Summary: Move RenderNamedFlowThread nextRendererForElement logic to RenderTreeUpdater.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-07 20:44 PST by zalan
Modified: 2016-11-09 12:41 PST (History)
7 users (show)

See Also:

Attachments
Patch (12.35 KB, patch)
2016-11-07 20:49 PST, zalan 		no flags Details | Formatted Diff | Diff
Patch (15.59 KB, patch)
2016-11-08 22:26 PST, zalan 		no flags Details | Formatted Diff | Diff
Patch (15.67 KB, patch)
2016-11-09 11:18 PST, zalan 		no flags Details | Formatted Diff | Diff
Patch (15.60 KB, patch)
2016-11-09 11:35 PST, zalan 		no flags Details | Formatted Diff | Diff
Patch (15.60 KB, patch)
2016-11-09 11:37 PST, zalan 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) 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 zalan 2016-11-07 20:44:43 PST 
This logic has no render tree dependency. Also by moving the logic to RenderTreeUpdate, we can remove RenderNamedFlowThread::m_flowThreadChildList -> more secure.
Comment 1 zalan 2016-11-07 20:49:15 PST 
Created attachment 294129 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-11-07 21:42:29 PST 
Comment on attachment 294129 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No change in functionality.

Moar words here pls.
Comment 3 Antti Koivisto 2016-11-07 22:41:15 PST 
Comment on attachment 294129 [details]
Patch

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

> Source/WebCore/style/RenderTreeUpdater.cpp:335
>      renderTreePosition().computeNextSibling(element);
> -
> -    RenderTreePosition insertionPosition = parentFlowRenderer
> -        ? RenderTreePosition(*parentFlowRenderer, parentFlowRenderer->nextRendererForElement(element))
> -        : renderTreePosition();
> -
> +    RenderTreePosition insertionPosition = !parentFlowRenderer ? renderTreePosition() : RenderTreePosition(*parentFlowRenderer,
> +        renderTreePosition().nextSiblingRenderer(element, m_styleUpdate->elementStyle(element)));

Wouldn't just using 'style' here work instead of 'm_styleUpdate->elementStyle(element)'?

Could we just provide style or Style::Update& to computeNextSibling() above and eliminate the parentFlowRenderer special case completely?
Comment 4 zalan 2016-11-08 22:26:14 PST 
Created attachment 294217 [details]
Patch
Comment 5 zalan 2016-11-09 11:18:23 PST 
Created attachment 294252 [details]
Patch
Comment 6 Antti Koivisto 2016-11-09 11:23:54 PST 
Comment on attachment 294252 [details]
Patch

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

> Source/WebCore/style/RenderTreeUpdater.cpp:324
> +        bool willMoveToFlowThread = element.shouldMoveToFlowThread(style);
> +        
> +        if (willMoveToFlowThread)

Doesn't really need boolean here.
Comment 7 zalan 2016-11-09 11:35:58 PST 
Created attachment 294255 [details]
Patch
Comment 8 zalan 2016-11-09 11:37:12 PST 
Created attachment 294256 [details]
Patch
Comment 9 WebKit Commit Bot 2016-11-09 12:41:32 PST 
Comment on attachment 294256 [details]
Patch

Clearing flags on attachment: 294256

Committed r208470: <http://trac.webkit.org/changeset/208470>
Comment 10 WebKit Commit Bot 2016-11-09 12:41:37 PST 
All reviewed patches have been landed.  Closing bug.