PFR soon.
<rdar://problem/23466749>
Created attachment 265923 [details] Proposed Fix This might not compile yet, I just rebased it before heading out.
Any chance you could provide a diff that doesn't treat the renamed files as completely new?
Comment on attachment 265923 [details] Proposed Fix Attachment 265923 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/452139 Number of test failures exceeded the failure limit.
Created attachment 265925 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265923 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=265923&action=review Looking good, but I wasn't able to really determine the changes in the Debuggable classes due to all the new files. I'll take a deeper luck later (and maybe apply the patch locally to get a real diff). > Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:39 > +class JS_EXPORT_PRIVATE RemoteControlledTarget { This name sounds weird to me. Maybe just "RemoteTarget"? I see the idea though. "RemoteTarget" sounds like something remote... Unfortunately here "Remote" is just a prefix, not a description. > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:121 > - HashMap<unsigned, std::pair<RemoteInspectorDebuggable*, RemoteInspectorDebuggableInfo>> m_debuggableMap; > - HashMap<unsigned, RefPtr<RemoteInspectorDebuggableConnection>> m_connectionMap; > + HashMap<unsigned, RemoteControlledTarget*> m_targetMap; > + HashMap<unsigned, RetainPtr<NSDictionary>> m_listingMap; Oh, much nicer (listingMap instead of sticking the thread safe info into the debuggable map)! > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:688 > + std::lock_guard<Lock> lock(m_mutex); Does this lock guard anything? I think the only thing the block captures is self. > Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:51 > - virtual Inspector::RemoteInspectorDebuggable::DebuggableType type() const override { return Inspector::RemoteInspectorDebuggable::JavaScript; } > + virtual Inspector::RemoteControlledTarget::Type type() const override { return Inspector::RemoteControlledTarget::Type::JavaScript; } Not sure how I feel about this naming. "RemoteControlled" seems prescriptive rather then descriptive. "Target" and "RemoteInspectionTarget" I think are good. > Source/WebKit2/UIProcess/WebAutomationTarget.h:36 > +namespace Inspector { > +class FrontendChannel; > +} Probably not needed since these are present in the virtual methods. > Source/WebKit2/UIProcess/WebProcessPool.cpp:475 > + m_sharedAutomationTarget->setAutomationAllowed(!privateBrowsingEnabled); This should be guarded by ENABLE(REMOTE_INSPECTOR).
Comment on attachment 265923 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=265923&action=review >> Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:39 >> +class JS_EXPORT_PRIVATE RemoteControlledTarget { > > This name sounds weird to me. Maybe just "RemoteTarget"? I see the idea though. "RemoteTarget" sounds like something remote... Unfortunately here "Remote" is just a prefix, not a description. This was my least favorite name in this patch. Maybe ControllableTarget? > Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:55 > + virtual void dispatchMessageFromRemote(const String& message) = 0; ...and yet the API is somewhat specific to *remote* controlling. It is not that different from local frontend channels, though. >> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:121 >> + HashMap<unsigned, RetainPtr<NSDictionary>> m_listingMap; > > Oh, much nicer (listingMap instead of sticking the thread safe info into the debuggable map)! This was the best solution I could come up with that is polymorphic and not terrible or requiring sending listings from main / target run loop. >> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:688 >> + std::lock_guard<Lock> lock(m_mutex); > > Does this lock guard anything? I think the only thing the block captures is self. Oops, I think it used to guard something in earlier patch, but now the inner lock suffices. >> Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:51 >> + virtual Inspector::RemoteControlledTarget::Type type() const override { return Inspector::RemoteControlledTarget::Type::JavaScript; } > > Not sure how I feel about this naming. "RemoteControlled" seems prescriptive rather then descriptive. "Target" and "RemoteInspectionTarget" I think are good. See above. >> Source/WebKit2/UIProcess/WebAutomationTarget.h:36 >> +} > > Probably not needed since these are present in the virtual methods. Oops. >> Source/WebKit2/UIProcess/WebProcessPool.cpp:475 >> + m_sharedAutomationTarget->setAutomationAllowed(!privateBrowsingEnabled); > > This should be guarded by ENABLE(REMOTE_INSPECTOR). OK
Created attachment 265943 [details] Proposed Fix v2 (better formatting) Addressed comments, and told git to treat it as a rename harder. (git diff --staged -M0.5)
Comment on attachment 265943 [details] Proposed Fix v2 (better formatting) View in context: https://bugs.webkit.org/attachment.cgi?id=265943&action=review r=me, but there are some necessary fixes. > Source/JavaScriptCore/inspector/remote/ControllableTarget.cpp:27 > +#include "ControllableTarget.h" All of the files in JavaScriptCore/inspector/remote started with the "Remote" prefix. I'm fine with either, but might be nice to be consistent even if the name isn't the greatest. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.hSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.h:94 > + // Configuring the queue on which dispatches are performed. None of these seem to have to do with configuring. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.hSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.h:123 > + ControllableTarget* m_target { nullptr }; > + unsigned m_identifier {0}; Style: Converge on one style. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:162 > - std::lock_guard<Lock> lock(m_debuggableMutex); > - if (!m_debuggable || !m_debuggable->remoteDebuggingAllowed() || m_debuggable->hasLocalDebugger()) { > - RemoteInspector::singleton().setupFailed(identifier()); > - m_debuggable = nullptr; > - } else { > - m_debuggable->connect(this, isAutomaticInspection); > + std::lock_guard<Lock> lock(m_targetMutex); > + if (!m_target || !m_target->remoteControlAllowed()) { > + RemoteInspector::singleton().setupFailed(m_identifier); This change should be documented in the ChangeLog. Previously we would have failed if inspecting a debuggable that had a local debugger. Now we don't. InspectorRouter allows multiple frontends now. EDIT: Reading more of the patch I see you moved combined these two checks into remoteControlAllowed. Seems okay. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:164 > + return; This early return is bad. It misses the necessary balancing "deref()" for this connection's action on a different queue. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:177 > + if (is<RemoteInspectionTarget>(m_target)) { > + auto castedTarget = downcast<RemoteInspectionTarget>(m_target); > + castedTarget->connect(this, isAutomaticInspection); > m_connected = true; > > if (automaticallyPause) > - m_debuggable->pause(); > + castedTarget->pause(); > + } else if (is<RemoteAutomationTarget>(m_target)) { > + auto castedTarget = downcast<RemoteAutomationTarget>(m_target); > + castedTarget->connect(this); > } `m_connected = true` should be set for both of these cases in order to call disconnect when the connection is closed. Hoisting this above the if/elses seems fine. > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:45 > +class RemoteConnectionToTarget; > +class ControllableTarget; Style: sort > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:487 > + RetainPtr<NSMutableDictionary> listing = [[NSMutableDictionary alloc] init]; This is a leak. It needs adoptNS(...). > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:523 > + RetainPtr<NSMutableDictionary> listing = [[NSMutableDictionary alloc] init]; This is a leak. It needs adoptNS(...). > Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:621 > + updateHasActiveDebugSession(); We may want this for Automation as well. At the moment this is basically dead code with a FIXME, but its intention is to inform the UIProcess to prevent screen idling behavior on iOS. > Source/WebKit2/UIProcess/WebAutomationTarget.h:36 > +namespace Inspector { > +class FrontendChannel; > +} Nit: Not needed. > Source/WebKit2/UIProcess/WebProcessPool.h:543 > + std::unique_ptr<WebAutomationTarget> m_sharedAutomationTarget; Is the "shared" prefix significant? Shared with what?
Created attachment 266023 [details] Addressed Comments We may want to hold off landing this until a better location for RemoteAutomationTarget is figured out. Or, if it ends up requiring other changes (to accomodate Host Application being reported earlier), then maybe land this and change in a followup bug.
(In reply to comment #10) > Created attachment 266023 [details] > Addressed Comments > > We may want to hold off landing this until a better location for > RemoteAutomationTarget is figured out. Or, if it ends up requiring other > changes (to accomodate Host Application being reported earlier), then maybe > land this and change in a followup bug. A potential solution: * Host (UIProcess) sets up RemoteInspector connection immediately, instead of on first WebProcess creation. * Add an SPI or setting somewhere that can flag a WebProcessPool as being in Remote Automation mode. This causes the UIProcess to instantiate an owned WebAutomationTarget that calls RemoteInspector::{,un}registerTarget. We definitely should split WebAutomationTarget and the WK2 bits from this patch, which is mainly a refactoring of RemoteInspector.
Committed r192753: <http://trac.webkit.org/changeset/192753>
(In reply to comment #12) > Committed r192753: <http://trac.webkit.org/changeset/192753> It broke the Apple Mac cmake build.
(In reply to comment #13) > (In reply to comment #12) > > Committed r192753: <http://trac.webkit.org/changeset/192753> > > It broke the Apple Mac cmake build. Woops, I should start checking that. This was detectable just in the deps checking, no build required. Fixed in http://trac.webkit.org/changeset/192759 (I messed up the commit message, wrong revision!)