Bug 58113 – [Refactoring] DocumentMarkerController::MarkerMapVectorPair should be replaced with a list of some class.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

DocumentMarkerController maintains two lists for each marked node.
But these list should be unified to single list.

[Hajime Morrita]

Created attachment 90231 [details]
Patch

[Hajime Morrita]

CC-ing people who wrote original DocumentMarkerController class.

[Tony Chang]

Comment on attachment 90231 [details]
Patch

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

Does this change improve memory usage or is it just a refactor.  Might be good to include that in the changelog.

r- to get out of the review queue so you can upload a version with the fixed ChangeLog

> Source/WebCore/ChangeLog:47
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

Change description shows up twice.

> Source/WebCore/dom/DocumentMarkerController.cpp:338
> +        unsigned markerCount = list->size();

Nit: Should we use size_t here?

> Source/WebCore/dom/RenderedDocumentMarker.h:50
> +    static IntRect invalidMarkerRect() { return IntRect(-1, -1, -1, -1); }

Nit: Should we just make this a const static member variable to avoid making a copy for every call?  I guess that has the downside of needing to add a .cpp file.

[Hajime Morrita]

Created attachment 91309 [details]
Patch

[Hajime Morrita]

Hi Tony, thank you for taking a look!
I updated to address the problem.

> > Source/WebCore/ChangeLog:47
> > [phone]  MORITA Hajime  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Change description shows up twice.
Fixed.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:338
> > +        unsigned markerCount = list->size();
> 
> Nit: Should we use size_t here?
> 
Sure.

> > Source/WebCore/dom/RenderedDocumentMarker.h:50
> > +    static IntRect invalidMarkerRect() { return IntRect(-1, -1, -1, -1); }
> 
> Nit: Should we just make this a const static member variable to avoid making a copy for every call?  I guess that has the downside of needing to add a .cpp file.
Using static class variables generates a "global initializer" error on XCode.
I changed it to use a function static variable, as others do.

[Tony Chang]

Comment on attachment 91309 [details]
Patch

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

> Source/WebCore/dom/DocumentMarkerController.cpp:457
> +// XXX: something wrong

Did you mean to remove this?

> Source/WebCore/dom/DocumentMarkerController.cpp:466
> +    unsigned markerCount = list->size();

Nit: size_t

[Hajime Morrita]

Hi Tony, thank you for quick review!
I'll land this after fix your points.

(In reply to comment #6)
> (From update of attachment 91309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91309&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:457
> > +// XXX: something wrong
> 
> Did you mean to remove this?
Hmm. I don't remember what this means... I'm removing this because it doesn't makes sense.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:466
> > +    unsigned markerCount = list->size();
> 
> Nit: size_t
Will fix.

[Hajime Morrita]

Committed r85118: <http://trac.webkit.org/changeset/85118>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85118 might have broken Qt Linux Release