DocumentMarker::description and DocumentMarker::activeMatch are never used at the same time. Thus move to separate polymorphic object whose implementation is determined by DocumentMarker::type.
Created attachment 93753 [details] Patch
This is a part of effort for implementing Spellcheck API (Bug 59693) I'll add a new marker type and a details class for the API.
Comment on attachment 93753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93753&action=review > Source/WebCore/dom/DocumentMarker.h:109 > + bool isDescriptionAllowed() const { return m_type == Grammar || m_type == Autocorrected; } Morita, the autocorrection panel implementation on Mac relies heavily on description in marker of following types, Replacement, Autocorrected and DeletedAutocorrection. It seems what we need here is a way to allow different platforms to extend the marker types, in addition to a minimum set of base marker types. For instance, in order to implement the autocorrection panel, I have added 5 new marker types, which aren't really useful for other platforms.
Hi Jia, thank you for taking a look! > Morita, the autocorrection panel implementation on Mac relies heavily on description in marker of following types, Replacement, Autocorrected and DeletedAutocorrection. > Wow, so I should add these (at least), or... > It seems what we need here is a way to allow different platforms to extend the marker types, in addition to a minimum set of base marker types. For instance, in order to implement the autocorrection panel, I have added 5 new marker types, which aren't really useful for other platforms. it might be better to move m_type to DocumentMarkerDetails as a type-tag. That means, we will have MarkerType DocumentMarkerDetails::type() bool DocumentMarkerDetails::hasDescription() .... I'll try that way.
Created attachment 94364 [details] Patch
Comment on attachment 94364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94364&action=review > Source/WebCore/dom/DocumentMarker.h:105 > + PassRefPtr<DocumentMarkerDetails> details() const { return m_details; } I did not yet have a chance to review this patch, but this function should not be returning a PassRefPtr. It should return a raw pointer. Only a function that passes ownership should return a PassRefPtr.
Created attachment 94396 [details] Patch
> > Source/WebCore/dom/DocumentMarker.h:105 > > + PassRefPtr<DocumentMarkerDetails> details() const { return m_details; } > > I did not yet have a chance to review this patch, but this function should not be returning a PassRefPtr. It should return a raw pointer. Only a function that passes ownership should return a PassRefPtr. Hi Darin, Thank you for the catch! I was confused but now fixed it.
Created attachment 102499 [details] Patch
Comment on attachment 102499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102499&action=review I'm not sure if this refactoring makes the code more obvious than it is now because of verbosity and indirection introduced in the class hierarchy. > Source/WebCore/dom/DocumentMarker.cpp:4 > + * Copyright (C) 1999 Lars Knoll (knoll@kde.org) > + * (C) 1999 Antti Koivisto (koivisto@kde.org) > + * (C) 2001 Dirk Mueller (mueller@kde.org) Really? These people touched this file? > Source/WebCore/dom/DocumentMarker.cpp:8 > + * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/) > + * Copyright (C) Research In Motion Limited 2010. All rights reserved. And these companies? > Source/WebCore/dom/DocumentMarker.cpp:86 > + | DocumentMarker::Grammar > + | DocumentMarker::Replacement > + | DocumentMarker::Autocorrected > + | DocumentMarker::DeletedAutocorrection); Nit: deep indentation. > Source/WebCore/dom/DocumentMarker.h:151 > + virtual const String& description() const; > + virtual bool activeMatch() const; It seems odd that the abstract class has interface for derived classes.
Created attachment 102741 [details] Patch
Hi Ryosuke, thanks for taking a look! > I'm not sure if this refactoring makes the code more obvious than it is now because of verbosity and indirection introduced in the class hierarchy. Well, this is a preparation for add another type of metadata. I'm going to put suggested word list to the spellcheck marker but don't want to waste extra space with it on non-spellchecking markers. So I splited metadata out to type specific objects. > > > Source/WebCore/dom/DocumentMarker.cpp:4 > > + * Copyright (C) 1999 Lars Knoll (knoll@kde.org) > > + * (C) 1999 Antti Koivisto (koivisto@kde.org) > > + * (C) 2001 Dirk Mueller (mueller@kde.org) > > Really? These people touched this file? Replaced with a correct one. > > > Source/WebCore/dom/DocumentMarker.cpp:86 > > + | DocumentMarker::Grammar > > + | DocumentMarker::Replacement > > + | DocumentMarker::Autocorrected > > + | DocumentMarker::DeletedAutocorrection); > > Nit: deep indentation. Removed redundant constructor call. > > > Source/WebCore/dom/DocumentMarker.h:151 > > + virtual const String& description() const; > > + virtual bool activeMatch() const; > > It seems odd that the abstract class has interface for derived classes. Pushed down to the subclasses.
Comment on attachment 102741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102741&action=review > Source/WebCore/dom/DocumentMarker.cpp:38 > +DocumentMarkerDetails::DocumentMarkerDetails() > +{ > +} We can make this constructor inline. > Source/WebCore/dom/DocumentMarker.cpp:75 > + | DocumentMarker::Grammar > + | DocumentMarker::Replacement > + | DocumentMarker::Autocorrected > + | DocumentMarker::DeletedAutocorrection); Nit: Still too deep. Should be indented by 4 spaces. > Source/WebCore/dom/DocumentMarker.cpp:135 > + : m_type(type), m_startOffset(startOffset), m_endOffset(endOffset), m_details(DocumentMarkerDescription::createUnlessEmpty(description)) This is a really long line. Split into two? And I'd prefer having a tertiary here and have a regular create in DocumentMarkerDescription. > Source/WebCore/dom/DocumentMarker.cpp:150 > +DocumentMarker::DocumentMarker(MarkerType type, unsigned startOffset, unsigned endOffset, PassRefPtr<DocumentMarkerDetails> details) > + : m_type(type), m_startOffset(startOffset), m_endOffset(endOffset), m_details(details) > +{ > + ASSERT(!m_details || m_details->isAllowedFor(m_type)); > +} Is this constructor used somewhere? It appears that this is the only reason we need isAllowedFor and compatibleTypes because we know the exact type of details in all other cases. > Source/WebCore/dom/DocumentMarker.cpp:155 > + m_endOffset += delta; Nit: two spaces after +=. > Source/WebCore/dom/DocumentMarker.h:141 > +// A superclass of marker type specific metadata. > +// > +// Note that every details subclasses should know about description > +// and activeMatch as a least common denominator. It seems like this comment repeats what the code says. Please remove. > Source/WebCore/dom/DocumentMarker.h:146 > +class DocumentMarkerDetails : public RefCounted<DocumentMarkerDetails> > { > -} > +public: > + DocumentMarkerDetails(); > + virtual ~DocumentMarkerDetails(); Do we need to expose this class in the header file at all? If not, I'd prefer it be moved into DocumentMarker.cpp.
Created attachment 103015 [details] Patch
Created attachment 103019 [details] Patch
Comment on attachment 103019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103019&action=review What's up with all those idl changes? > Source/WebCore/ChangeLog:60 > +2011-08-04 Mark Pilgrim <pilgrim@chromium.org> > + > + Remove LegacyDefaultOptionalArguments flag from DOM-related files except Document.idl > + https://bugs.webkit.org/show_bug.cgi?id=65715 > + > + Reviewed by Adam Barth. > + > + No new tests, all existing tests pass. ??
Created attachment 103020 [details] Patch
Comment on attachment 102741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102741&action=review Hi Ryosuke, thanks for another round! I updated the patch. >> Source/WebCore/dom/DocumentMarker.cpp:38 >> +} > > We can make this constructor inline. Done. >> Source/WebCore/dom/DocumentMarker.cpp:75 >> + | DocumentMarker::DeletedAutocorrection); > > Nit: Still too deep. Should be indented by 4 spaces. Done. >> Source/WebCore/dom/DocumentMarker.cpp:135 >> + : m_type(type), m_startOffset(startOffset), m_endOffset(endOffset), m_details(DocumentMarkerDescription::createUnlessEmpty(description)) > > This is a really long line. Split into two? And I'd prefer having a tertiary here and have a regular create in DocumentMarkerDescription. Done. >> Source/WebCore/dom/DocumentMarker.cpp:150 >> +} > > Is this constructor used somewhere? It appears that this is the only reason we need isAllowedFor and compatibleTypes because we know the exact type of details in all other cases. Good point. I removed this constructor. > Source/WebCore/dom/DocumentMarker.cpp:156 > +} Oops. removed. >> Source/WebCore/dom/DocumentMarker.h:141 >> +// and activeMatch as a least common denominator. > > It seems like this comment repeats what the code says. Please remove. Removed. >> Source/WebCore/dom/DocumentMarker.h:146 >> + virtual ~DocumentMarkerDetails(); > > Do we need to expose this class in the header file at all? If not, I'd prefer it be moved into DocumentMarker.cpp. We need this definition because DocumentMarker is copiable (we copy it actually) and the implicit copy constructor should know DocumentMarkerDetail is a subclass of RefCounted.
Comment on attachment 103020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103020&action=review > Source/WebCore/dom/DocumentMarker.cpp:59 > +PassRefPtr<DocumentMarkerDescription> DocumentMarkerDescription::create(const String& description) > +{ > + return adoptRef(new DocumentMarkerDescription(description)); > +} Should we make this inline? > Source/WebCore/dom/DocumentMarker.cpp:96 > + if (details && details->isTextMatch()) > + return static_cast<DocumentMarkerTextMatch*>(details); > + return 0; I would have used tertiary for this. > Source/WebCore/dom/DocumentMarker.cpp:101 > + : m_type(Spelling), m_startOffset(0), m_endOffset(0) Shouldn't they be on separate lines? > Source/WebCore/dom/DocumentMarker.cpp:106 > + : m_type(type), m_startOffset(startOffset), m_endOffset(endOffset) Ditto.
Comment on attachment 103020 [details] Patch Clearing flags on attachment: 103020 Committed r92441: <http://trac.webkit.org/changeset/92441>
All reviewed patches have been landed. Closing bug.