Bug 33467

Summary:

[Chromium] Move geolocationEnabled setting into an EnabledAtRuntime v8 setting

Product:

WebKit

Reporter:

Jonathan Dixon <joth>

Component:

WebCore Misc.

Assignee:

Jonathan Dixon <joth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, jorlow, oliver, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

32066

Bug Blocks:

Attachments:

Description	Flags
First patch	none
Patch 2 - using [EnabledAtRuntime] modifier	none
Patch 3 - missed a ChangeLog in previous one.	none

Jonathan Dixon

Reported 2010-01-11 06:46:11 PST

Bug https://bugs.webkit.org/show_bug.cgi?id=32066 added a new parameter, geolocationEnabled, to the Settings class. However nothing makes use of this -- somehow the change to Navigator.cpp to use it got missed out of the patch. Need to add code to conditionally (runtime) decide whether to create the Navigtor::m_geolocation object based on the above setting.

Attachments
First patch (1.59 KB, patch) 2010-01-11 06:53 PST, Jonathan Dixon	no flags	Details Formatted Diff Diff
Patch 2 - using [EnabledAtRuntime] modifier (7.51 KB, patch) 2010-01-15 07:31 PST, Jonathan Dixon	no flags	Details Formatted Diff Diff
Patch 3 - missed a ChangeLog in previous one. (7.66 KB, patch) 2010-01-15 07:41 PST, Jonathan Dixon	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jonathan Dixon

Comment 1 2010-01-11 06:53:37 PST

Created attachment 46272 [details] First patch

Darin Adler

Comment 2 2010-01-11 09:51:38 PST

Comment on attachment 46272 [details] First patch Note that this changes behavior if the first access to geolocation occurs when m_frame is 0 or m_frame is no longer attached to a page even if geolocation is enabled. Maybe that's OK; it could be strange edge case to check something like this in a frame after the window is closed (one case where m_frame is 0) or after the frame is removed from its parent frame (one case where m_frame->page() is 0). But it would be fairly straightforward to construct a test case that could see the difference.

Darin Adler

Comment 3 2010-01-11 09:52:22 PST

In the past we have instead made the object in question behave differently based on the setting rather than using 0 instead of the object.

Jonathan Dixon

Comment 4 2010-01-15 05:11:33 PST

(In reply to comment #3) > In the past we have instead made the object in question behave differently > based on the setting rather than using 0 instead of the object. Thanks for the comments Darin: digging into them, Jeremy pointed me to a much better way to do what I wanted using the [EnabledAtRuntime] flag in the IDL file to control this, which will address both your comments. Just creating a patch now to do this.

Jonathan Dixon

Comment 5 2010-01-15 07:31:30 PST

Created attachment 46680 [details] Patch 2 - using [EnabledAtRuntime] modifier

Jonathan Dixon

Comment 6 2010-01-15 07:41:49 PST

Created attachment 46681 [details] Patch 3 - missed a ChangeLog in previous one.

Jeremy Orlow

Comment 7 2010-01-15 11:15:37 PST

LGTM, but I'm not a reviewer.

Adam Barth

Comment 8 2010-01-15 18:58:27 PST

Comment on attachment 46681 [details] Patch 3 - missed a ChangeLog in previous one. LGTM. I wish this code were auto-generated from the IDL attribute.

WebKit Commit Bot

Comment 9 2010-01-18 06:37:07 PST

Comment on attachment 46681 [details] Patch 3 - missed a ChangeLog in previous one. Clearing flags on attachment: 46681 Committed r53406: <http://trac.webkit.org/changeset/53406>

WebKit Commit Bot

Comment 10 2010-01-18 06:37:13 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.