Bug 78860 - Frame and Navigator shouldn't need to worry about Geolocation
: Frame and Navigator shouldn't need to worry about Geolocation
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 79327
  Show dependency treegraph
 
Reported: 2012-02-16 17:08 PST by
Modified: 2012-02-23 20:58 PST (History)


Attachments
work in progress (14.62 KB, patch)
2012-02-16 17:08 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.76 KB, patch)
2012-02-17 14:12 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (41.56 KB, patch)
2012-02-17 15:04 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (41.49 KB, patch)
2012-02-21 11:28 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
should fix qt (41.49 KB, patch)
2012-02-21 13:04 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
for EWSWS (59.08 KB, patch)
2012-02-21 13:20 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (86.61 KB, patch)
2012-02-21 14:39 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (59.40 KB, patch)
2012-02-21 14:40 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (59.98 KB, patch)
2012-02-21 15:52 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (60.01 KB, patch)
2012-02-21 16:59 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-02-16 17:08:22 PST
Frame and Navigator shouldn't need to worry about Geolocation
------- Comment #1 From 2012-02-16 17:08:40 PST -------
Created an attachment (id=127475) [details]
work in progress
------- Comment #2 From 2012-02-16 17:17:27 PST -------
(From update of attachment 127475 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127475&action=review

> Source/WebCore/page/Frame.cpp:-724
> -        // FIXME: We should ideally allow existing Geolocation activities to continue
> -        // when the Geolocation's iframe is reparented.
> -        // See https://bugs.webkit.org/show_bug.cgi?id=55577
> -        // and https://bugs.webkit.org/show_bug.cgi?id=52877

Do you need to move these fixmes?
------- Comment #3 From 2012-02-17 10:43:59 PST -------
I'm going to move them into NavigatorGeolocation::pageDestroyed.

I believe the plan is to remove the notion of reparenting iframes.  It doesn't really work anyway.
------- Comment #4 From 2012-02-17 14:12:00 PST -------
Created an attachment (id=127657) [details]
Patch
------- Comment #5 From 2012-02-17 14:22:41 PST -------
(From update of attachment 127657 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127657&action=review

> Source/WebCore/ChangeLog:18
> +        * Modules/geolocation: Added.

It's not immediately clear to me what "modules" are in WebKit.  What features are modules?  It seems modules have less to do with feature defines, and more to do with separability from the core code?

> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:49
> +    // FIXME: We should ideally allow existing Geolocation activities to continue
> +    // when the Geolocation's iframe is reparented. (Assuming we continue to
> +    // support reparenting iframes.)
> +    // See https://bugs.webkit.org/show_bug.cgi?id=55577
> +    // and https://bugs.webkit.org/show_bug.cgi?id=52877

Doing so would require a pageChanged or some other notification to know that you have a new page, no?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:7390
> -		1C435CD314E8544F004E10EA /* Inspector.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = Inspector.json; sourceTree = "<group>"; };
> -		1C435CD414E8545B004E10EA /* Inspector-0.1.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "Inspector-0.1.json"; sourceTree = "<group>"; };
> -		1C435CD514E8545B004E10EA /* Inspector-1.0.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "Inspector-1.0.json"; sourceTree = "<group>"; };
> +		1C435CD314E8544F004E10EA /* Inspector.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = Inspector.json; sourceTree = "<group>"; };
> +		1C435CD414E8545B004E10EA /* Inspector-0.1.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Inspector-0.1.json"; sourceTree = "<group>"; };
> +		1C435CD514E8545B004E10EA /* Inspector-1.0.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Inspector-1.0.json"; sourceTree = "<group>"; };

This seems to be an Xcode version war?

> Source/WebCore/page/DOMWindow.cpp:493
> +    HashSet<DOMWindowProperty*>::iterator stop = m_properties.end();

So DOMWindowProperties are always FrameDestructionObservers?

> Source/WebCore/page/Frame.cpp:730
> +            (*it)->pageDestroyed();

It's not immediately clear to me why we call pageDestroyed here.  Maybe pageDestroyed is just poorly named?  Will this FrameDestructionObserver ever get a new page?
------- Comment #6 From 2012-02-17 14:32:10 PST -------
(In reply to comment #5)
> (From update of attachment 127657 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127657&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        * Modules/geolocation: Added.
> 
> It's not immediately clear to me what "modules" are in WebKit.  What features are modules?  It seems modules have less to do with feature defines, and more to do with separability from the core code?

Correct.  Code outside of a given module doesn't depend on anything inside the module.  Modules do not depend on each other.

The idea is that they roughly correspond to features / specifications that aren't core to the rendering engine (like a CSS feature might be).  Usually module will be introduced with a corresponding ENABLE flag.  The dependency rules ensure that nothing outside the module will be burdened with knowledge/complexity of the ENABLE flag.

> > Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:49
> > +    // FIXME: We should ideally allow existing Geolocation activities to continue
> > +    // when the Geolocation's iframe is reparented. (Assuming we continue to
> > +    // support reparenting iframes.)
> > +    // See https://bugs.webkit.org/show_bug.cgi?id=55577
> > +    // and https://bugs.webkit.org/show_bug.cgi?id=52877
> 
> Doing so would require a pageChanged or some other notification to know that you have a new page, no?

Correct.  However, it's more likely will we simply remove the functionality.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:7390
> > -		1C435CD314E8544F004E10EA /* Inspector.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = Inspector.json; sourceTree = "<group>"; };
> > -		1C435CD414E8545B004E10EA /* Inspector-0.1.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "Inspector-0.1.json"; sourceTree = "<group>"; };
> > -		1C435CD514E8545B004E10EA /* Inspector-1.0.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "Inspector-1.0.json"; sourceTree = "<group>"; };
> > +		1C435CD314E8544F004E10EA /* Inspector.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = Inspector.json; sourceTree = "<group>"; };
> > +		1C435CD414E8545B004E10EA /* Inspector-0.1.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Inspector-0.1.json"; sourceTree = "<group>"; };
> > +		1C435CD514E8545B004E10EA /* Inspector-1.0.json */ = {isa = PBXFileReference; lastKnownFileType = text; path = "Inspector-1.0.json"; sourceTree = "<group>"; };
> 
> This seems to be an Xcode version war?

Not sure.  I'll remove these changes before landing.

> > Source/WebCore/page/DOMWindow.cpp:493
> > +    HashSet<DOMWindowProperty*>::iterator stop = m_properties.end();
> 
> So DOMWindowProperties are always FrameDestructionObservers?

It's not, although they both can observe pageDestroyed().  It's likely they'll merge in the future, but at the moment, DOMWindow forwards the pageDestroyed() call to its properties.

Note: I plan to rename FrameDestructionObserver to FrameObservers if it needs any more callbacks.

> > Source/WebCore/page/Frame.cpp:730
> > +            (*it)->pageDestroyed();
> 
> It's not immediately clear to me why we call pageDestroyed here.  Maybe pageDestroyed is just poorly named?  Will this FrameDestructionObserver ever get a new page?

We can call it disconnectPage() or willDisconnectPage() if you'd prefer.  The idea is that the association with the Page is going away, either because the Page is about to be destroyed or because we're moving to a new Page.
------- Comment #7 From 2012-02-17 14:53:12 PST -------
> We can call it disconnectPage() or willDisconnectPage() if you'd prefer.  The idea is that the association with the Page is going away, either because the Page is about to be destroyed or because we're moving to a new Page.

I've renamed it to willDetachPage, a name inspired by Frame::detachFromPage.
------- Comment #8 From 2012-02-17 15:04:49 PST -------
Created an attachment (id=127662) [details]
Patch
------- Comment #9 From 2012-02-17 15:11:16 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> Correct.  Code outside of a given module doesn't depend on anything inside the module.  Modules do not depend on each other.
> 
> The idea is that they roughly correspond to features / specifications that aren't core to the rendering engine (like a CSS feature might be).  Usually module will be introduced with a corresponding ENABLE flag.  The dependency rules ensure that nothing outside the module will be burdened with knowledge/complexity of the ENABLE flag.

I know WebKit hasn't done this in the past, but I think we might consider a README in modules to explain this design pattern.  Maybe we have a wiki on this?  In any case, "modules" itself does not convey enough information to make this design point clear. :)
------- Comment #10 From 2012-02-17 15:14:11 PST -------
(From update of attachment 127662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127662&action=review

> Source/WebCore/page/Page.cpp:207
> +        frame->willDetachPage();
> +        frame->detachFromPage();

Makes one wonder if detachFromPage should call willDetachPage()...
------- Comment #11 From 2012-02-17 15:14:52 PST -------
> I know WebKit hasn't done this in the past, but I think we might consider a README in modules to explain this design pattern.  Maybe we have a wiki on this?  In any case, "modules" itself does not convey enough information to make this design point clear. :)

Adding a README file is a good idea.  I believe Kentaro is working on a Wiki page that describes [Supplemental] and the other IDL attributes.  We should probably add a section on modules as well.
------- Comment #12 From 2012-02-17 15:15:40 PST -------
(In reply to comment #10)
> (From update of attachment 127662 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127662&action=review
> 
> > Source/WebCore/page/Page.cpp:207
> > +        frame->willDetachPage();
> > +        frame->detachFromPage();
> 
> Makes one wonder if detachFromPage should call willDetachPage()...

Yeah, that's a good idea.  I believe it's an error to call detachFromPage without calling willDetachPage, so we should make that error impossible.
------- Comment #13 From 2012-02-17 15:50:30 PST -------
(From update of attachment 127662 [details])
Attachment 127662 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11535706
------- Comment #14 From 2012-02-17 15:51:37 PST -------
View in context: https://bugs.webkit.org/attachment.cgi?id=127662&action=review

> Source/WebCore/Target.pri:37
> +    Modules/Geolocation/NavigatorGeolocation.cpp \

Geolocation => geolocation

> Source/WebCore/Target.pri:38
> +

Also we need a \ on this line.
------- Comment #15 From 2012-02-17 15:53:33 PST -------
From the GTK bot:

DerivedSources/webkit/WebKitDOMNavigator.cpp: In function 'WebKitDOMGeolocation* webkit_dom_navigator_get_geolocation(WebKitDOMNavigator*)':
DerivedSources/webkit/WebKitDOMNavigator.cpp:230:58: error: 'NavigatorGeolocation' has not been declared
DerivedSources/webkit/WebKitDOMNavigator.cpp: In function 'void webkit_dom_navigator_get_property(GObject*, guint, GValue*, GParamSpec*)':
DerivedSources/webkit/WebKitDOMNavigator.cpp:384:54: error: 'class WebCore::Navigator' has no member named 'geolocation'

Looks like we need to teach another code generator about [Supplemental].
------- Comment #16 From 2012-02-17 15:57:27 PST -------
(From update of attachment 127662 [details])
Attachment 127662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11543299
------- Comment #17 From 2012-02-17 17:12:51 PST -------
(In reply to comment #9)
> I know WebKit hasn't done this in the past, but I think we might consider a README in modules to explain this design pattern.  Maybe we have a wiki on this?  In any case, "modules" itself does not convey enough information to make this design point clear. :)

Eric: I already have a wiki and a SpreadSheet of modularization plan and am planning to announce it to webkit-dev@. But before that, I need to fix GTK build regression caused by [Supplemental] to prevent people from complaining about [Supplemental]. After that fix, I'll share it.
------- Comment #18 From 2012-02-17 17:16:24 PST -------
(In reply to comment #15)
> From the GTK bot:
> 
> Looks like we need to teach another code generator about [Supplemental].

Ah... this is _another_ build regression in GTK:-) I'll take a look on Monday.
------- Comment #19 From 2012-02-20 03:08:17 PST -------
(From update of attachment 127662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127662&action=review

> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:2
> + * Copyright (C) 2011, Google Inc. All rights reserved.

2012, and elsewhere

> Source/WebCore/Modules/geolocation/NavigatorGeolocation.idl:4
> + * This library is free software; you can redistribute it and/or

Is this intended to be LGPL?

> Source/WebCore/page/Frame.cpp:666
>      // This cleanup is needed whenever we remove a frame from page.

Stale comment
------- Comment #20 From 2012-02-20 19:48:25 PST -------
(From update of attachment 127662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127662&action=review

>> Source/WebCore/Modules/geolocation/NavigatorGeolocation.idl:4
>> + * This library is free software; you can redistribute it and/or
> 
> Is this intended to be LGPL?

Dunno.  This comes from Navigator.idl, so maybe it needs to be LGPL to match?
------- Comment #21 From 2012-02-20 20:24:14 PST -------
*** Bug 77758 has been marked as a duplicate of this bug. ***
------- Comment #22 From 2012-02-21 11:28:56 PST -------
Created an attachment (id=128007) [details]
Patch
------- Comment #23 From 2012-02-21 11:49:33 PST -------
(From update of attachment 128007 [details])
Looks good.  Thanks.
------- Comment #24 From 2012-02-21 12:00:44 PST -------
(From update of attachment 128007 [details])
Attachment 128007 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11520210
------- Comment #25 From 2012-02-21 12:24:38 PST -------
(From update of attachment 128007 [details])
Attachment 128007 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11558195
------- Comment #26 From 2012-02-21 13:04:56 PST -------
Created an attachment (id=128028) [details]
should fix qt
------- Comment #27 From 2012-02-21 13:20:15 PST -------
Created an attachment (id=128033) [details]
for EWSWS
------- Comment #28 From 2012-02-21 13:51:49 PST -------
(From update of attachment 128033 [details])
Attachment 128033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11562241
------- Comment #29 From 2012-02-21 14:39:36 PST -------
Created an attachment (id=128047) [details]
Patch for landing
------- Comment #30 From 2012-02-21 14:40:48 PST -------
Created an attachment (id=128048) [details]
Patch for landing
------- Comment #31 From 2012-02-21 15:18:18 PST -------
(From update of attachment 128048 [details])
Attachment 128048 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11557267
------- Comment #32 From 2012-02-21 15:52:26 PST -------
Created an attachment (id=128065) [details]
Patch for landing
------- Comment #33 From 2012-02-21 16:51:08 PST -------
(From update of attachment 128065 [details])
Rejecting attachment 128065 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 73761.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11556324
------- Comment #34 From 2012-02-21 16:59:22 PST -------
Created an attachment (id=128085) [details]
Patch for landing
------- Comment #35 From 2012-02-21 18:22:04 PST -------
(From update of attachment 128085 [details])
Clearing flags on attachment: 128085

Committed r108428: <http://trac.webkit.org/changeset/108428>
------- Comment #36 From 2012-02-21 18:22:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #37 From 2012-02-22 15:43:57 PST -------
mrobinson landed the patch for the GTK supplemental build fix: https://bugs.webkit.org/show_bug.cgi?id=76388

Shall we now try to remove "!defined(LANGUAGE_GOBJECT)"?
------- Comment #38 From 2012-02-22 16:03:43 PST -------
> Shall we now try to remove "!defined(LANGUAGE_GOBJECT)"?

Sure.  I'll do that in a separate bug.