Bug 78860 – Frame and Navigator shouldn't need to worry about Geolocation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Frame and Navigator shouldn't need to worry about Geolocation

[Adam Barth]

Created attachment 127475 [details]
work in progress

[Eric Seidel (no email)]

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?

[Adam Barth]

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.

[Adam Barth]

Created attachment 127657 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

(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.

[Adam Barth]

> 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.

[Adam Barth]

Created attachment 127662 [details]
Patch

[Eric Seidel (no email)]

(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. :)

[Eric Seidel (no email)]

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()...

[Adam Barth]

> 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.

[Adam Barth]

(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.

[Philippe Normand]

Comment on attachment 127662 [details]
Patch

Attachment 127662 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11535706

[Adam Barth]

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.

[Adam Barth]

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].

[Early Warning System Bot]

Comment on attachment 127662 [details]
Patch

Attachment 127662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11543299

[Kentaro Hara]

(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.

[Kentaro Hara]

(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.

[Steve Block]

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

[Adam Barth]

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?

[Hajime Morrita]

*** Bug 77758 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 128007 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 128007 [details]
Patch

Looks good.  Thanks.

[Philippe Normand]

Comment on attachment 128007 [details]
Patch

Attachment 128007 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11520210

[Early Warning System Bot]

Comment on attachment 128007 [details]
Patch

Attachment 128007 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11558195

[Adam Barth]

Created attachment 128028 [details]
should fix qt

[Adam Barth]

Created attachment 128033 [details]
for EWSWS

[Philippe Normand]

Comment on attachment 128033 [details]
for EWSWS

Attachment 128033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11562241

[Adam Barth]

Created attachment 128047 [details]
Patch for landing

[Adam Barth]

Created attachment 128048 [details]
Patch for landing

[Gustavo Noronha (kov)]

Comment on attachment 128048 [details]
Patch for landing

Attachment 128048 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11557267

[Adam Barth]

Created attachment 128065 [details]
Patch for landing

[WebKit Review Bot]

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

[Adam Barth]

Created attachment 128085 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128085 [details]
Patch for landing

Clearing flags on attachment: 128085

Committed r108428: <http://trac.webkit.org/changeset/108428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

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)"?

[Adam Barth]

> Shall we now try to remove "!defined(LANGUAGE_GOBJECT)"?

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