Bug 80373 - InsertionPoint::attach should be consistent with Element.
Summary: InsertionPoint::attach should be consistent with Element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 78596
  Show dependency tree / graph
 
Reported: 2012-03-05 20:37 PST by Shinya Kawanaka
Modified: 2012-03-05 22:59 PST (History)
0 users

See Also:

Attachments
Patch (4.54 KB, patch)
2012-03-05 20:45 PST, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2012-03-05 21:35 PST, Shinya Kawanaka 		morrita: 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 Shinya Kawanaka 2012-03-05 20:37:32 PST 
This bug is preparation for Bug 78596.

InsertionPoint::attach will have code for <shadow> element later, but it is a bit messy now.
So let's clean up the code.

(1) Currently InsertionPoint attaches fallback elements before attaching distributed elements.
To be consistent, attaching distributed elements first is better, since Element attaches shadow tree first.

(2) Let's extract functions from InsertionPoint::attach() also.
Comment 1 Shinya Kawanaka 2012-03-05 20:45:35 PST 
Created attachment 130278 [details]
Patch
Comment 2 Hajime Morrita 2012-03-05 21:14:14 PST 
Comment on attachment 130278 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Refactoring: Make InsertionPoint code clean.

Could you explain the change more concretely?
What the following explanation says isn't cleanup apparently.

> Source/WebCore/ChangeLog:10
> +        InsertionPoint used to attach fallback element before attaching distributed elements.

fallback elements?

> Source/WebCore/ChangeLog:11
> +        To be consistent with Element::attach behavior, attachign distributed elements first is

s/attachign/attaching/

> Source/WebCore/html/shadow/InsertionPoint.cpp:52
> +        distributeLightChildren(root->tree());

Could you reconsider the name? There is no term "light children" in the standard.
Comment 3 Shinya Kawanaka 2012-03-05 21:34:01 PST 
(In reply to comment #2)
> (From update of attachment 130278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130278&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Refactoring: Make InsertionPoint code clean.
> 
> Could you explain the change more concretely?
> What the following explanation says isn't cleanup apparently.

Done.

> 
> > Source/WebCore/ChangeLog:10
> > +        InsertionPoint used to attach fallback element before attaching distributed elements.
> 
> fallback elements?

Done.

> 
> > Source/WebCore/ChangeLog:11
> > +        To be consistent with Element::attach behavior, attachign distributed elements first is
> 
> s/attachign/attaching/

Done.

> 
> > Source/WebCore/html/shadow/InsertionPoint.cpp:52
> > +        distributeLightChildren(root->tree());
> 
> Could you reconsider the name? There is no term "light children" in the standard.

Renamed to distributeHostChildren.
Comment 4 Shinya Kawanaka 2012-03-05 21:35:00 PST 
Created attachment 130280 [details]
Patch
Comment 5 Hajime Morrita 2012-03-05 22:21:13 PST 
Comment on attachment 130280 [details]
Patch

looks good.
Comment 6 Shinya Kawanaka 2012-03-05 22:59:29 PST 
Committed r109864: <http://trac.webkit.org/changeset/109864>