[webkit-reviews] review requested: [Bug 29178] Geolocation should be refactored to use a single map to store all notifiers : [Attachment 40716] Patch 2 for Bug 29178

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has asked  for review:
Bug 29178: Geolocation should be refactored to use a single map to store all
notifiers
https://bugs.webkit.org/show_bug.cgi?id=29178

Attachment 40716: Patch 2 for Bug 29178
https://bugs.webkit.org/attachment.cgi?id=40716&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
(In reply to comment #5)
> Why m_id instead of m_requestId or similar, as you seem to suggest it
> could/should be called in your changelog comments.
>
> "id" is a reserved word in Obj-C, so if this header was included by
> Objective-C code, that could be an issue.
Fixed.

(In reply to comment #6)
> I'll
> point out that the same thing can be done with unsigned integers just by
> choosing an arbitrary large number and having both sets of IDs go up. The
large
> number approach avoids having to write the comment about avoiding the magic
> number -1.
Yes, but it would complicate the logic in clearOneShots, which relies on the
fact that one-shot IDs are smaller than watch IDs.

> You also would need a function isValidWatchId to use instead of the
> "> 0" and "<= 0" checks, but this is actually an advantage rather than a
> disadvantage, since the code will be easier to read. A function like that
would
> be good even if we do stick with the negative numbers.
True, I've added isOneShotRequest().

> In both cases, the real issue is guaranteeing the number won't wrap around.
I'd
> like to see a comment explaining why overflow is known to not be an issue.
> Can't people intentionally do something in a tight loop and create the
> overflow?
The existing code doesn't guard against overflow and I wasn't attempting to
address it in this patch. Note that overflow is 'safe' in that when a request
ID is re-used, the previously existing request will be stopped, so requests are
not leaked. Of course, references to the old ID will now refer to the wrong
request, which should be fixed. I've opened Bug 30122 to track this, as finding
the right solution will probably require some discussion.

> Normally I like to name the global something like lastUsedWatchID or
> nextAvailableWatchID rather than watchID to make it more clear why it's a
> global variable.
Fixed.

> This is an unnecessary bit of reference count churn. I suggest writing it
like
Fixed.