Bug 78860

Created attachment 127475 [details] work in progress

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?

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.

Created attachment 127657 [details] Patch

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?

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

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

Created attachment 127662 [details] Patch

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

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

(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 on attachment 127662 [details] Patch Attachment 127662 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11535706

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.

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 on attachment 127662 [details] Patch Attachment 127662 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11543299

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

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

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

Created attachment 128007 [details] Patch

Comment on attachment 128007 [details] Patch Looks good. Thanks.

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

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

Created attachment 128028 [details] should fix qt

Created attachment 128033 [details] for EWSWS

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

Created attachment 128047 [details] Patch for landing

Created attachment 128048 [details] Patch for landing

Comment on attachment 128048 [details] Patch for landing Attachment 128048 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11557267

Created attachment 128065 [details] Patch for landing

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

Created attachment 128085 [details] Patch for landing

Comment on attachment 128085 [details] Patch for landing Clearing flags on attachment: 128085 Committed r108428: <http://trac.webkit.org/changeset/108428>

All reviewed patches have been landed. Closing bug.

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

> Shall we now try to remove "!defined(LANGUAGE_GOBJECT)"? Sure. I'll do that in a separate bug.