Optimization in handleStyleSpans() for restricting the for loop to iterate only

Optimization in handleStyleSpans() for restricting the for loop to iterate only
over the InsertedNodes (i.e. the inserted fragment).
This is in line with other methods such as removeRedundantStylesAndKeepStyleSpanInline()
and makeInsertedContentRoundTrippableWithHTMLTreeBuilder() that iterate over
the InsertedNodes.

No existing test should fail with this patch.

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

[yosin_UTC9, 2013-11-15 01:31:58]

ACK

Thanks for finding this.
How about moving this to member function of InsertedNodes? It is easier to read.

[yosin_UTC9, 2013-11-15 01:50:16]

BTW, it is better to remove isLegacyAppleStyle(). I filed it as
http://crbug.com/319594. We may want to have another patch for this.

[arpitab_, 2013-11-15 05:38:28]

On 2013/11/15 01:50:16, Yoshi wrote:
> BTW, it is better to remove isLegacyAppleStyle(). I filed it as
> http://crbug.com/319594. We may want to have another patch for this.

Yes! I was thinking about removing the same. I will get on it post this patch.

[arpitab_, 2013-11-15 07:25:13]

On 2013/11/15 01:31:58, Yoshi wrote:
> ACK
> 
> Thanks for finding this.
> How about moving this to member function of InsertedNodes? It is easier to
read.

Hi Yoshi,

