Change WebDragData to use ChromiumDataObject interface methods rather than direct member access

This patch changes WebDragData to make use of the new ChromiumDataObjcet interface methods as described in http://code.google.com/p/chromium/issues/detail?id=24880 .

As detailed there, this is a prerequisite step for fixing https://bugs.webkit.org/show_bug.cgi?id=28293 (event.dataTransfer.get/setData) that requires the members to change. 
(I plan to make the members private once no Chromium code accesses them directly anymore).

BUG=24880
TEST=NONE (no functional change)

[Roland, 2009-10-15 10:59:35]

Rather simple change from direct access of members to the corresponding accessor
methods.

Note that the current try bot failure is in a unit test that AFAICT doesn't
touch the changed code.

[eseidel, 2009-10-15 15:01:46]

looks OK.  Several names strike me as strange.

"mainURL".  Why is it main?  What's a non-main url?

contentFileExtension, only applies when fileContent/content is set?
Does it not get used in the plainText or HTML or URL cases, I assume?

appendToFileNames, I probably would have called appendFileName, but
it's also fine as is.

I think "fileContent" was more clear than "content".  Almost makes me
want to split the whole content/filename/fileextension off into some
separarate struct that you set all at once.  Assuming that all 3
members are always used together like that.

Anyway, those are nits.  I trust your and darin's judgement for naming
over my own, since you all have worked at this layer much more than I.

LGTM.

On Thu, Oct 15, 2009 at 6:59 AM,  <rolandsteiner@google.com> wrote:
> Reviewers: Eric Seidel (Google), darin,
>
> Message:
> Rather simple change from direct access of members to the corresponding
> accessor
> methods.
>
> Note that the current try bot failure is in a unit test that AFAICT doesn=
't
> touch the changed code.
>
> Description:
> This patch changes WebDragData to make use of the new ChromiumDataObjcet
> interface methods as described in
> http://code.google.com/p/chromium/issues/detail?id=3D24880 .
>
> As detailed there, this is a prerequisite step for fixing
> https://bugs.webkit.org/show_bug.cgi?id=3D28293
> (event.dataTransfer.get/setData)
> that requires the members to change.
> (I plan to make the members private once no Chromium code accesses them
> directly
> anymore).
>
> BUG=3D24880
> TEST=3DNONE (no functional change)
>
>
>
> Please review this at http://codereview.chromium.org/269091
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 webkit/api/src/WebDragData.cpp
>
>
>

[darin (slow to review), 2009-10-15 16:10:00]

I don't really understand why the getter/setters were added to
ChromiumDataObject.  It was always intended to be a simple struct.  I see that
you already committed that change to WebKit.

The explanation given in bug 28964 is not very compelling.  Soon, all consumers
of ChromiumDataObject will live in svn.webkit.org, so the purpose of the methods
will soon go away.

I think this is just adding extra layers of indirection for little benefit,
which feels to me like it is just making things more complicated.

[Roland, 2009-10-16 03:31:45]

On 2009/10/15 15:01:46, Eric Seidel (Google) wrote:
> looks OK.  Several names strike me as strange.
> 
> "mainURL".  Why is it main?  What's a non-main url?

Most of the naming was a compromise to avoid naming clashes as mentioned in
https://bugs.webkit.org/show_bug.cgi?id=28964.

Specifically, I chose "mainURL" because the goal of the patches is to allow
setting of 'text/uri-list'. In such a case, getData('URL') is supposed to return
the first URL in that list. OTOH, most other places only set a single URL to
begin with.

On 2009/10/15 16:10:00, darin wrote:
> I don't really understand why the getter/setters were added to
> ChromiumDataObject.  It was always intended to be a simple struct.  I see that
> you already committed that change to WebKit.
> 
> The explanation given in bug 28964 is not very compelling.  Soon, all
consumers
> of ChromiumDataObject will live in http://svn.webkit.org, so the purpose of
the methods
> will soon go away.
> 
> I think this is just adding extra layers of indirection for little benefit,
> which feels to me like it is just making things more complicated.

I didn't know that - of course, if this isn't going to be a permanent interface
between WebKit and Chromium, then my changes lose most of their purpose.

Since this is blocking me from continuing with
https://bugs.webkit.org/show_bug.cgi?id=28293, Is there a way where I can help
speed up the process of this conversion?

[darin (slow to review), 2009-10-16 03:44:04]

On Thu, Oct 15, 2009 at 8:31 PM, <rolandsteiner@google.com> wrote:

> On 2009/10/15 15:01:46, Eric Seidel (Google) wrote:
>
>> looks OK.  Several names strike me as strange.
>>
>
>  "mainURL".  Why is it main?  What's a non-main url?
>>
>
> Most of the naming was a compromise to avoid naming clashes as mentioned in
> https://bugs.webkit.org/show_bug.cgi?id=28964.
>
> Specifically, I chose "mainURL" because the goal of the patches is to allow
> setting of 'text/uri-list'. In such a case, getData('URL') is supposed to
> return
> the first URL in that list. OTOH, most other places only set a single URL
> to
> begin with.
>
> On 2009/10/15 16:10:00, darin wrote:
>
>> I don't really understand why the getter/setters were added to
>> ChromiumDataObject.  It was always intended to be a simple struct.  I see
>> that
>> you already committed that change to WebKit.
>>
>
>  The explanation given in bug 28964 is not very compelling.  Soon, all
>>
> consumers
>
>> of ChromiumDataObject will live in http://svn.webkit.org, so the purpose
>> of
>>
> the methods
>
>> will soon go away.
>>
>
>  I think this is just adding extra layers of indirection for little
>> benefit,
>> which feels to me like it is just making things more complicated.
>>
>
> I didn't know that - of course, if this isn't going to be a permanent
> interface
> between WebKit and Chromium, then my changes lose most of their purpose.
>
>
Sorry to cause more busy work for you in the short term, but can you revert
the unnecessary bits from ChromiumDataObject?

-Darin



> Since this is blocking me from continuing with
> https://bugs.webkit.org/show_bug.cgi?id=28293, Is there a way where I can
> help
> speed up the process of this conversion?
>
>
>
> http://codereview.chromium.org/269091
>

[Roland, 2009-10-16 04:51:48]

Added https://bugs.webkit.org/show_bug.cgi?id=30427 and corresponding patch that
removes the methods again.


On 2009/10/16 03:44:04, darin wrote:
> Sorry to cause more busy work for you in the short term, but can you revert
> the unnecessary bits from ChromiumDataObject?

[darin (slow to review), 2009-10-16 04:59:31]

Thanks!

On Thu, Oct 15, 2009 at 9:51 PM, <rolandsteiner@google.com> wrote:

> Added https://bugs.webkit.org/show_bug.cgi?id=30427 and corresponding
> patch that
> removes the methods again.
>
>
> On 2009/10/16 03:44:04, darin wrote:
>
>> Sorry to cause more busy work for you in the short term, but can you
>> revert
>> the unnecessary bits from ChromiumDataObject?
>>
>
>
> http://codereview.chromium.org/269091
>