Bug 108498

Summary: [Chromium] Add two-word misspelling to mock spellchecker
Product: WebKit Reporter: Rouslan Solomakhin <rouslan+webkit>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: groby, jochen, rniwa, rouslan+webkit, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106815, 108525    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Rouslan Solomakhin 2013-01-31 10:33:09 PST
[Chromium] Add two-word misspelling to mock spellchecker
Comment 1 Rouslan Solomakhin 2013-01-31 10:38:09 PST
Created attachment 185809 [details]
Patch
Comment 2 Rouslan Solomakhin 2013-01-31 10:39:55 PST
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.
Comment 3 Tony Chang 2013-01-31 11:23:55 PST
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::
Comment 4 Rouslan Solomakhin 2013-01-31 11:51:47 PST
Created attachment 185831 [details]
Patch for landing
Comment 5 Rouslan Solomakhin 2013-01-31 11:54:24 PST
(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.
Comment 6 Rouslan Solomakhin 2013-01-31 12:02:05 PST
(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.
Comment 7 Ryosuke Niwa 2013-01-31 12:03:18 PST
This patch modifies the actual code, not just the mock code, doesn't it?
Comment 8 Rouslan Solomakhin 2013-01-31 12:03:34 PST
Comment on attachment 185831 [details]
Patch for landing

Wrong patch, sorry.
Comment 9 Rouslan Solomakhin 2013-01-31 12:04:53 PST
Created attachment 185832 [details]
Patch for landing
Comment 10 Tony Chang 2013-01-31 12:25:44 PST
(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+.
Comment 11 Rouslan Solomakhin 2013-01-31 12:31:57 PST
(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 rouslan+webkit@chromium.org...
Assigning bug 108498 to rouslan+webkit@chromium.org
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 webkit-committers@lists.webkit.org 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+
Comment 12 WebKit Review Bot 2013-01-31 13:00:25 PST
Comment on attachment 185832 [details]
Patch for landing

Clearing flags on attachment: 185832

Committed r141471: <http://trac.webkit.org/changeset/141471>
Comment 13 WebKit Review Bot 2013-01-31 13:00:30 PST
All reviewed patches have been landed.  Closing bug.