Removed WebTextCheckingResult legacy API use.

Removed WebTextCheckingResult legacy API use.

BUG=none
TEST=unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122429

[Hajime Morrita, 2012-02-10 02:05:20]

Hi Bono-san, could you take a look at this when you have time?
This trivial change is just a cleanup change for replacing legacy API with new
one.

[Hironori Bono, 2012-02-10 06:44:20]

Greetings Morita-san,

Thanks for your change.
I notice we need to change 'content/public/common/webkit_param_traits.h' so we
can send WebKit::WebString while checking your change. Is it possible to add the
following code to the file, add avi and jam to the reviewers' list, and upload
your new change?

template <>
struct SimilarTypeTraits<WebKit::WebString> {
  typedef string16 Type;
};

Regards,

Hironori Bono

http://codereview.chromium.org/9368052/diff/4001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(right):

http://codereview.chromium.org/9368052/diff/4001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:66:
EXPECT_EQ(sent_results[0].error,
nit: sent_results[0].error -> sent_results[0].type?

http://codereview.chromium.org/9368052/diff/4001/chrome/common/spellcheck_mes...
File chrome/common/spellcheck_messages.h (right):

http://codereview.chromium.org/9368052/diff/4001/chrome/common/spellcheck_mes...
chrome/common/spellcheck_messages.h:19: IPC_STRUCT_TRAITS_MEMBER(length)
Do we need IPC_STRUCT_TRAITS_MEMBER(replacement)?

[Hajime Morrita, 2012-02-10 07:01:55]

Bono-san, thank you for taking a look... and catch the ball.
I updated the patch. New try shots are in flight.

[Hironori Bono, 2012-02-10 08:12:51]

Greetings Morita-san,

Thanks for your quick update. I notice SimilarTypeTraits causes a crash when
Reading a WebKit::WebString. Would it be possible to use ParamTraits instead of
SimilarTypeTraits and implement its methods as listed in my comment?

Regards,

Hironori Bono

http://codereview.chromium.org/9368052/diff/3002/content/public/common/webkit...
File content/public/common/webkit_param_traits.h (right):

http://codereview.chromium.org/9368052/diff/3002/content/public/common/webkit...
content/public/common/webkit_param_traits.h:235: struct
SimilarTypeTraits<WebKit::WebString> {
Sorry, my previous comment was wrong. This 'SimilarTypeTraits' should be
ParamTraits and we need to implement its methods as listed below. (I would
recommend them to add in 'webkit_param_traits.cc'.)

void ParamTraits<WebKit::WebString>::Write(Message* m,
                                           const param_type& p) {
  WriteParam(m, static_cast<string16>(p));
}

bool ParamTraits<WebKit::WebString>::Read(const Message* m,
                                          void** iter,
                                          param_type* p) {
  string16 string;
  if (!ReadParam(m, iter, &string))
    return false;
  p->assign(string);
  return true;
}

void ParamTraits<WebKit::WebString>::Log(const param_type& p,
                                         std::string* l) {
  LogParam(static_cast<string16>(p), l);
}

[Hajime Morrita, 2012-02-10 08:29:20]

Done. Thanks for the catch again!

[Hironori Bono, 2012-02-10 10:18:46]

LGTM (for chrome/browser/spellchecker and chrome/common/).

Greetings Avi and John,

Would it be possible to review its 'content' code and its 'browser/tab_contents'
one? The 'content' code is for directly sending WebKit::WebString via IPC. The
'tab_contents' code is for using a new parameter added by his WebKit change.

Regards,

Hironori Bono

[jam, 2012-02-10 18:00:09]

http://codereview.chromium.org/9368052/diff/4005/content/public/common/webkit...
File content/public/common/webkit_param_traits.h (right):

http://codereview.chromium.org/9368052/diff/4005/content/public/common/webkit...
content/public/common/webkit_param_traits.h:235: struct CONTENT_EXPORT
ParamTraits<WebKit::WebString> {
I think it was by design that we didn't have this before, i.e. we tried to avoid
using webkit types in chrome code, and only just when talking to WebKit API.

I'm CCing Darin for his thoughts.

[darin (slow to review), 2012-02-13 17:40:43]

http://codereview.chromium.org/9368052/diff/4005/content/public/common/webkit...
File content/public/common/webkit_param_traits.h (right):

http://codereview.chromium.org/9368052/diff/4005/content/public/common/webkit...
content/public/common/webkit_param_traits.h:237: static void Write(Message* m,
const param_type& p);
yes, please please please don't add this.  WebString is not thread-safe.  you
should not be using it in chromium code except at the interface with WebKit.  it
is really too easy via PostTask to send a WebString to another thread.  that
will cause double-free bugs.

[darin (slow to review), 2012-02-13 17:42:37]

http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
File chrome/common/spellcheck_messages.h (right):

http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
chrome/common/spellcheck_messages.h:14:
IPC_ENUM_TRAITS(WebKit::WebTextCheckingType)
serialization of webkit stuff should be in webkit_param_traits.h

since WebTextCheckingResult now has a WebString, it means you probably need to
create a new structure to copy the WebTextCheckingResult into :-(

[gmorrita, 2012-02-14 11:25:09]

On 2012/02/13 17:42:37, darin wrote:
>
http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
> File chrome/common/spellcheck_messages.h (right):
> 
>
http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
> chrome/common/spellcheck_messages.h:14:
> IPC_ENUM_TRAITS(WebKit::WebTextCheckingType)
> serialization of webkit stuff should be in webkit_param_traits.h
Fixed.

> 
> since WebTextCheckingResult now has a WebString, it means you probably need to
> create a new structure to copy the WebTextCheckingResult into :-(
Yeah, that's a sad fact....

[jam, 2012-02-14 16:15:10]

On 2012/02/14 11:25:09, morrita wrote:
> On 2012/02/13 17:42:37, darin wrote:
> >
>
http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
> > File chrome/common/spellcheck_messages.h (right):
> > 
> >
>
http://codereview.chromium.org/9368052/diff/4005/chrome/common/spellcheck_mes...
> > chrome/common/spellcheck_messages.h:14:
> > IPC_ENUM_TRAITS(WebKit::WebTextCheckingType)
> > serialization of webkit stuff should be in webkit_param_traits.h
> Fixed.
> 
> > 
> > since WebTextCheckingResult now has a WebString, it means you probably need
to
> > create a new structure to copy the WebTextCheckingResult into :-(
> Yeah, that's a sad fact....

as a hack to avoid creating a wrapper struct, can't you keep the existing traits
and not add an entry for the string, and also send the string as another
argument to the IPC?

[gmorrita, 2012-02-15 04:57:20]

Hi John, thanks for the comment.

> as a hack to avoid creating a wrapper struct, can't you keep the existing
traits
> and not add an entry for the string, and also send the string as another
> argument to the IPC?
Sending the string sounds too hacky considering we are sending a vector of
WebTextCheckingResult.  On the other hand, I agree that we shouldn't have
ParamTraits<WebString> for avoiding WebKit API abuse. 

So I inlined the WebString serialization code into
ParamTraits<WebTextCheckingResult> method. This looks reasonable "hack" for me.
What do you think?

[darin (slow to review), 2012-02-15 05:54:26]

http://codereview.chromium.org/9368052/diff/21001/content/public/common/webki...
File content/public/common/webkit_param_traits.cc (right):

http://codereview.chromium.org/9368052/diff/21001/content/public/common/webki...
content/public/common/webkit_param_traits.cc:333:
p->replacement.assign(replacement);
I don't think we want this.  It means creating a WebString in the browser
process.

Some suggestions:
1- Break replacement out of WebTextCheckingResult.  (This sounds bad since you
want arrays of WebTextCheckingResult objects.)
2- Define a similar struct for use in Chromium.  Copy WebTextCheckingResult to
that struct.

I'd probably go with #2.

[Hajime Morrita, 2012-02-16 02:45:46]

> Some suggestions:
> 1- Break replacement out of WebTextCheckingResult.  (This sounds bad since you
> want arrays of WebTextCheckingResult objects.)
> 2- Define a similar struct for use in Chromium.  Copy WebTextCheckingResult to
> that struct.
> 
> I'd probably go with #2.
OK, it's time to pay for the leakage! I updated the patch for this #2 direction.
Darin, could you take a look to see if this goes the right direction?

The change introduced SpellCheckResult class which mirrors WebTextCheckingResult
and whole chrome codebase except the bridge classes (SpellCheckProvider)
switches to
use it.

[Hironori Bono, 2012-02-16 04:22:45]

Morita-san,

Many thanks for your hard work.
These comments are just for fixing build breaks on try bots.

Regards,

Hironori Bono

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
File chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
chrome/browser/spellchecker/spellcheck_message_filter_mac_browsertest.cc:67:
SpellCheckResut::Spelling);
nit: SpellCheckResut -> SpellCheckResult?

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
File chrome/browser/spellchecker/spellcheck_platform_mac.mm (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
chrome/browser/spellchecker/spellcheck_platform_mac.mm:75:
SpellCheckResut::Spelling,
nit: SpellCheckResut -> SpellCheckResult?

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
File chrome/browser/spellchecker/spelling_service_client.h (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/spel...
chrome/browser/spellchecker/spelling_service_client.h:20: class
SpellCheckResult;
nit: class -> struct.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/tab_...
File chrome/browser/tab_contents/spelling_menu_observer.h (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/browser/tab_...
chrome/browser/tab_contents/spelling_menu_observer.h:18: class SpellCheckResult;
nit: class -> struct.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
File chrome/renderer/spellchecker/spellcheck.h (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
chrome/renderer/spellchecker/spellcheck.h:23: class SpellCheckResult;
nit: class -> struct.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
chrome/renderer/spellchecker/spellcheck_provider.cc:45: void ToWebResultList(
nit: need 'namespace {' above this function to prevent these functions from
becoming global ones.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
chrome/renderer/spellchecker/spellcheck_provider.cc:65: }
nit: need '}  // namespace' here.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/renderer/spe...
chrome/renderer/spellchecker/spellcheck_provider.h:16: class SpellCheckResult;
nit: class -> struct.

[darin (slow to review), 2012-02-16 04:59:28]

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
File chrome/common/spellcheck_result.h (right):

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
chrome/common/spellcheck_result.h:20: struct SpellCheckResult {
Do we care that "spell check result" can express more than just spelling related
results?  Should we be using a more generic name, or have we decided to just
stick with "spellcheck" as the general name for this class of stuff (i.e., most
of the other files and classes just use the "spellcheck" name)?

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
chrome/common/spellcheck_result.h:22: Spelling = 1 << 1,
nit: use google c++ style.

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
chrome/common/spellcheck_result.h:49: Type type_;
nit: I recommend just eliminating the accessor functions above and rename these
member variables so that they do not have the trailing "_"

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
chrome/common/spellcheck_result.h:57: template <>
usually, IPC traits go into a separate header file with IPC traits for other
types.

[Hajime Morrita, 2012-02-16 06:17:58]

Bono-san, Darin, thanks for your quick feedback!
I updated the patch to address your points.

On the name of SpellCheckResult...

https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spell...
> chrome/common/spellcheck_result.h:20: struct SpellCheckResult {
> Do we care that "spell check result" can express more than just spelling
related
> results?  Should we be using a more generic name, or have we decided to just
> stick with "spellcheck" as the general name for this class of stuff (i.e.,
most
> of the other files and classes just use the "spellcheck" name)?

Because of its consistency. I prefer the name of SpellCheckResult even though it
could cover more general type of text checking: Our spelling component uses the
word "spelling" or "spellcheck" as a part of its class names. So following such
an unofficial convention makes it clear that the class is a part of that
component.

Regards,

[darin (slow to review), 2012-02-16 19:03:21]

On Wed, Feb 15, 2012 at 10:17 PM, <morrita@chromium.org> wrote:

> Bono-san, Darin, thanks for your quick feedback!
> I updated the patch to address your points.
>
> On the name of SpellCheckResult...
>
>
> https://chromiumcodereview.**appspot.com/9368052/diff/**
>
28002/chrome/common/**spellcheck_result.h#newcode20<https://chromiumcodereview.appspot.com/9368052/diff/28002/chrome/common/spellcheck_result.h#newcode20>
>
>> chrome/common/spellcheck_**result.h:20: struct SpellCheckResult {
>> Do we care that "spell check result" can express more than just spelling
>>
> related
>
>> results?  Should we be using a more generic name, or have we decided to
>> just
>> stick with "spellcheck" as the general name for this class of stuff (i.e.,
>>
> most
>
>> of the other files and classes just use the "spellcheck" name)?
>>
>
> Because of its consistency. I prefer the name of SpellCheckResult even
> though it
> could cover more general type of text checking: Our spelling component
> uses the
> word "spelling" or "spellcheck" as a part of its class names. So following
> such
> an unofficial convention makes it clear that the class is a part of that
> component.
>

OK, that's what I figured was going on.  Thanks!


>
> Regards,
>
>
>
https://chromiumcodereview.**appspot.com/9368052/<https://chromiumcodereview....
>

[Hajime Morrita, 2012-02-16 23:43:27]

> > Because of its consistency. I prefer the name of SpellCheckResult even
> > though it
> > could cover more general type of text checking: Our spelling component
> > uses the
> > word "spelling" or "spellcheck" as a part of its class names. So following
> > such
> > an unofficial convention makes it clear that the class is a part of that
> > component.
> >
> 
> OK, that's what I figured was going on.  Thanks!
> 
Darin, thanks for your understanding! And I think we need your lgtm for landing
this ;-)

[darin (slow to review), 2012-02-16 23:53:47]

LGTM

On Thu, Feb 16, 2012 at 3:43 PM, <morrita@chromium.org> wrote:

>
>  > Because of its consistency. I prefer the name of SpellCheckResult even
>> > though it
>> > could cover more general type of text checking: Our spelling component
>> > uses the
>> > word "spelling" or "spellcheck" as a part of its class names. So
>> following
>> > such
>> > an unofficial convention makes it clear that the class is a part of that
>> > component.
>> >
>>
>
>  OK, that's what I figured was going on.  Thanks!
>>
>
> Darin, thanks for your understanding! And I think we need your lgtm for
> landing
> this ;-)
>
>
>
>
https://chromiumcodereview.**appspot.com/9368052/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-02-17 00:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/9368052/36001

[commit-bot: I haz the power, 2012-02-17 02:14:55]

Change committed as 122429