The results of setTimeout, setInterval and navigator.geolocation.watchPosition are positive integer values extracted from a simple circular sequential number generator, whose uniqueness can be guaranteed for no more than 2^31 calls to any of these functions. In order to provide this guarantee beyond this limit, we repeatedly ask for the next sequential id until we get one that's not used already. With this solution we fix a FIXME note.
Created attachment 176014 [details] Patch
+Alexey I addressed a FIXME note which I had originally added at his request.
Comment on attachment 176014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176014&action=review > Source/WebCore/Modules/geolocation/Geolocation.cpp:183 > + bool result = m_idToNotifierMap.add(id, notifier.get()).isNewEntry; Won't this add() call overwrite an entry in m_idToNotifierMap on collision?
(In reply to comment #3) > Won't this add() call overwrite an entry in m_idToNotifierMap on collision? The original call (m_timeouts.set) used to. The new one (m_timeouts.add) does not. That is, essentially, the difference between HashMap::set and HashMap::add.
(In reply to comment #4) > The original call (m_timeouts.set) used to. The new one (m_timeouts.add) does not. Sorry, I meant to refer to m_idToNotifierMap. In fact, the comment above applies both to m_idToNotifierMap and m_timeouts.
> That is, essentially, the difference between HashMap::set and HashMap::add. Why does add() return "isNewEntry" if it doesn't overwrite?
(In reply to comment #6) > Why does add() return "isNewEntry" if it doesn't overwrite? Both HashMap::set and HashMap::add return an object of type HashTableType::AddResult, which is an (iterator, isNewEntry) tuple. The iterator member points to the newly-added or already-existing object (depending on what gets called); the isNewEntry member is true if a new object has just been set or added, and false in case of replacement (for set) or failure (for add). See the difference in the implementation of HashMap::set and HashMap::add. They both call HashMap::inlineAdd; but the difference is that HashMap::add does nothing on failure, while HashMap::set does the following: if (!result.isNewEntry) { // The inlineAdd call above found an existing hash table entry; we need to set the mapped value. MappedTraits::store(mapped, result.iterator->second); } In my patch, by replacing HashMap::set with HashMap::add, I no longer execute the above code, leaving the original key/value pair intact.
Created attachment 176838 [details] Patch After discussing offline with Alexey about various alternative names for the "result" variable, I eventually opted for "addSucceeded". I found out this is a common naming pattern in WebKit.
Comment on attachment 176838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176838&action=review review- because of the strange RefPtr local variable thing; otherwise looks OK > Source/WebCore/Modules/geolocation/Geolocation.cpp:186 > + bool addSucceeded = m_idToNotifierMap.add(id, notifier.get()).isNewEntry; > + if (addSucceeded) > + m_notifierToIdMap.set(notifier.release(), id); > + return addSucceeded; I would write it this way: if (!m_idToNotifierMap.add(id, notifier.get()).isNewEntry) return false; m_notifierToIdMap.set(notifier.release(), id); return true; > Source/WebCore/Modules/geolocation/Geolocation.cpp:321 > + RefPtr<GeoNotifier> passNotifier = notifier; > + success = m_watchers.add(watchID, passNotifier.release()); This should just be: success = m_watches.add(watchID, notifier); There is no need for the passNotifier local variable.
Created attachment 176889 [details] Patch (In reply to comment #9) Done; and I simplified it further, by removing the "success" variable. In addition, I made the "unlikely" comment accessible not only to the reader, but also to the compiler.
I rerun the tests with my patch on today's master, and they are still passing. May I please have a review?
+Darin
Comment on attachment 176889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176889&action=review > Source/WebCore/Modules/geolocation/Geolocation.cpp:318 > + } while (UNLIKELY(!m_watchers.add(watchID, notifier))); Please don't put this UNLIKELY here. It's not hot code, and there is no need to add visual noise to it.
Created attachment 188409 [details] Patch Thank you for the review. I removed UNLIKELY as you requested. Could you please cq+ the updated patch for me?
Comment on attachment 188409 [details] Patch Clearing flags on attachment: 188409 Committed r142909: <http://trac.webkit.org/changeset/142909>
All reviewed patches have been landed. Closing bug.
Mass move bugs into the DOM component.