Bug 149071 – Web Inspector: disambiguate inspected/frontend controllers and pages in backend code

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Splitting this from other changes to make easy diffing.

[Blaze Burg]

Created attachment 261014 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 261020 [details]
Proposed Fix (fix ios)

[WebKit Commit Bot]

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]

Created attachment 261028 [details]
Proposed Fix (EWS)

[WebKit Commit Bot]

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

Committed r189654: <http://trac.webkit.org/changeset/189654>