Did you mean to have something like this here?:
for (Node* node = insertedNodes.firstNodeInserted(); node; node =
insertedNodes.nextInsertedNode()) {

[yosin_UTC9, 2013-11-15 07:49:34]

> Did you mean to have something like this here?:
> for (Node* node = insertedNodes.firstNodeInserted(); node; node =
> insertedNodes.nextInsertedNode()) {

Sorry for confusion. I mean
Node* wrappingStyleSpan = insertedNodes.findWrappingStyleSpan();
However, it may be overkill.

Your idea seems to be good. How about having Iterator?
(better name please!)

class InsertedNodes {
public:
  class Iterator {
  public:
     Iterator(const InsertedNodes& nodes) :
m_pasetEndNode(nodes.pastLeafNode()), m_node(nodes.firstNodeInserted()) {}
     void next() { m_node = NodeTraversal::next(*m_node); }
     PassRef<Node> node() const { return m_node; }
  private:
     RefPtr<Node> m_node;
     RefPtr<Node> m_pastEndNode;
  };
};

[arpitab_, 2013-11-15 14:10:46]

On 2013/11/15 07:49:34, Yoshi wrote:
> > Did you mean to have something like this here?:
> > for (Node* node = insertedNodes.firstNodeInserted(); node; node =
> > insertedNodes.nextInsertedNode()) {
> 
> Sorry for confusion. I mean
> Node* wrappingStyleSpan = insertedNodes.findWrappingStyleSpan();
> However, it may be overkill.
> 
> Your idea seems to be good. How about having Iterator?
> (better name please!)
> 
> class InsertedNodes {
> public:
>   class Iterator {
>   public:
>      Iterator(const InsertedNodes& nodes) :
> m_pasetEndNode(nodes.pastLeafNode()), m_node(nodes.firstNodeInserted()) {}
>      void next() { m_node = NodeTraversal::next(*m_node); }
>      PassRef<Node> node() const { return m_node; }
>   private:
>      RefPtr<Node> m_node;
>      RefPtr<Node> m_pastEndNode;
>   };
> };

Hi Yoshi,

I did actually implement an iterator here as you had suggested but am not very
sure as to how much we may gain by introducing one here.
Wouldn't we ultimately end up doing something similar to the current for loop??

[yosin_UTC9, 2013-11-18 01:21:28]

On 2013/11/15 14:10:46, arpitab_ wrote:
> On 2013/11/15 07:49:34, Yoshi wrote:
> > > Did you mean to have something like this here?:
> > > for (Node* node = insertedNodes.firstNodeInserted(); node; node =
> > > insertedNodes.nextInsertedNode()) {
> > 
> > Sorry for confusion. I mean
> > Node* wrappingStyleSpan = insertedNodes.findWrappingStyleSpan();
> > However, it may be overkill.
> > 
> > Your idea seems to be good. How about having Iterator?
> > (better name please!)
> > 
> > class InsertedNodes {
> > public:
> >   class Iterator {
> >   public:
> >      Iterator(const InsertedNodes& nodes) :
> > m_pasetEndNode(nodes.pastLeafNode()), m_node(nodes.firstNodeInserted()) {}
> >      void next() { m_node = NodeTraversal::next(*m_node); }
> >      PassRef<Node> node() const { return m_node; }
> >   private:
> >      RefPtr<Node> m_node;
> >      RefPtr<Node> m_pastEndNode;
> >   };
> > };
> 
> Hi Yoshi,
> 
> I did actually implement an iterator here as you had suggested but am not very
> sure as to how much we may gain by introducing one here.
> Wouldn't we ultimately end up doing something similar to the current for
loop??

My motivation is there are three places, include this one, has patter:

RefPtr<Node*> pastEndNode = insertedNode.pastEndNode();
for (RefPtr<Node*> node = insertedNode.firstNodeInserted(); node && node !=
pastEndNode; ...)

We may or may not add another for loop. This is just my preference. So, if you
don't need to follow.
Introducing iterator class increases code size.

[arpitab_, 2013-11-18 10:07:28]

On 2013/11/18 01:21:28, Yoshi wrote:
> On 2013/11/15 14:10:46, arpitab_ wrote:
> > On 2013/11/15 07:49:34, Yoshi wrote:
> > > > Did you mean to have something like this here?:
> > > > for (Node* node = insertedNodes.firstNodeInserted(); node; node =
> > > > insertedNodes.nextInsertedNode()) {
> > > 
> > > Sorry for confusion. I mean
> > > Node* wrappingStyleSpan = insertedNodes.findWrappingStyleSpan();
> > > However, it may be overkill.
> > > 
> > > Your idea seems to be good. How about having Iterator?
> > > (better name please!)
> > > 
> > > class InsertedNodes {
> > > public:
> > >   class Iterator {
> > >   public:
> > >      Iterator(const InsertedNodes& nodes) :
> > > m_pasetEndNode(nodes.pastLeafNode()), m_node(nodes.firstNodeInserted()) {}
> > >      void next() { m_node = NodeTraversal::next(*m_node); }
> > >      PassRef<Node> node() const { return m_node; }
> > >   private:
> > >      RefPtr<Node> m_node;
> > >      RefPtr<Node> m_pastEndNode;
> > >   };
> > > };
> > 
> > Hi Yoshi,
> > 
> > I did actually implement an iterator here as you had suggested but am not
very
> > sure as to how much we may gain by introducing one here.
> > Wouldn't we ultimately end up doing something similar to the current for
> loop??
> 
> My motivation is there are three places, include this one, has patter:
> 
> RefPtr<Node*> pastEndNode = insertedNode.pastEndNode();
> for (RefPtr<Node*> node = insertedNode.firstNodeInserted(); node && node !=
> pastEndNode; ...)
> 
> We may or may not add another for loop. This is just my preference. So, if you
> don't need to follow.
> Introducing iterator class increases code size.

Hi Yoshi,
Perhaps we can let the for loop be for the time being. Adding the iterator
appears to make the code more cumbersome here (in my opinion).
Also, if at all we remove the legacyAppleStyle (later), there will be one less
instance of this loop.

[yosin_UTC9, 2013-11-19 06:29:20]

ACK

[leviw_travelin_and_unemployed, 2013-11-26 23:50:57]

Did this come up in a profile? How about a perf test that shows that this is a
win?

[yosin_UTC9, 2013-11-27 01:09:15]

On 2013/11/26 23:50:57, Levi wrote:
> Did this come up in a profile? How about a perf test that shows that this is a
> win?

It seems this is following the pattern to others. Other two loops remembers
pastEndNode before for-loop. So, each iteration reduce time for calling
NodeTraversal::next() + Node::lastDescendant().

[yosin_UTC9, 2013-11-27 01:09:51]

On 2013/11/27 01:09:15, Yoshi wrote:
> On 2013/11/26 23:50:57, Levi wrote:
> > Did this come up in a profile? How about a perf test that shows that this is
a
> > win?
> 
> It seems this is following the pattern to others. Other two loops remembers
> pastEndNode before for-loop. So, each iteration reduce time for calling
> NodeTraversal::next() + Node::lastDescendant().

How about changing description to remove the word "Optimization"?

[leviw_travelin_and_unemployed, 2013-11-27 01:56:10]

On 2013/11/27 01:09:51, Yoshi wrote:
> On 2013/11/27 01:09:15, Yoshi wrote:
> > On 2013/11/26 23:50:57, Levi wrote:
> > > Did this come up in a profile? How about a perf test that shows that this
is
> a
> > > win?
> > 
> > It seems this is following the pattern to others. Other two loops remembers
> > pastEndNode before for-loop. So, each iteration reduce time for calling
> > NodeTraversal::next() + Node::lastDescendant().
> 
> How about changing description to remove the word "Optimization"?

If it doesn't change behavior but does change performance, I'd say optimization
is an accurate description.

In general we should always try and include a perf test with this sort of
change, particularly if it hasn't shown up as a performance issue in any real
world sites. LGTM on this patch for the fact that it's consistent with the other
similar loops in ReplaceSelectionCommand.

[commit-bot: I haz the power, 2013-11-27 01:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/72743002/1

[commit-bot: I haz the power, 2013-11-27 09:41:22]

Change committed as 162740

[arpitab_, 2013-11-27 11:58:20]

On 2013/11/27 01:56:10, Levi wrote:
> On 2013/11/27 01:09:51, Yoshi wrote:
> > On 2013/11/27 01:09:15, Yoshi wrote:
> > > On 2013/11/26 23:50:57, Levi wrote:
> > > > Did this come up in a profile? How about a perf test that shows that
this
> is
> > a
> > > > win?
> > > 
> > > It seems this is following the pattern to others. Other two loops
remembers
> > > pastEndNode before for-loop. So, each iteration reduce time for calling
> > > NodeTraversal::next() + Node::lastDescendant().
> > 
> > How about changing description to remove the word "Optimization"?
> 
> If it doesn't change behavior but does change performance, I'd say
optimization
> is an accurate description.
> 
> In general we should always try and include a perf test with this sort of
> change, particularly if it hasn't shown up as a performance issue in any real
> world sites. LGTM on this patch for the fact that it's consistent with the
other
> similar loops in ReplaceSelectionCommand.

Thanks for the review Levi, Yoshi.
I'll def try to include perf tests for similar patches in the future.