Bug 83549 – Introducing DictationCommand.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

This is preparation for bug 83047 and bug 82503. On OS X, alternative dictation results are stored in markers. So we need to be able to insert text along with markers attached it.

[Jia Pu]

Created attachment 136402 [details]
Patch (v1)

[Jia Pu]

Created attachment 136408 [details]
Update to TOT.

[WebKit Review Bot]

Comment on attachment 136408 [details]
Update to TOT.

Attachment 136408 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12368676

[Build Bot]

Comment on attachment 136408 [details]
Update to TOT.

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

[Philippe Normand]

Comment on attachment 136408 [details]
Update to TOT.

Attachment 136408 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12371853

[Build Bot]

Comment on attachment 136408 [details]
Update to TOT.

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

[Collabora GTK+ EWS bot]

Comment on attachment 136408 [details]
Update to TOT.

Attachment 136408 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12376393

[Jia Pu]

Created attachment 136457 [details]
Fix build failures.

[WebKit Review Bot]

Comment on attachment 136457 [details]
Fix build failures.

Attachment 136457 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12383440

[Jia Pu]

Created attachment 136580 [details]
Fix build failure on Chromium.

[Hajime Morrita]

Comment on attachment 136457 [details]
Fix build failures.

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

> Source/WebCore/editing/AlternativeTextController.h:92
> +#if USE(DICTATION_ALTERNATIVES)

Do we need DICTATION_ALTERNATIVES separately here? 
We already have many code outside DICTATION_ALTERNATIVES. 
Omitting DICTATION_ALTERNATIVES looks fine and good for preventing build breakage.

> Source/WebCore/editing/DictationAlternative.h:33
> +    DictationAlternative(const DictationAlternative& another);

Do we need to define this copy constructor explicitly? the definition looks basically same as what compilers do.

> Source/WebCore/editing/DictationCommand.cpp:32
> +#include "DocumentMarker.h"

It looks we also needs Element.h to rescue cr-linux build.

> Source/WebCore/editing/DictationCommand.cpp:57
> +    DictationCommand* dictationCommand;

Please follow the class naming convention. That will eliminate any needs of "a" prefix....

> Source/WebCore/editing/DictationCommand.cpp:81
> +            startNode->rootEditableElement()->dispatchEvent(evt, ec);

The event can be prevented. We need to stop editing stuff in that case.

> Source/WebCore/editing/DictationCommand.cpp:101
> +        frame->selection()->setSelection(currentSelection);

It looks currentSelection can be invalid here because we've inserted the text.

> Source/WebCore/editing/StringHelper.h:27
> +#define StringHelper_h

The filename is too generic considering that WebKit includes stuff using base name. 
Maybe this is OK to be in htmlediting.h since this is used for that purpose.

> Source/WebCore/editing/StringHelper.h:36
> +LineOperation forEachLineInString(const String& string, LineOperation operation)

Do we need to return stuff? Also, can operation  be const reference?

> Source/WebCore/editing/TypingCommand.cpp:74
> +    TypingCommand* typingCommand;

Please follow usual WebKit naming convention which starts names from m_. 
I thinks this struct is totally fine to be a class.

> Source/WebCore/rendering/InlineTextBox.cpp:1003
> +void InlineTextBox::paintSpellingGrammarDictationMarker(GraphicsContext* pt, const FloatPoint& boxOrigin, DocumentMarker* marker, RenderStyle* style, const Font& font, bool grammar)

It's time to figure out some generic name like DocumentMaker or TextMarker.

[Hajime Morrita]

Comment on attachment 136580 [details]
Fix build failure on Chromium.

Oops. sorry for overlapping. Please see the comment above.

[Jia Pu]

