[webkit-reviews] review requested: [Bug 98224] MarkupAccumulator should optimally handle 8 bit Strings : [Attachment 167764] Patch with some updates from reviewer's comments

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 98224: MarkupAccumulator should optimally handle 8 bit Strings
https://bugs.webkit.org/show_bug.cgi?id=98224

Attachment 167764: Patch with some updates from reviewer's comments
https://bugs.webkit.org/attachment.cgi?id=167764&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #3)
> (From update of attachment 166798 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=166798&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:74
> > +		 for (size_t m = 0; m < WTF_ARRAY_LENGTH(entityMaps); ++m) {
> 
> Please don't use abbreviations like m.

This was from the original code, but I changed 'm' to 'entryIndex'.

> > Source/WebCore/editing/MarkupAccumulator.cpp:98
> > +	     const UChar* text = source.characters16() + offset;
> > +
> > +	     size_t positionAfterLastEntity = 0;
> > +	     for (size_t i = 0; i < length; ++i) {
> > +		 for (size_t m = 0; m < WTF_ARRAY_LENGTH(entityMaps); ++m) {
> > +		     if (text[i] == entityMaps[m].entity && entityMaps[m].mask
& entityMask) {
> > +			 result.append(text + positionAfterLastEntity, i -
positionAfterLastEntity);
> > +			 result.append(entityMaps[m].reference);
> > +			 positionAfterLastEntity = i + 1;
> > +			 break;
> > +		     }
> >		 }
> >	     }
> > +	     result.append(text + positionAfterLastEntity, length -
positionAfterLastEntity);
> 
> It seems like we should be able to use a template to avoid code duplication
here.

I thought about that with the original patch.  Given there are static variables
in the method that are used in the loops, either the statics would need to be
duplicated in the template or a pointer to the array along with it's size needs
to be passed into the templated loop function.	Both of these options seemed
less desirable to having the two loops.  If you think either of these would be
more readable, I could be convinced.

> > Source/WebCore/editing/markup.cpp:247
> > +	     MarkupAccumulator::appendCharactersReplacingEntities(buffer,
content, 0, content.length(), EntityMaskInPCDATA);
> 
> StyledMarkupAccumulator inherits from MarkupAccumulator so we shouldn't have
to quality it.

Removed the class name here.

> > Source/WebCore/editing/markup.cpp:994
> > -	 appendCharactersReplacingEntities(markup, title.characters(),
title.length(), EntityMaskInPCDATA);
> > +	 MarkupAccumulator::appendCharactersReplacingEntities(markup, title, 0,
title.length(), EntityMaskInPCDATA);
> 
> Ditto.

Can't remove the class name here as this isn't a member function.