Bug 108498 – [Chromium] Add two-word misspelling to mock spellchecker

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

[Rouslan Solomakhin]

[Chromium] Add two-word misspelling to mock spellchecker

[Rouslan Solomakhin]

Created attachment 185809 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 185809 [details]
Patch

This is the part of https://webkit.org/b/106815 that makesediting/spelling/spelling-exactly-selected-multiple-words pass in Chromium.

[Tony Chang]

Comment on attachment 185809 [details]
Patch

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

You can use "webkit-patch land-safely" to reupload the patch and forward on the r+.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:76
> +        int maxWordLength = static_cast<int>(stringText.length()) - wordOffset;
> +        int wordLength;

I think you would have fewer casts if you made all the length types unsigned.  Conceptually, a negative length doesn't make sense.

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:167
> +        m_misspelledWords.append(WTF::String::fromUTF8(misspelledWords[i]));

Nit: remove WTF::

> Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h:73
> +    Vector<WTF::String> m_misspelledWords;

Nit: remove WTF::

[Rouslan Solomakhin]

Created attachment 185831 [details]
Patch for landing

[Rouslan Solomakhin]

(In reply to comment #3)
> You can use "webkit-patch land-safely" to reupload the patch and forward on the r+.

I ran "webkit-patch land-safely", but I think it did not produce the desired effect. It said that do not have EditBugs permission and my login is not listed in committers.py.

[Rouslan Solomakhin]

(In reply to comment #3)
> > Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:76
> > +        int maxWordLength = static_cast<int>(stringText.length()) - wordOffset;
> > +        int wordLength;
> 
> I think you would have fewer casts if you made all the length types unsigned.  Conceptually, a negative length doesn't make sense.

As we discussed offline, keeping int types because WTF::String::find() returns int types that can be negative.

> > Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp:167
> > +        m_misspelledWords.append(WTF::String::fromUTF8(misspelledWords[i]));
> 
> Nit: remove WTF::

Done.

> > Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.h:73
> > +    Vector<WTF::String> m_misspelledWords;
> 
> Nit: remove WTF::

Done.

[Ryosuke Niwa]

This patch modifies the actual code, not just the mock code, doesn't it?

[Rouslan Solomakhin]

Comment on attachment 185831 [details]
Patch for landing

Wrong patch, sorry.

[Rouslan Solomakhin]

Created attachment 185832 [details]
Patch for landing

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #3)
> > You can use "webkit-patch land-safely" to reupload the patch and forward on the r+.
> 
> I ran "webkit-patch land-safely", but I think it did not produce the desired effect. It said that do not have EditBugs permission and my login is not listed in committers.py.

It worked.  It added "Reviewed by Tony Chang" and it set cq?.  My guess is the editbugs permission was because it tried to assign the bug to you.  The committers.py was probably a warning.  If you pasted the full error message, that would be helpful to diagnose.

It sets cq? because you're not a committer yet.  Once you get commit, it will set cq+.

[Rouslan Solomakhin]

(In reply to comment #10)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > You can use "webkit-patch land-safely" to reupload the patch and forward on the r+.
> > 
> > I ran "webkit-patch land-safely", but I think it did not produce the desired effect. It said that do not have EditBugs permission and my login is not listed in committers.py.
> 
> It worked.  It added "Reviewed by Tony Chang" and it set cq?.  My guess is the editbugs permission was because it tried to assign the bug to you.  The committers.py was probably a warning.  If you pasted the full error message, that would be helpful to diagnose.
> 
> It sets cq? because you're not a committer yet.  Once you get commit, it will set cq+.

Reading your explanation makes me think that the behavior was correct. Here is the command output, just to make sure:

$ ./Tools/Scripts/webkit-patch land-safely
Fetching: https://bugs.webkit.org/show_bug.cgi?id=108498&ctype=xml&excludefield=attachmentdata
No reviewed patches on bug 108498, cannot infer reviewer.
Failed to guess reviewer from bug 108498 and --reviewer= not provided.  Not updating ChangeLogs with reviewer.
Fetching: https://bugs.webkit.org/show_bug.cgi?id=108498&ctype=xml&excludefield=attachmentdata
Fetching: https://bugs.webkit.org/show_bug.cgi?id=108498&ctype=xml&excludefield=attachmentdata
Logging in as [e-mail]...
Assigning bug 108498 to [e-mail]
Failed to assign bug to you (can't find assigned_to) control.
Do you have EditBugs privileges at bugs.webkit.org?
https://bugs.webkit.org/userprefs.cgi?tab=permissions

If not, you should email [e-mail] or ask in #webkit
for someone to add EditBugs to your bugs.webkit.org account.
Adding patch "Patch for landing" to https://bugs.webkit.org/show_bug.cgi?id=108498
Your Bugzilla login is not listed in committers.py. Uploading with cq? instead of cq+

[WebKit Review Bot]

Comment on attachment 185832 [details]
Patch for landing

Clearing flags on attachment: 185832

Committed r141471: <http://trac.webkit.org/changeset/141471>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.