Bug 59855 – DocumentMarker: Type specific details should be separately held by other object.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 93753 [details]
Patch

[Hajime Morrita]

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.

[Jia Pu]

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.

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 94364 [details]
Patch

[Darin Adler]

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.

[Hajime Morrita]

Created attachment 94396 [details]
Patch

[Hajime Morrita]

> > 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.

[Hajime Morrita]

Created attachment 102499 [details]
Patch

[Ryosuke Niwa]

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 ([e-mail])
> + *           (C) 1999 Antti Koivisto ([e-mail])
> + *           (C) 2001 Dirk Mueller ([e-mail])

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.

[Hajime Morrita]

Created attachment 102741 [details]
Patch

[Hajime Morrita]

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 ([e-mail])
> > + *           (C) 1999 Antti Koivisto ([e-mail])
> > + *           (C) 2001 Dirk Mueller ([e-mail])
> 
> 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.

[Ryosuke Niwa]

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.

[Hajime Morrita]

Created attachment 103015 [details]
Patch

[Hajime Morrita]

Created attachment 103019 [details]
Patch

[Ryosuke Niwa]

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
> [phone]  Mark Pilgrim  <[e-mail]>
> +
> +        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.

??

[Hajime Morrita]

Created attachment 103020 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Comment on attachment 103020 [details]
Patch

Clearing flags on attachment: 103020

Committed r92441: <http://trac.webkit.org/changeset/92441>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.