(In reply to comment #11)
> (From update of attachment 136457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136457&action=review

Thanks for reviewing, Hajime.
> 
> > Source/WebCore/editing/DictationCommand.cpp:81
> > +            startNode->rootEditableElement()->dispatchEvent(evt, ec);
> 
> The event can be prevented. We need to stop editing stuff in that case.

Just to make sure, since I'm not familiar with this event code. Do you mean I need check the return value of dispatchEvent()?

> 
> > Source/WebCore/editing/DictationCommand.cpp:101
> > +        frame->selection()->setSelection(currentSelection);

Actually I'm quite sure what this code does. I was following TypingCommand::insertText(). Could you elaborate what need to be done here?

[Jia Pu]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 136457 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136457&action=review
> 
> > 
> > > Source/WebCore/editing/DictationCommand.cpp:101
> > > +        frame->selection()->setSelection(currentSelection);
> 
> Actually I'm quite sure what this code does. I was following TypingCommand::insertText(). Could you elaborate what need to be done here?

sorry, typo, i meant to say "I'm not quite sure ..."

[Hajime Morrita]

Comment on attachment 136457 [details]
Fix build failures.

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

>>> Source/WebCore/editing/DictationCommand.cpp:81
>>> +            startNode->rootEditableElement()->dispatchEvent(evt, ec);
>> 
>> The event can be prevented. We need to stop editing stuff in that case.
> 
> Just to make sure, since I'm not familiar with this event code. Do you mean I need check the return value of dispatchEvent()?

Oops. I was confused this with TextEvent. This event cannot be prevented. Please ignore my comment. I'm sorry for the confusion.

>>>> Source/WebCore/editing/DictationCommand.cpp:101
>>>> +        frame->selection()->setSelection(currentSelection);
>>> 
>>> It looks currentSelection can be invalid here because we've inserted the text.
>> 
>> Actually I'm quite sure what this code does. I was following TypingCommand::insertText(). Could you elaborate what need to be done here?
> 
> sorry, typo, i meant to say "I'm not quite sure ..."

Hmm. It looks this code is based on TypingCommand::insertText() and the original code also looks strange....
Maybe this is OK for now. I hope we could share the code between TypingCommand::insertText and DictationCommand::inserText() somehow.

[Jia Pu]

Created attachment 136770 [details]
Addressed review feedback.

[Jia Pu]

(In reply to comment #16)
> Created an attachment (id=136770) [details]
> Addressed review feedback.

This patch addressed feedback in comment 11. The primary difference from previous patches is the addition of a new class TextInsertionBaseCommand, which derives from CompositeEditCommand and serves as base class of TypingCommand and DictationCommand. TextInsertionBaseCommand.h declares several functions shared by TypingCommand and DictationCommand. These functions are used to be part inside of TypingCommand's member functions.

[Jia Pu]

Created attachment 136774 [details]
Fix build.

[Jia Pu]

Created attachment 136783 [details]
Fix build.

[Jia Pu]

Created attachment 136798 [details]
Fix build, again.

[WebKit Review Bot]

Comment on attachment 136798 [details]
Fix build, again.

Attachment 136798 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12393104

New failing tests:
fast/forms/number/input-number-commit-valid-only.html

[WebKit Review Bot]

Created attachment 136806 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 136798 [details]
Fix build, again.

Attachment 136798 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12390204

New failing tests:
fast/forms/number/input-number-commit-valid-only.html

[WebKit Review Bot]

Created attachment 136816 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Hi Jian,

Is it possible to put DocumentMarker creation behind DocumentMarkerController?
Letting InsertIntoTextNodeCommand to own DocumentMarker looks violating encapsulation.
DocumentMarker should be hidden behind DocumentMarkerController if possible.

How about to let InsertIntoTextNodeCommand to have Vector<DictationAlternative> instead of 
document markers?

I think adding either
- DocumentMarkerController::addMarker(Node*, size_t offset, const DictationAlternative&) or
- DocumentMarkerController::addMarker(Range*, const DictationAlternative&)
is fine then. 

Another possible approach would be to let DictationCommand itself add makers,
instead of asking TextInsertionCommand to do it.
In that case, we can extend TextInsertionCommand a a bit
to remember the inserted position (node and offset),
then DictationCommand can add markers based on that position.

What do you think?

[Jia Pu]

(In reply to comment #25)
> Hi Jian,
> 
> Is it possible to put DocumentMarker creation behind DocumentMarkerController?
> Letting InsertIntoTextNodeCommand to own DocumentMarker looks violating encapsulation.
> DocumentMarker should be hidden behind DocumentMarkerController if possible.

Current arrangement avoids making it necessary for DocumentMarkerController to know about marker detail implementation, in this case DictationMarkerDetails. Seems to me, the responsibility of DocumentMarkerController is storing markers, keeping track the location of markers, and removing markers when associated range is removed. It's not necessarily wrong to create marker objects outside of DocumentMarkerController, then pass in the ownership.

> 
> How about to let InsertIntoTextNodeCommand to have Vector<DictationAlternative> instead of 
> document markers?

Current change to InsertIntoTextNodeCommand allows us to pass in any marker object. It may be useful for other scenarios that isn't related to dictation in the future. If we change it to take DictationAlternative objects, then this interface becomes very much specialized. 

> 
> I think adding either
> - DocumentMarkerController::addMarker(Node*, size_t offset, const DictationAlternative&) or
> - DocumentMarkerController::addMarker(Range*, const DictationAlternative&)
> is fine then. 
> 
> Another possible approach would be to let DictationCommand itself add makers,
> instead of asking TextInsertionCommand to do it.
> In that case, we can extend TextInsertionCommand a a bit
> to remember the inserted position (node and offset),
> then DictationCommand can add markers based on that position.

This seems require DictationCommand to keep track off the TextInsertionCommand objects it created, and query them after they have been added to composite command and been applied. As far as I can tell, it's not very easy to do.

> 
> What do you think?

Overall, I feel that current implementation is perhaps the best balance among all comprises. If you prefer not creating marker object out side of marker controller. I will go with your first proposal.

[Hajime Morrita]

(In reply to comment #26)
> (In reply to comment #25)
> > Hi Jian,
> > 
> > Is it possible to put DocumentMarker creation behind DocumentMarkerController?
> > Letting InsertIntoTextNodeCommand to own DocumentMarker looks violating encapsulation.
> > DocumentMarker should be hidden behind DocumentMarkerController if possible.
> 
> Current arrangement avoids making it necessary for DocumentMarkerController to know about marker detail implementation, in this case DictationMarkerDetails. Seems to me, the responsibility of DocumentMarkerController is storing markers, keeping track the location of markers, and removing markers when associated range is removed. It's not necessarily wrong to create marker objects outside of DocumentMarkerController, then pass in the ownership.
> 
Agreed.

> > 
> > How about to let InsertIntoTextNodeCommand to have Vector<DictationAlternative> instead of 
> > document markers?
> 
> Current change to InsertIntoTextNodeCommand allows us to pass in any marker object. It may be useful for other scenarios that isn't related to dictation in the future. If we change it to take DictationAlternative objects, then this interface becomes very much specialized. 
> 
Makes sense. Ideally we could have something like AttributedString which can maintain string with markers.
But that's outside of the scope...

> > 
> > I think adding either
> > - DocumentMarkerController::addMarker(Node*, size_t offset, const DictationAlternative&) or
> > - DocumentMarkerController::addMarker(Range*, const DictationAlternative&)
> > is fine then. 

So does following make more sense?

- DocumentMarkerController::addMarker(Node*, size_t offset, PassRefPtr<DocumentMarkerDetail>) or
- DocumentMarkerController::addMarker(Range*, PassRefPtr<DocumentMarkerDetail>)

> > 
> > Another possible approach would be to let DictationCommand itself add makers,
> > instead of asking TextInsertionCommand to do it.
> > In that case, we can extend TextInsertionCommand a a bit
> > to remember the inserted position (node and offset),
> > then DictationCommand can add markers based on that position.
> 
> This seems require DictationCommand to keep track off the TextInsertionCommand objects it created, and query them after they have been added to composite command and been applied. As far as I can tell, it's not very easy to do.
Is it?
DictationCommand::insertTextRunWithoutNewlines() creates an InsertTextCommand object and apply it immediately. 
So we have the finished command there. What left we need to do is to add m_insertedNode and m_insertedOffset to InsertTextCommand and refer it.
Does this make sense?

> 
> > 
> > What do you think?
> 
> Overall, I feel that current implementation is perhaps the best balance among all comprises. If you prefer not creating marker object out side of marker controller. I will go with your first proposal.

Basically I agree what you are explaining.
If extending InsertTextCommand has any complexity I'm not aware of,  It's fine to go this way.

[Jia Pu]

> > > 
> > > I think adding either
> > > - DocumentMarkerController::addMarker(Node*, size_t offset, const DictationAlternative&) or
> > > - DocumentMarkerController::addMarker(Range*, const DictationAlternative&)
> > > is fine then. 
> 
> So does following make more sense?
> 
> - DocumentMarkerController::addMarker(Node*, size_t offset, PassRefPtr<DocumentMarkerDetail>) or
> - DocumentMarkerController::addMarker(Range*, PassRefPtr<DocumentMarkerDetail>)

I think this is better.

> 
> > > 
> > > Another possible approach would be to let DictationCommand itself add makers,
> > > instead of asking TextInsertionCommand to do it.
> > > In that case, we can extend TextInsertionCommand a a bit
> > > to remember the inserted position (node and offset),
> > > then DictationCommand can add markers based on that position.
> > 
> > This seems require DictationCommand to keep track off the TextInsertionCommand objects it created, and query them after they have been added to composite command and been applied. As far as I can tell, it's not very easy to do.
> Is it?
> DictationCommand::insertTextRunWithoutNewlines() creates an InsertTextCommand object and apply it immediately. 
> So we have the finished command there. What left we need to do is to add m_insertedNode and m_insertedOffset to InsertTextCommand and refer it.
> Does this make sense?

I will look closer at this, see what I can do.

[Jia Pu]

Created attachment 136832 [details]
Fixing a regressing caused by previous patch.

[Jia Pu]

(In reply to comment #29)
> Created an attachment (id=136832) [details]
> Fixing a regressing caused by previous patch.

Just want to check if the patch fixes the regression caused by previous patch. This patch doesn't contain the DocumentMarkerController change suggested by Hajime yet.

[Jia Pu]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > 
> > > Another possible approach would be to let DictationCommand itself add makers,
> > > instead of asking TextInsertionCommand to do it.
> > > In that case, we can extend TextInsertionCommand a a bit
> > > to remember the inserted position (node and offset),
> > > then DictationCommand can add markers based on that position.
> > 
> > This seems require DictationCommand to keep track off the TextInsertionCommand objects it created, and query them after they have been added to composite command and been applied. As far as I can tell, it's not very easy to do.
> Is it?
> DictationCommand::insertTextRunWithoutNewlines() creates an InsertTextCommand object and apply it immediately. 
> So we have the finished command there. What left we need to do is to add m_insertedNode and m_insertedOffset to InsertTextCommand and refer it.
> Does this make sense?
> 
> > 
> > > 
> > > What do you think?
> > 
> > Overall, I feel that current implementation is perhaps the best balance among all comprises. If you prefer not creating marker object out side of marker controller. I will go with your first proposal.
> 
> Basically I agree what you are explaining.
> If extending InsertTextCommand has any complexity I'm not aware of,  It's fine to go this way.

In InsertTextCommand::doApply(), we get the offset in node before calling insertTextIntoNode(). However this offset may become incorrect due to rebalancing whitespaces after the command is applied. To retrieve new offset, we would have to modify rebalanceWhitespaceOnTextSubstring() so that it returns new offset value. In contrast, if we add marker before doing rebalancing, as it does now, those marker will end up in correct position due to replaceTextInNodePreservingMarkers(), which is called by rebalanceWhitespaceOnTextSubstring().

If our goal here is to avoid having InsertTextCommand to own the markers or marker detail objects, another possibility is passing in a callback or functor when creating InsertTextCommand object. Then InsertTextCommand could ask the callback function or functor for marker details to add. The responsibility of creating marker details will be on the callback, or indirectly on DictationCommand.

Thought?

[Jia Pu]

Created attachment 136982 [details]
Reorganized interaction between DictationCommmand and InsertTextCommand.

[Jia Pu]

(In reply to comment #32)
> Created an attachment (id=136982) [details]
> Reorganized interaction between DictationCommmand and InsertTextCommand.

This patch is based on discussion in comment 25 and comment 31. The primary difference from previous patch is the reorganizing of interaction between DictationCommand and InsertTextCommand, and between InsertTextCommand and DocumentMarkerController. With this patch, we keep creation of marker objects inside of marker controller. And DictationCommand is the only class that needs to know about DictationMarkerDetails class. This also avoids changing current InsertIntoTextNodeCommand class.

In this patch, InsertTextCommand constructor optionally accept a TextInsertionMarkerSupplier object. After text is inserted, InsertTextCommand will ask the marker supplier to add document marker. DictationCommand implements a subclass of TextInsertionMarkerSupplier, and pass it toInsertTextCommand constructor.

[Build Bot]

Comment on attachment 136982 [details]
Reorganized interaction between DictationCommmand and InsertTextCommand.

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

[Jia Pu]

Created attachment 136988 [details]
Fix build.

[Hajime Morrita]

Comment on attachment 136988 [details]
Fix build.

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

The supplier is neat. This patch looks good. Could you take my small comments? Then I'll be happy to r+ this.

> Source/WebCore/editing/DictationCommand.h:39
> +    static void insertText(Document*, const String&, const Vector<DictationAlternative> alternatives, const VisibleSelection&);

This Vector could be a reference.

> Source/WebCore/editing/InsertTextCommand.h:39
> +    virtual void addMarkersToTextNode(Text*, Document*, unsigned offsetOfInsertion, const String& textInserted) = 0;

We don't need Document* since it can be retrieved via Text::document()

[Jia Pu]

Created attachment 137105 [details]
Minor changes based on review feedback.

[Jia Pu]

Created attachment 137211 [details]
Added two functions to DocumentMarkerController.

[Jia Pu]

(In reply to comment #38)
> Created an attachment (id=137211) [details]
> Added two functions to DocumentMarkerController.

This patch added two DocumentMarkerController functions: 

void addMarkerToNode(Node*, unsigned startOffset, unsigned length, DocumentMarker::MarkerType);
void addMarkerToNode(Node*, unsigned startOffset, unsigned length, DocumentMarker::MarkerType, PassRefPtr<DocumentMarkerDetails>);

for DictationMarkerSupplier to use. 

It seems existing Range based addMarker() functions don't work in this case. When I create Range object in DictationMarkerSupplier, then call addMarker(), the TextIterator in addMarker() would terminate the loop immediately.

[Hajime Morrita]

Comment on attachment 137211 [details]
Added two functions to DocumentMarkerController.

This one looks good.

[WebKit Review Bot]

Comment on attachment 137211 [details]
Added two functions to DocumentMarkerController.

Clearing flags on attachment: 137211

Committed r114220: <http://trac.webkit.org/changeset/114220>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.