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
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Adam Barth
:
Depends on:
Blocks: 79327
  Show dependency treegraph
 
Reported: 2012-02-16 17:08 PST by Adam Barth
Modified: 2012-02-23 20:58 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-02-16 17:08:22 PST
Frame and Navigator shouldn't need to worry about Geolocation
Comment 1 Adam Barth 2012-02-16 17:08:40 PST
Created attachment 127475 [details]
work in progress
Comment 2 Eric Seidel 2012-02-16 17:17:27 PST
Comment on attachment 127475 [details]
work in progress

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 Adam Barth 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 Adam Barth 2012-02-17 14:12:00 PST
Created attachment 127657 [details]
Patch
Comment 5 Eric Seidel 2012-02-17 14:22:41 PST
Comment on attachment 127657 [details]
Patch

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 Adam Barth 2012-02-17 14:32:10 PST
(In reply to comment #5)
> (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?

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 Adam Barth 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 Adam Barth 2012-02-17 15:04:49 PST
Created attachment 127662 [details]
Patch
Comment 9 Eric Seidel 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 Eric Seidel 2012-02-17 15:14:11 PST
Comment on attachment 127662 [details]
Patch

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 Adam Barth 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 Adam Barth 2012-02-17 15:15:40 PST
(In reply to comment #10)
> (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()...

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 Philippe Normand 2012-02-17 15:50:30 PST
Comment on attachment 127662 [details]
Patch

Attachment 127662 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11535706
Comment 14 Adam Barth 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 Adam Barth 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 Early Warning System Bot 2012-02-17 15:57:27 PST
Comment on attachment 127662 [details]
Patch

Attachment 127662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11543299
Comment 17 Kentaro Hara 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 Kentaro Hara 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 Steve Block 2012-02-20 03:08:17 PST
Comment on attachment 127662 [details]
Patch

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 Adam Barth 2012-02-20 19:48:25 PST
Comment on attachment 127662 [details]
Patch

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 Hajime Morrita 2012-02-20 20:24:14 PST
*** Bug 77758 has been marked as a duplicate of this bug. ***
Comment 22 Adam Barth 2012-02-21 11:28:56 PST
Created attachment 128007 [details]
Patch
Comment 23 Eric Seidel 2012-02-21 11:49:33 PST
Comment on attachment 128007 [details]
Patch

Looks good.  Thanks.
Comment 24 Philippe Normand 2012-02-21 12:00:44 PST
Comment on attachment 128007 [details]
Patch

Attachment 128007 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11520210
Comment 25 Early Warning System Bot 2012-02-21 12:24:38 PST
Comment on attachment 128007 [details]
Patch

Attachment 128007 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11558195
Comment 26 Adam Barth 2012-02-21 13:04:56 PST
Created attachment 128028 [details]
should fix qt
Comment 27 Adam Barth 2012-02-21 13:20:15 PST
Created attachment 128033 [details]
for EWSWS
Comment 28 Philippe Normand 2012-02-21 13:51:49 PST
Comment on attachment 128033 [details]
for EWSWS

Attachment 128033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11562241
Comment 29 Adam Barth 2012-02-21 14:39:36 PST
Created attachment 128047 [details]
Patch for landing
Comment 30 Adam Barth 2012-02-21 14:40:48 PST
Created attachment 128048 [details]
Patch for landing
Comment 31 Gustavo Noronha (kov) 2012-02-21 15:18:18 PST
Comment on attachment 128048 [details]
Patch for landing

Attachment 128048 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11557267
Comment 32 Adam Barth 2012-02-21 15:52:26 PST
Created attachment 128065 [details]
Patch for landing
Comment 33 WebKit Review Bot 2012-02-21 16:51:08 PST
Comment on attachment 128065 [details]
Patch for landing

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 Adam Barth 2012-02-21 16:59:22 PST
Created attachment 128085 [details]
Patch for landing
Comment 35 WebKit Review Bot 2012-02-21 18:22:04 PST
Comment on attachment 128085 [details]
Patch for landing

Clearing flags on attachment: 128085

Committed r108428: <http://trac.webkit.org/changeset/108428>
Comment 36 WebKit Review Bot 2012-02-21 18:22:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Kentaro Hara 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 Adam Barth 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.