WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
149071
Web Inspector: disambiguate inspected/frontend controllers and pages in backend code
https://bugs.webkit.org/show_bug.cgi?id=149071
Summary
Web Inspector: disambiguate inspected/frontend controllers and pages in backe...
Blaze Burg
Reported
2015-09-11 13:37:20 PDT
Splitting this from other changes to make easy diffing.
Attachments
Patch
(55.68 KB, patch)
2015-09-11 14:25 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Proposed Fix (fix ios)
(58.15 KB, patch)
2015-09-11 15:01 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Proposed Fix (EWS)
(58.17 KB, patch)
2015-09-11 16:37 PDT
,
Blaze Burg
joepeck
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2015-09-11 14:25:05 PDT
Created
attachment 261014
[details]
Patch
WebKit Commit Bot
Comment 2
2015-09-11 14:26:35 PDT
Attachment 261014
[details]
did not pass style-queue: ERROR: Source/WebKit/win/WebInspector.cpp:108: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKit/win/WebInspector.cpp:130: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Blaze Burg
Comment 3
2015-09-11 15:01:43 PDT
Created
attachment 261020
[details]
Proposed Fix (fix ios)
WebKit Commit Bot
Comment 4
2015-09-11 15:04:57 PDT
Attachment 261020
[details]
did not pass style-queue: ERROR: Source/WebKit/win/WebInspector.cpp:108: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKit/win/WebInspector.cpp:130: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Blaze Burg
Comment 5
2015-09-11 16:37:45 PDT
Created
attachment 261028
[details]
Proposed Fix (EWS)
WebKit Commit Bot
Comment 6
2015-09-11 16:39:52 PDT
Attachment 261028
[details]
did not pass style-queue: ERROR: Source/WebKit/win/WebInspector.cpp:108: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebKit/win/WebInspector.cpp:130: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 7
2015-09-11 17:31:09 PDT
Comment on
attachment 261028
[details]
Proposed Fix (EWS) View in context:
https://bugs.webkit.org/attachment.cgi?id=261028&action=review
r=me
> Source/WebCore/WebCore.order:17081 > -__ZN7WebCore20EmptyInspectorClient18inspectorDestroyedEv > +__ZN7WebCore20EmptyInspectorClient22inspectedPageDestroyedEv
We typically do not change order files. This seems harmless, but you may want to just not touch this at all.
> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:131 > InspectorFrontendClientLocal::InspectorFrontendClientLocal(InspectorController* inspectedPageController, Page* frontendPage, std::unique_ptr<Settings> settings) > - : m_inspectorController(inspectedPageController) > + : m_inspectedPageController(inspectedPageController)
Hmm, I'm not sure about this one. It reads a bit funny. Was there ever a case where "inspectorController" did not mean the WebCore::InspectorController for the inspected page?
> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:73 > + WebInspectorFrontendClient *_frontendClient; > + WebInspectorClient *_inspectorClient;
Style: These types are not-ObjC so the * should be on the left (on the type).
> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:88 > +- (void)setFrontendClient:(WebInspectorFrontendClient *)frontendClient; > +- (void)setInspectorClient:(WebInspectorClient *)inspectorClient; > +- (WebInspectorClient *)inspectorClient;
Same with all of these.
> Source/WebKit/mac/WebInspector/WebInspector.mm:179 > + // FIXME: this is a really roundabout way to evaluate in the frontend. > + // We could directly use the frontend instead of going through the inspected page. > + if (Page* inspectedPage = core(_inspectedWebView)) > + inspectedPage->inspectorController().evaluateForTestInFrontend(script);
Hmm, I think sending to InspectorController is appropriate. Currently it does InspectorController -> InspectorAgent -> event. If there are multiple frontends, this would ensure it goes to all frontends.
> Source/WebKit/mac/WebKit.order:1024 > +__ZN18WebInspectorClient22inspectedPageDestroyedEv > +__ZN18WebInspectorClient18closeLocalFrontendEv
Ditto: Order files.
> Source/WebKit2/mac/WebKit2.order:4328 > +__ZN6WebKit18WebInspectorClient22inspectedPageDestroyedEv > +__ZN6WebKit18WebInspectorClient18closeLocalFrontendEv
Ditto: Order files.
Blaze Burg
Comment 8
2015-09-12 10:12:57 PDT
Comment on
attachment 261028
[details]
Proposed Fix (EWS) View in context:
https://bugs.webkit.org/attachment.cgi?id=261028&action=review
>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:131 >> + : m_inspectedPageController(inspectedPageController) > > Hmm, I'm not sure about this one. It reads a bit funny. > > Was there ever a case where "inspectorController" did not mean the WebCore::InspectorController for the inspected page?
In the stub frontend, the frontendClient is hooked to the frontend page's inspector controller, and the FrontendChannel is connected to the inspected page's inspector controller.
>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:73 >> + WebInspectorClient *_inspectorClient; > > Style: These types are not-ObjC so the * should be on the left (on the type).
Oops, ok. Will double-check style guide and update if necessary.
>> Source/WebKit/mac/WebInspector/WebInspector.mm:179 >> + inspectedPage->inspectorController().evaluateForTestInFrontend(script); > > Hmm, I think sending to InspectorController is appropriate. Currently it does InspectorController -> InspectorAgent -> event. If there are multiple frontends, this would ensure it goes to all frontends.
Hmm, I suppose that would make sense to ensure it's actually sent over a FrontendChannel. Other code paths for WK2, win, etc do the same thing. I'll remove the comment.
Blaze Burg
Comment 9
2015-09-12 10:44:51 PDT
Committed
r189654
: <
http://trac.webkit.org/changeset/189654
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug