Bug 78778 - [Refactoring] Use HTMLInsertionPoint instead of HTMLContentElement
Summary: [Refactoring] Use HTMLInsertionPoint instead of HTMLContentElement
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: 78771
Blocks: 78596
  Show dependency treegraph
 
Reported: 2012-02-15 22:41 PST by Shinya Kawanaka
Modified: 2012-02-20 22:14 PST (History)
7 users (show)

See Also:


Attachments
Test (18.22 KB, patch)
2012-02-20 00:35 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
use InsertionPoint (12.60 KB, patch)
2012-02-20 03:12 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
update (14.21 KB, patch)
2012-02-20 17:54 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (14.36 KB, patch)
2012-02-20 20:46 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-15 22:41:41 PST
We had assumed shadow insertion point was only HTMLContentElement before.
So a lot of code use HTMLContentElement* type instead of HTMLInsertionPoint type.
But we now have <shadow> element, which is another insertion point.
We should convert HTMLContentElement* type to HTMLInsertionPoint* in NodeRenderingContext and so on.
Comment 1 Shinya Kawanaka 2012-02-20 00:35:41 PST
Created attachment 127775 [details]
Test
Comment 2 Hayato Ito 2012-02-20 03:12:14 PST
Created attachment 127796 [details]
use InsertionPoint
Comment 3 Hajime Morrita 2012-02-20 15:57:23 PST
Comment on attachment 127796 [details]
use InsertionPoint

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

> Source/WebCore/html/shadow/HTMLContentElement.h:NaN
>  public:

It looks we no longer need some #include.

> Source/WebCore/html/shadow/InsertionPoint.cpp:43
> +    , m_selections(adoptPtr(new HTMLContentSelectionList()))

If we instantiate here and have a #include, there would be no reason to allocate this in heap.
I guess the original class lose the point at some point.
Comment 4 Hayato Ito 2012-02-20 17:54:54 PST
Created attachment 127881 [details]
update
Comment 5 Hayato Ito 2012-02-20 17:55:44 PST
(In reply to comment #3)
> (From update of attachment 127796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127796&action=review
> 
> > Source/WebCore/html/shadow/HTMLContentElement.h:NaN
> >  public:
> 
> It looks we no longer need some #include.

Done. I've cleaned up #include.

> 
> > Source/WebCore/html/shadow/InsertionPoint.cpp:43
> > +    , m_selections(adoptPtr(new HTMLContentSelectionList()))
> 
> If we instantiate here and have a #include, there would be no reason to allocate this in heap.
> I guess the original class lose the point at some point.

Done.
Comment 6 Hajime Morrita 2012-02-20 19:38:30 PST
Comment on attachment 127881 [details]
update

Let's make sure that win bot green. It looks unrelated to this patch though.
Comment 7 Shinya Kawanaka 2012-02-20 20:19:11 PST
(In reply to comment #6)
> (From update of attachment 127881 [details])
> Let's make sure that win bot green. It looks unrelated to this patch though.

Unfortunately, win bot is failing these days i(In reply to comment #6)
> (From update of attachment 127881 [details])
> Let's make sure that win bot green. It looks unrelated to this patch though.

This is an error message. 

6>c:\cygwin\home\buildbot\WebKit\Source\WebCore\html\shadow\HTMLContentElement.cpp(100) : error C2819: type 'WebCore::HTMLContentSelectionList' does not have an overloaded member 'operator ->'
6>        c:\cygwin\home\buildbot\webkit\source\webcore\html\shadow\HTMLContentSelector.h(77) : see declaration of 'WebCore::HTMLContentSelectionList'
6>        did you intend to use '.' instead?
6>c:\cygwin\home\buildbot\WebKit\Source\WebCore\html\shadow\HTMLContentElement.cpp(100) : error C2232: '->WebCore::HTMLContentSelectionList::isEmpty' : left operand has 'class' type, use '.'
Comment 8 Hayato Ito 2012-02-20 20:46:14 PST
Created attachment 127907 [details]
Patch for landing
Comment 9 Hayato Ito 2012-02-20 20:47:17 PST
This patch is related. I've fixed the debug build.

(In reply to comment #6)
> (From update of attachment 127881 [details])
> Let's make sure that win bot green. It looks unrelated to this patch though.
Comment 10 WebKit Review Bot 2012-02-20 22:12:47 PST
The commit-queue encountered the following flaky tests while processing attachment 127907 [details]:

perf/object-keys.html bug 63769 (author: ojan@chromium.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Review Bot 2012-02-20 22:14:51 PST
Comment on attachment 127907 [details]
Patch for landing

Clearing flags on attachment: 127907

Committed r108303: <http://trac.webkit.org/changeset/108303>
Comment 12 WebKit Review Bot 2012-02-20 22:14:58 PST
All reviewed patches have been landed.  Closing bug.