[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 #19 from Marcus Bulach <bulach at chromium.org>  2010-03-30 03:40:27 PST ---
Thanks Darin!
Replies inline, would you mind a final look please?

(In reply to comment #17)
> (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.

Ahn, ok, thanks for the clarification!
I went back one step and removed the struct, is now back to multiple params.
More details below.

> 
> 
> > > 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.

Got it, I probably based on some older interface when implemented this, thanks
for the heads up.

> 
> 
> > > 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.

I reverted back to multiple params so that we can get the rest of the change in
place.

I'm happy to follow up on this, but I'd appreciate your help clarifying the
following:
There's a Coordinates.idl which marks all fields as read-only, afaict that's
what's exposed to javascript.
Is there any constraint in diverging the .idl and the .h? That is, adding the
list of setters (rather than passing all params at ctor / factory time), might
introduce some confusion.
If there's no such constraint, I'll add the setters and a factory method with
no params as a separate change.
Thoughts?

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