Bug 149071

Summary: Web Inspector: disambiguate inspected/frontend controllers and pages in backend code
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, japhet, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 149006    
Attachments:
Description Flags
Patch
none
Proposed Fix (fix ios)
none
Proposed Fix (EWS) joepeck: review+

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>