Vector stores Expression object as OwnPtr instead of raw pointer in XPath.

Vector stores Expression object as OwnPtr instead of raw pointer in XPath.

Vector can handle OwnPtr, so we don't have to delete an element in Vector
manually.

BUG=307055
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159752

[dshwang, 2013-10-11 20:24:30]

could you review? :)

[abarth-chromium, 2013-10-11 20:28:05]

lgtm

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
File Source/core/xml/XPathExpressionNode.h (right):

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
Source/core/xml/XPathExpressionNode.h:66: void addSubExpression(Expression*
expr)
This should be a PassOwnPtr<Expression>

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
Source/core/xml/XPathExpressionNode.h:68:
m_subExpressions.append(adoptPtr(expr));
We should call adoptPtr immediately after calling new.  There shouldn't be
random adoptPtr calls littered throughout the code base.

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathPath.cpp
File Source/core/xml/XPathPath.cpp (right):

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathPath....
Source/core/xml/XPathPath.cpp:40: : m_expr(adoptPtr(expr))
Again, |expr| should have the type PassOwnPtr<Expression>

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathPath....
Source/core/xml/XPathPath.cpp:44: m_predicates.append(adoptPtr(predicates[i]));
Calling adoptPtr manually here is wrong for the same reason.

[dshwang, 2013-10-11 20:43:34]

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
File Source/core/xml/XPathExpressionNode.h (right):

https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
Source/core/xml/XPathExpressionNode.h:68:
m_subExpressions.append(adoptPtr(expr));
On 2013/10/11 20:28:05, abarth wrote:
> We should call adoptPtr immediately after calling new.  There shouldn't be
> random adoptPtr calls littered throughout the code base.

I hope so but this method is used by yacc.c
If it is possible, I will do it in next CL. the patch purpose will be very
different of this CL. Could you advice me? :)

/* Line 1806 of yacc.c  */
#line 411 "xml/XPathGrammar.y"
    {
        (yyval.expr) = new Union;
        (yyval.expr)->addSubExpression((yyvsp[(1) - (3)].expr));
        (yyval.expr)->addSubExpression((yyvsp[(3) - (3)].expr));
        parser->unregisterParseNode((yyvsp[(1) - (3)].expr));
        parser->unregisterParseNode((yyvsp[(3) - (3)].expr));
        parser->registerParseNode((yyval.expr));
    }

[abarth-chromium, 2013-10-11 20:56:04]

On 2013/10/11 20:43:34, dshwang wrote:
>
https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
> File Source/core/xml/XPathExpressionNode.h (right):
> 
>
https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
> Source/core/xml/XPathExpressionNode.h:68:
> m_subExpressions.append(adoptPtr(expr));
> On 2013/10/11 20:28:05, abarth wrote:
> > We should call adoptPtr immediately after calling new.  There shouldn't be
> > random adoptPtr calls littered throughout the code base.
> 
> I hope so but this method is used by yacc.c
> If it is possible, I will do it in next CL. the patch purpose will be very
> different of this CL. Could you advice me? :)
> 
> /* Line 1806 of yacc.c  */
> #line 411 "xml/XPathGrammar.y"
>     {
>         (yyval.expr) = new Union;
>         (yyval.expr)->addSubExpression((yyvsp[(1) - (3)].expr));
>         (yyval.expr)->addSubExpression((yyvsp[(3) - (3)].expr));
>         parser->unregisterParseNode((yyvsp[(1) - (3)].expr));
>         parser->unregisterParseNode((yyvsp[(3) - (3)].expr));
>         parser->registerParseNode((yyval.expr));
>     }

Can we put the adoptPtr calls in xml/XPathGrammar.y to make it clear that it's
yacc causing this sadness?

[dshwang, 2013-10-14 15:55:54]

On 2013/10/11 20:56:04, abarth wrote:
> On 2013/10/11 20:43:34, dshwang wrote:
> >
>
https://codereview.chromium.org/26763004/diff/7001/Source/core/xml/XPathExpre...
> > File Source/core/xml/XPathExpressionNode.h (right):
> > 
> >
> Can we put the adoptPtr calls in xml/XPathGrammar.y to make it clear that it's
> yacc causing this sadness?

Thank you for advice. I did it.

[dshwang, 2013-10-14 15:56:04]

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathFunc...
File Source/core/xml/XPathFunctions.h (left):

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathFunc...
Source/core/xml/XPathFunctions.h:50: Function* createFunction(const String&
name, const Vector<Expression*>& args = Vector<Expression*>());
Currently, Vector has a bug as follows
Vector<OwnPtr<Foo> > foo1;
Vector<OwnPtr<Foo> > foo2 = foo1; <- compile error.

So default parameter is impossible.

I'll fix it in another CL.

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathPath...
File Source/core/xml/XPathPath.cpp (right):

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathPath...
Source/core/xml/XPathPath.cpp:45: 
As I mentioned above, Vector<OwnPtr<>> copy constructor fail to compile.

I'll fix Vector and then change Vector<Predicate*> to Vector<OwnPtr<Predicate> >
in whole XPath code.

[abarth-chromium, 2013-10-15 17:51:36]

LGTM.  This looks like a good direction for this code.  Thank you for the CL.

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathPath...
File Source/core/xml/XPathPath.cpp (right):

https://codereview.chromium.org/26763004/diff/18001/Source/core/xml/XPathPath...
Source/core/xml/XPathPath.cpp:45: 
On 2013/10/14 15:56:04, dshwang wrote:
> As I mentioned above, Vector<OwnPtr<>> copy constructor fail to compile.
> 
> I'll fix Vector and then change Vector<Predicate*> to Vector<OwnPtr<Predicate>
>
> in whole XPath code.

Great!

[commit-bot: I haz the power, 2013-10-15 17:52:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/26763004/18001

[commit-bot: I haz the power, 2013-10-16 10:33:36]

Change committed as 159752