Bug 28964 – [Chromium] ChromiumDataObject should have getter/setter interface

• Component: Platform
• Status: RESOLVED FIXED

[Roland Steiner]

The class ChromiumDataObject currently exposes its members directly. As this class is accessed by Chromium glue code, this is inconvenient if one wants to modify its members. Therefore it should only expose suitable accessor methods rather than the raw data members.

[Roland Steiner]

Created attachment 39047 [details]
patch: add getter/setter accessor methods


Added mostly trivial getter & setter accessor methods for the existing data members.

Once the Chromium code is changed to use these instead of directly accessing the member variables, they should be made private.

[David Levin]

Comment on attachment 39047 [details]
patch: add getter/setter accessor methods

r- for the PassRefPtr return value


> Index: WebCore/ChangeLog
> ===================================================================

> + Added interface methods.

Really you are adding methods to a class not to an interface. (You have implementations here.)

> +
> +        No new tests.

Usually you would explain why there are no new tests.  For example,

  "No functional behavior change so no new tests."


> Index: WebCore/platform/chromium/ChromiumDataObject.h
> +        inline PassRefPtr<SharedBuffer> content() const { return fileContent; }

"If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions." -- http://webkit.org/coding/RefPtr.html

>          KURL url;
>          String urlTitle;

Later: Please consider renaming the variables to m_ when you make them private.

[Roland Steiner]

Created attachment 39135 [details]
patch: add getter/setter accessor methods

Made the requested changes:

- content() method returns SharedBuffer*
- added new method PassRefPtr<SharedBuffer> releaseContent(), as it seems that it'd be useful to have
- clarified the ChangeLog

Renaming the variables after they become private is already on the agenda. :)

On a further note: since I couldn't yet rename the member variables, and the compiler complained if a getter method had the same name as a variable, I had to be a bit creative when coming up with names. If you have other/better suggestions, please let me know!

[Eric Seidel (no email)]

Comment on attachment 39135 [details]
patch: add getter/setter accessor methods

These don't need to be marked inline.  Compilers already inline such functions.  If they didn't we'd see duplicated symbol definitions all over the place. :)

I take it that the "chromium code" in question lives outside of the WebKit tree?  So that we have to do this in two-stages like this?

[Roland Steiner]

Created attachment 39267 [details]
patch: add getter/setter accessor methods


Removed 'inline' from methods.

> I take it that the "chromium code" in question lives outside of the WebKit
> tree?  So that we have to do this in two-stages like this?

Yes, the code in question is (Chromium) webkit\api\src\WebDragData.cpp .

The main reason for the change is that I want to implement get/setData("text/uri-list") in Chromium.
(see https://bugs.webkit.org/show_bug.cgi?id=28293)
However, according to the HTML5 spec, "text/uri-list" and "URL" are not independent.
So having WebDragData accessing a single 'url' member directly is inconvenient.

The getter/setter methods for the other members is just "collateral damage" ;)
(but it also allows cleaning up the naming).

[Eric Seidel (no email)]

Comment on attachment 39267 [details]
patch: add getter/setter accessor methods

Looks fine to me.

[Eric Seidel (no email)]

Comment on attachment 39267 [details]
patch: add getter/setter accessor methods

Clearing flags on attachment: 39267

Committed r48208: <http://trac.webkit.org/changeset/48208>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Roland Steiner]

Created attachment 40061 [details]
patch - extend getter/setter interface

(Sorry for adding to this after the patch has already landed.)

During further implementation with the interface in the original patch I found that it is, while technically sufficient, perhaps a tad too simplistic, esp. when considering that the interface should stay largely unchanged even if the underlying data members change. In detail:

- The methods for file names are too reliant on the data member actually being a Vector<String>.
  Manipulation of the list of filenames requires wholesale copying or using a swapping trick, neither of which is very elegant.
- Several places of the code are only interested in whether a given member exists, not what its actual value is.
  In these cases it is inefficient to have the code create a duplicate that is soon discarded.

Therefore I added a new patch that extends the interface somewhat to try to make it more useful and robust to further changes (*cough*). From the ChangeLog:

- added contains...() methods to just query the state
- added containsValid...URL() methods for URL data members
- removed takeFileNames() as this was too type-dependent
- changed return type of fileNames() to Vector<String>
- added interface methods to allow appending to and iteration over file names

[Roland Steiner]

(Not sure if reopening the bug is the right thing to do here.)

As noted in the comments to my new patch I found that it'd be useful to somewhat extend the interface with a few more methods that make it more convenient and efficient. It also aims to remove the last cases where the interface was relying on a specific underlying type.

[Eric Seidel (no email)]

Comment on attachment 40061 [details]
patch - extend getter/setter interface

Patch was uploaded with the wrong mime type and w/o the patch checkbox set.  You might find "bugzilla-tool post-diff" a useful alternative to posting patches by hand.  It will take care of all those details.

[Eric Seidel (no email)]

Comment on attachment 40061 [details]
patch - extend getter/setter interface

Do you want !isEmpty() or !isNull()?

I have no idea what this DataObject does, so I can't really review this.

[Adam Barth]

Comment on attachment 40061 [details]
patch - extend getter/setter interface

It's hard for me to evaluate whether we need this patch because the client for this interface does not appear to exist in the repo, but this patch appears formally correct.

[WebKit Commit Bot]

Comment on attachment 40061 [details]
patch - extend getter/setter interface

Clearing flags on attachment: 40061

Committed r49524: <http://trac.webkit.org/changeset/49524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

> The main reason for the change is that I want to implement
> get/setData("text/uri-list") in Chromium.
> (see https://bugs.webkit.org/show_bug.cgi?id=28293)
> However, according to the HTML5 spec, "text/uri-list" and "URL" are not
> independent.
> So having WebDragData accessing a single 'url' member directly is inconvenient.

It seems to me that this ^^^ is the only method that should have been added.

I intended ChromiumDataObject to remain very lightweight since we have it replicated in two places already in Chrome: WebDragData and WebDropData.  It sucks to have so many layers.

Soon, WebDragData will live in svn.webkit.org, so the justification given in comment #0 will go away.  It'll be easy to change the WebKit layer implementation (WebDragData.cpp) and the ChromiumDataObject.h in the same patch.

[Roland Steiner]

(In reply to comment #16)
> > The main reason for the change is that I want to implement
> > get/setData("text/uri-list") in Chromium.
> > (see https://bugs.webkit.org/show_bug.cgi?id=28293)
> > However, according to the HTML5 spec, "text/uri-list" and "URL" are not
> > independent.
> > So having WebDragData accessing a single 'url' member directly is inconvenient.
> 
> It seems to me that this ^^^ is the only method that should have been added.

Yes, I added the other interface methods mainly for consistency.
 
> I intended ChromiumDataObject to remain very lightweight since we have it
> replicated in two places already in Chrome: WebDragData and WebDropData.  It
> sucks to have so many layers.

There I wholeheartedly agree! 
 
> Soon, WebDragData will live in svn.webkit.org, so the justification given in
> comment #0 will go away.  It'll be easy to change the WebKit layer
> implementation (WebDragData.cpp) and the ChromiumDataObject.h in the same
> patch.

As I wrote http://codereview.chromium.org/269091, I didn't know that was the plan. It'd certainly simplify things.