WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(33.79 KB, patch)
2011-04-27 10:21 PDT
,
Hajime Morrita
tony
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Hajime Morrita
Comment 1
2011-04-19 11:48:23 PDT
Created
attachment 90231
[details]
Patch
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
Created
attachment 91309
[details]
Patch
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
Committed
r85118
: <
http://trac.webkit.org/changeset/85118
>
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.
Top of Page
Format For Printing
XML
Clone This Bug