Bug 110208 – Allow to retrieve the request data from abstract TextCheckingRequest to be accessible for WK2

• Component: HTML Editing
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

The WebCore changes are required to implement asynchronous spell checking in WK2. The idea of asynchronous spell checking in WK1 is to pass the pointer to the abstract object to the client who is able to verify the given text and notify the WebCore about results. WK2 will extract the request data from TextCheckingRequest and pass it (with additional information) to the UIProcess.

[Grzegorz Czajkowski]

Created attachment 189054 [details]
proposed patch

[Hajime Morrita]

Comment on attachment 189054 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189054&action=review

> Source/WebCore/platform/text/TextChecking.h:119
> +class TextCheckingRequest : public RefCounted<TextCheckingRequest>, public TextCheckingRequestData {

Using inheritance here should be avoidable if possible. Why not just turns it a member?
This looks a typical composition over inheritance situation.

[Hajime Morrita]

(In reply to comment #2)
> Using inheritance here should be avoidable if possible. Why not just turns it a member?
s/avoidable/avoided/

[Grzegorz Czajkowski]

Created attachment 189279 [details]
TextCheckingRequestData as a member of TextCheckingRequest

[Build Bot]

Comment on attachment 189279 [details]
TextCheckingRequestData as a member of TextCheckingRequest

Attachment 189279 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16655269

[Grzegorz Czajkowski]

Created attachment 189298 [details]
hopefully fixes Mac build

[Hajime Morrita]

Comment on attachment 189298 [details]
hopefully fixes Mac build

View in context: https://bugs.webkit.org/attachment.cgi?id=189298&action=review

Looks good overall. Could you polish a bit more?

> Source/WebCore/platform/text/TextChecking.h:91
> +const int unrequestedSequence = -1;

Could you move this into some classes or change the name a bit more verbose?
The name is too ambiguous to be WebCore global.

> Source/WebCore/platform/text/TextChecking.h:130
> +    const TextCheckingRequestData& data() const { return m_requestData; }

Looks like we no longer need to have m_requestData in this class. It can be pushed down to SpellCheckRequest then data() can be pure virtual.

[Grzegorz Czajkowski]

Created attachment 189488 [details]
apply Morrita's suggestions

[Grzegorz Czajkowski]

(In reply to comment #7)
> (From update of attachment 189298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189298&action=review
> 
> Looks good overall. Could you polish a bit more?
> 
> > Source/WebCore/platform/text/TextChecking.h:91
> > +const int unrequestedSequence = -1;> 
> Could you move this into some classes or change the name a bit more verbose?
> The name is too ambiguous to be WebCore global.

Sure, changed to 'unrequestedTextCheckingSequence'.

> 
> > Source/WebCore/platform/text/TextChecking.h:130
> > +    const TextCheckingRequestData& data() const { return m_requestData; }
> 
> Looks like we no longer need to have m_requestData in this class. It can be pushed down to SpellCheckRequest then data() can be pure virtual.

Definitely, done. Thanks for great review.

Is it ok for you now?

[Hajime Morrita]

Comment on attachment 189488 [details]
apply Morrita's suggestions

thanks for the update!

[WebKit Review Bot]

Comment on attachment 189488 [details]
apply Morrita's suggestions

Clearing flags on attachment: 189488

Committed r143688: <http://trac.webkit.org/changeset/143688>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.