Bug 149071 - Web Inspector: disambiguate inspected/frontend controllers and pages in backend code
Summary: Web Inspector: disambiguate inspected/frontend controllers and pages in backe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks: 149006
  Show dependency treegraph
 
Reported: 2015-09-11 13:37 PDT by BJ Burg
Modified: 2015-09-12 10:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (55.68 KB, patch)
2015-09-11 14:25 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (fix ios) (58.15 KB, patch)
2015-09-11 15:01 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (EWS) (58.17 KB, patch)
2015-09-11 16:37 PDT, BJ Burg
joepeck: review+
bburg: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-09-11 13:37:20 PDT
Splitting this from other changes to make easy diffing.
Comment 1 BJ Burg 2015-09-11 14:25:05 PDT
Created attachment 261014 [details]
Patch
Comment 2 WebKit Commit Bot 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.
Comment 3 BJ Burg 2015-09-11 15:01:43 PDT
Created attachment 261020 [details]
Proposed Fix (fix ios)
Comment 4 WebKit Commit Bot 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.
Comment 5 BJ Burg 2015-09-11 16:37:45 PDT
Created attachment 261028 [details]
Proposed Fix (EWS)
Comment 6 WebKit Commit Bot 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.
Comment 7 Joseph Pecoraro 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.
Comment 8 BJ Burg 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.
Comment 9 BJ Burg 2015-09-12 10:44:51 PDT
Committed r189654: <http://trac.webkit.org/changeset/189654>