Bug 58113 - [Refactoring] DocumentMarkerController::MarkerMapVectorPair should be replaced with a list of some class.
Summary: [Refactoring] DocumentMarkerController::MarkerMapVectorPair should be replace...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 58111
  Show dependency treegraph
 
Reported: 2011-04-07 19:59 PDT by Hajime Morrita
Modified: 2011-04-27 16:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (35.23 KB, patch)
2011-04-19 11:48 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (33.79 KB, patch)
2011-04-27 10:21 PDT, Hajime Morrita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-04-07 19:59:30 PDT
DocumentMarkerController maintains two lists for each marked node.
But these list should be unified to single list.
Comment 1 Hajime Morrita 2011-04-19 11:48:23 PDT
Created attachment 90231 [details]
Patch
Comment 2 Hajime Morrita 2011-04-19 11:49:11 PDT
CC-ing people who wrote original DocumentMarkerController class.
Comment 3 Tony Chang 2011-04-26 16:55:15 PDT
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
> +2011-04-19  MORITA Hajime  <morrita@google.com>
> +
> +        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.
Comment 4 Hajime Morrita 2011-04-27 10:21:19 PDT
Created attachment 91309 [details]
Patch
Comment 5 Hajime Morrita 2011-04-27 10:25:10 PDT
Hi Tony, thank you for taking a look!
I updated to address the problem.

> > Source/WebCore/ChangeLog:47
> > +2011-04-19  MORITA Hajime  <morrita@google.com>
> > +
> > +        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.
Comment 6 Tony Chang 2011-04-27 10:42:15 PDT
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
Comment 7 Hajime Morrita 2011-04-27 16:02:26 PDT
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.
Comment 8 Hajime Morrita 2011-04-27 16:09:11 PDT
Committed r85118: <http://trac.webkit.org/changeset/85118>
Comment 9 WebKit Review Bot 2011-04-27 16:54:22 PDT
http://trac.webkit.org/changeset/85118 might have broken Qt Linux Release