Chromium Code Reviews
[email protected] (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(558)

Issue 3799010: Geolocation dispatcher rename: (Closed)

Created:
10 years, 2 months ago by John Knottenbelt
Modified:
9 years, 7 months ago
Reviewers:
joth, bulach
CC:
chromium-reviews, brettw-cc_chromium.org, darin-cc_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Geolocation dispatcher rename: Rename GeolocationDispatcherHost to GeolocationDispatcherHostOld and GeolocationDispatcher to GeolocationDispatcherOld in preparation for the new client-based implementation. BUG=59908 TEST=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63507

Patch Set 1 #

Total comments: 8

Patch Set 2 : Incorporate Marcus's suggestions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -77 lines) Patch
A + chrome/browser/geolocation/geolocation_dispatcher_host_old.h View 1 3 chunks +14 lines, -11 lines 0 comments Download
A + chrome/browser/geolocation/geolocation_dispatcher_host_old.cc View 1 14 chunks +31 lines, -28 lines 0 comments Download
M chrome/browser/geolocation/geolocation_permission_context.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/geolocation/geolocation_permission_context.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/chrome_renderer.gypi View 1 chunk +2 lines, -2 lines 0 comments Download
A + chrome/renderer/geolocation_dispatcher_old.h View 1 3 chunks +13 lines, -9 lines 0 comments Download
A + chrome/renderer/geolocation_dispatcher_old.cc View 5 chunks +15 lines, -15 lines 0 comments Download
M chrome/renderer/render_view.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/renderer/render_view.cc View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
bulach
LGTM overall looks fine, a few nits below and one suggestion: Since this is a ...
10 years, 2 months ago (2010-10-19 19:03:48 UTC) #1
John Knottenbelt
10 years, 2 months ago (2010-10-20 09:49:11 UTC) #2
http://codereview.chromium.org/3799010/diff/1/2
File chrome/browser/geolocation/geolocation_dispatcher_host_old.cc (right):

http://codereview.chromium.org/3799010/diff/1/2#newcode131
chrome/browser/geolocation/geolocation_dispatcher_host_old.cc:131: void
GeolocationDispatcherHostOldImpl::OnRegisterDispatcher(int render_view_id) {
On 2010/10/19 19:03:49, bulach wrote:
> >80cols

Done.

http://codereview.chromium.org/3799010/diff/1/2#newcode137
chrome/browser/geolocation/geolocation_dispatcher_host_old.cc:137: void
GeolocationDispatcherHostOldImpl::OnUnregisterDispatcher(int render_view_id) {
On 2010/10/19 19:03:49, bulach wrote:
> >80cols

Done.

http://codereview.chromium.org/3799010/diff/1/2#newcode228
chrome/browser/geolocation/geolocation_dispatcher_host_old.cc:228: return new
GeolocationDispatcherHostOldImpl(resource_message_filter_process_id,
On 2010/10/19 19:03:49, bulach wrote:
> >80cols

Done.

http://codereview.chromium.org/3799010/diff/1/11
File chrome/renderer/geolocation_dispatcher_old.h (right):

http://codereview.chromium.org/3799010/diff/1/11#newcode21
chrome/renderer/geolocation_dispatcher_old.h:21: // It's the complement of
GeolocationDispatcherOldHost (owned by RenderViewHost).
On 2010/10/19 19:03:49, bulach wrote:
> >80cols.
> 
> also, it'd be great to add the same TODO as the host:
> 
> // TODO(jknotten): Remove this class once the new client-based implementation
is
> // checked in (see https://bugs.webkit.org/show_bug.cgi?id=45752).

Done.

Powered by Google App Engine
This is Rietveld 408576698