RESOLVED FIXED 58113
[Refactoring] DocumentMarkerController::MarkerMapVectorPair should be replaced with a list of some class.
https://bugs.webkit.org/show_bug.cgi?id=58113
Summary [Refactoring] DocumentMarkerController::MarkerMapVectorPair should be replace...
Hajime Morrita
Reported 2011-04-07 19:59:30 PDT
DocumentMarkerController maintains two lists for each marked node. But these list should be unified to single list.
Attachments
Patch (35.23 KB, patch)
2011-04-19 11:48 PDT, Hajime Morrita
no flags
Patch (33.79 KB, patch)
2011-04-27 10:21 PDT, Hajime Morrita
tony: review+
Hajime Morrita
Comment 1 2011-04-19 11:48:23 PDT
Hajime Morrita
Comment 2 2011-04-19 11:49:11 PDT
CC-ing people who wrote original DocumentMarkerController class.
Tony Chang
Comment 3 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.
Hajime Morrita
Comment 4 2011-04-27 10:21:19 PDT
Hajime Morrita
Comment 5 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.
Tony Chang
Comment 6 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
Hajime Morrita
Comment 7 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.
Hajime Morrita
Comment 8 2011-04-27 16:09:11 PDT
WebKit Review Bot
Comment 9 2011-04-27 16:54:22 PDT
http://trac.webkit.org/changeset/85118 might have broken Qt Linux Release
Note You need to log in before you can comment on or make changes to this bug.