[Webkit-unassigned] [Bug 36535] [chromium] Rename / tidy up Geolocation bridge

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36535





--- Comment #17 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-03-29 12:36:45 PST ---
(In reply to comment #13)
> General question: after an "r+", should I make the changes your suggested here
> or as a follow up? I'm happy to do either way, I'm just not yet familiar with
> the flow here.

Sorry, r+ means you can commit.  I am happy if you address the remaining
feedback now or later or not at all pending further discussion.


> > Please note that we generally provide default implementations for all
> > virtual WebKit API methods so that it is easier to deprecate methods.
> > I would recommend doing so for dettachBridge at least.
> 
> Good point. As above, should I do this now or as a follow up?
> Also: should I add default impl for all methods, not just the ones being
> deprecated / changed?

We generally add default implementations to all methods.  Note:  There are
some older interfaces where this wasn't the case.  The decision to add
default implementations happened mid-way through the API development.  It
just makes life easier when you wish to change the API.


> > It seems like it might be nice to have WEBKIT_IMPLEMENTATION methods
> > for converting to/from WebCore::Geoposition.
> > 
> > Also, since Geoposition is reference counted, the way we'd normally
> > expose it through the WebKit API is to leverage WebPrivatePtr, such
> > that WebGeoposition is really just a wrapper around a Geoposition
> > object.  Is there a reason why a struct as you have done is better?
> > 
> > Are you planning on serializing this struct over Chromium IPC?
> 
> Hmm, good point. On the embedder side, we have our own "Geoposition" datatype,
> which we need to pass to WebKit, then WebCore (it's one way only, there's no
> communication back to the embedder).
>
> This struct is then only used as you suggested, to avoid a long param list for
> "setLastPosition". If we make turn this into a class aggregating Geoposition
> via WebPrivatePtr, it'd require a ctor with the same long list of params, or a
> long list of setters (for each individual field), no?
> 
> If so, wouldn't it be more straightforward to just keep the long list of param
> on setLastPosition in the first place?
> 
> Please, let me know what's the preferred way.

I see.  The problem seems to stem from the Coordinates constructor, which
takes a big list of arguments.  I think that was probably a poor design
choice since it suffers from the problem that led me to encourage you to
break up setLastPosition.

If Coordinates instead had a bunch of setters, then it would be easy to
create a WebGeoposition object with a similar array of setters.  I guess
the downside to that approach is that someone might forget to call one of
the setters.

My preference would be to change Coordinates to have setters with each
property having a reasonable default value.  Then, make a WebGeoposition
object that wraps Geoposition and provides similar setters (and getters).

Of course, this seems like a lot of work for the benefit of a single method.
If you want to ditch WebGeoposition and make setLastPosition have a
bazillion parameters again, I won't object.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.