First implementation of PlainTextController.

First implementation of PlainTextController.

Add a plainText object to the JS window object similarly to
layoutTestController or eventSender.

This object currently has a single function 'plainText' that
takes a range and returns the plain text contained within.

The implementation also adds a few methods that should be generally useful.


BUG=23285
TEST=LayoutTests/editing/text-iterator/basic-iteration.html

[Roland, 2009-09-29 10:22:53]

Hi all,

In the crusade to tackle LTTF bugs I uploaded a first attempt at implementing a
PlainTextController. It's probably not full finished yet - please see the
discussion with the patch and the original bug issue.

Cheers,

Roland

[M-A Ruel, 2009-09-29 11:21:27]

I don't know why you are putting me on this review as I don't know this code. A
few style nit.

http://codereview.chromium.org/243032/diff/1/2
File webkit/glue/cpp_variant.cc (right):

http://codereview.chromium.org/243032/diff/1/2#newcode269
Line 269: bool CppVariant::GetProperty(const std::string& property, CppVariant&
result) const {
80 cols (bis)

http://codereview.chromium.org/243032/diff/1/3
File webkit/glue/cpp_variant.h (right):

http://codereview.chromium.org/243032/diff/1/3#newcode109
Line 109: // Retrieve the value of a property. Returns whether the property was
Retrieves

http://codereview.chromium.org/243032/diff/1/5
File webkit/tools/test_shell/plain_text_controller.h (right):

http://codereview.chromium.org/243032/diff/1/5#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009

[Roland, 2009-09-29 12:42:29]

On 2009/09/29 11:21:27, Marc-Antoine Ruel wrote:
> I don't know why you are putting me on this review as I don't know this code.
A
> few style nit.

Oops, sorry about that, I thought I saw your name in 'git blame'.

Anyway, thanks for the remarks, I uploaded a corrected patch set.

[brettw, 2009-09-29 17:19:13]

http://codereview.chromium.org/243032/diff/4001/4003
File webkit/glue/cpp_variant.h (right):

http://codereview.chromium.org/243032/diff/4001/4003#newcode112
Line 112: bool GetProperty(const std::string& property, CppVariant& result)
const;
Output args should be pointers. The previous function is wrong, but we don't
need this to be wrong as well.

[darin (slow to review), 2009-09-30 05:10:13]

http://codereview.chromium.org/243032/diff/4001/4004
File webkit/tools/test_shell/plain_text_controller.cc (right):

http://codereview.chromium.org/243032/diff/4001/4004#newcode16
Line 16: if (NULL == shell_)
nit: if (!shell_)

http://codereview.chromium.org/243032/diff/4001/4004#newcode43
Line 43: // Range's text() method, which is not contained in the IDL.
we can add a text method to WebKit::WebRange.

then we just need a way to map args[0] to a WebRange.  i'm not sure how best to
do that.  perhaps we can have a static method on WebRange that takes a NPObject.

  class WebRange {
  ...
      WEBKIT_API static WebRange fromObject(NPObject*);
  ...
      WEBKIT_API WebString text() const;
  ...
  };

http://codereview.chromium.org/243032/diff/4001/4005
File webkit/tools/test_shell/plain_text_controller.h (right):

http://codereview.chromium.org/243032/diff/4001/4005#newcode6
Line 6: /*
nit: please use C++ style comments

http://codereview.chromium.org/243032/diff/4001/4005#newcode37
Line 37: private:
nit: please indent.

looks like you should run this CL through lint and fix the style errors.

[Roland, 2009-09-30 11:24:32]

On 2009/09/30 05:10:13, darin wrote:
> looks like you should run this CL through lint and fix the style errors.

...I guess the "bubut I copied and adapted the code from somewhere else"-excuse
won't catch. ^_^;

Uploaded a new cleaned-up patch set.

> http://codereview.chromium.org/243032/diff/4001/4004#newcode43
> Line 43: // Range's text() method, which is not contained in the IDL.
> we can add a text method to WebKit::WebRange.
> 
> then we just need a way to map args[0] to a WebRange.  i'm not sure how best
to
> do that.  perhaps we can have a static method on WebRange that takes a
NPObject.
> 
>   class WebRange {
>   ...
>       WEBKIT_API static WebRange fromObject(NPObject*);
>   ...
>       WEBKIT_API WebString text() const;
>   ...
>   };

However, how do we know that the passed-in NPObject is indeed a Range? I haven't
found a reliable way to detect this (then again, I'm far from an expert in these
parts of the code).

[Roland, 2009-10-13 11:58:46]

RFC!... ^_-

Cheers,

Roland


On 2009/09/30 11:24:32, Roland wrote:
> On 2009/09/30 05:10:13, darin wrote:
> > looks like you should run this CL through lint and fix the style errors.
> 
> ...I guess the "bubut I copied and adapted the code from somewhere
else"-excuse
> won't catch. ^_^;
> 
> Uploaded a new cleaned-up patch set.
> 
> > http://codereview.chromium.org/243032/diff/4001/4004#newcode43
> > Line 43: // Range's text() method, which is not contained in the IDL.
> > we can add a text method to WebKit::WebRange.
> > 
> > then we just need a way to map args[0] to a WebRange.  i'm not sure how best
> to
> > do that.  perhaps we can have a static method on WebRange that takes a
> NPObject.
> > 
> >   class WebRange {
> >   ...
> >       WEBKIT_API static WebRange fromObject(NPObject*);
> >   ...
> >       WEBKIT_API WebString text() const;
> >   ...
> >   };
> 
> However, how do we know that the passed-in NPObject is indeed a Range? I
haven't
> found a reliable way to detect this (then again, I'm far from an expert in
these
> parts of the code).

[Roland, 2009-10-13 14:35:36]

Addendum about testing that a NPObject is indeed a range: what seems to work is
to query for a specific method to ranges (I use "setStartBefore" for the time
being). I don't know, however, if this is an acceptable "solution" for the
purpose of this implementation.

I also still need to find the proper way to convert an NPObject* to a WebRange.
I will upload a new version once this is implemented (hints and suggestions are
of course welcome! ^_^;)

[Roland Steiner, 2009-11-04 12:23:55]

After a great starting point tip from dglazkov, I uploaded a (hopefully)
finalized version of PlainTextController.

'gcl upload' was also so helpful to create a new issue for me before I could
stop it: http://codereview.chromium.org/360006. If someone could delete that one
for me, please... :p

[darin (slow to review), 2009-11-04 17:31:15]

http://codereview.chromium.org/243032/diff/14001/14005
File webkit/api/public/WebBindings.h (right):

http://codereview.chromium.org/243032/diff/14001/14005#newcode131
Line 131: WEBKIT_API static bool getDragData(NPObject* event, int* eventId,
WebDragData* data);
please do not name parameters when the name does not provide additional
information.  this is a webkit style rule.

http://codereview.chromium.org/243032/diff/14001/14005#newcode140
Line 140: WEBKIT_API static bool getRange(NPObject* npobj, WebRange* range);
please change to:

WEBKIT_API static bool getRange(NPObject* range, WebRange*);

the parameter name "range" applies to the NPObject to indicate that the given
NPObject should correspond to a range.

http://codereview.chromium.org/243032/diff/14001/14004
File webkit/api/public/WebRange.h (right):

http://codereview.chromium.org/243032/diff/14001/14004#newcode65
Line 65: // extension functions
nit: i would leave out this comment since it doesn't really add information.

for consistency with the rest of the webkit API, can you please rename these
methods to:

  toHTMLText()
  toPlainText()

http://codereview.chromium.org/243032/diff/14001/14002
File webkit/api/src/WebBindings.cpp (right):

http://codereview.chromium.org/243032/diff/14001/14002#newcode281
Line 281: V8NPObject* v8npobject = reinterpret_cast<V8NPObject*>(npobj);
it is too bad that V8NPObject does not inherit from NPObject.  if it did, you
could use static_cast here instead!

http://codereview.chromium.org/243032/diff/14001/14008
File webkit/tools/test_shell/plain_text_controller.cc (right):

http://codereview.chromium.org/243032/diff/14001/14008#newcode14
Line 14: using namespace WebKit;
google style forbids 'using namespace'.  please just do:

  using WebKit::WebBindings;
  using WebKit::WebRange;
  using WebKit::WebString;

etc.

http://codereview.chromium.org/243032/diff/14001/14010
File webkit/tools/test_shell/test_shell.h (right):

http://codereview.chromium.org/243032/diff/14001/14010#newcode289
Line 289: protected:
this file is already abusing google style, but for consistency, i think you
should leave this unindented.

[Roland Steiner, 2009-11-05 05:35:21]

Uploaded a new patch set that addresses the requested code changes.

[dglazkov, 2009-11-05 21:29:45]

LGTM w/nits:

http://codereview.chromium.org/243032/diff/15014/14023
File webkit/api/public/WebRange.h (right):

http://codereview.chromium.org/243032/diff/15014/14023#newcode65
Line 65: // non-JS methods
No need for that comment here.

http://codereview.chromium.org/243032/diff/15014/14026
File webkit/glue/cpp_variant.cc (right):

http://codereview.chromium.org/243032/diff/15014/14026#newcode271
Line 271: CppVariant* result) const {
Same here.

http://codereview.chromium.org/243032/diff/15014/14025
File webkit/glue/cpp_variant.h (right):

http://codereview.chromium.org/243032/diff/15014/14025#newcode106
Line 106: 
No need to touch this file anymore, right? You can just remove it from this
patch.

[Roland Steiner, 2009-11-06 04:00:08]

On 2009/11/05 21:29:45, Dimitri Glazkov wrote:
> LGTM w/nits:
> 
> http://codereview.chromium.org/243032/diff/15014/14025#newcode106
> Line 106: 
> No need to touch this file anymore, right? You can just remove it from this
> patch.

Yes - I thought that that was an obviously missing functionality, but perhaps
it's better to keep it for another CL.

Uploaded a new patch set with the requested changes for try bots and posterity.