Bug 197434 - [WinCairo] Implement Remote Web Inspector Client.
Summary: [WinCairo] Implement Remote Web Inspector Client.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks: 197029
  Show dependency treegraph
 
Reported: 2019-04-30 13:10 PDT by Basuke Suzuki
Modified: 2019-05-20 14:18 PDT (History)
8 users (show)

See Also:


Attachments
PATCH (35.65 KB, patch)
2019-05-01 14:58 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (35.86 KB, patch)
2019-05-01 15:01 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (37.19 KB, patch)
2019-05-16 16:16 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (39.06 KB, patch)
2019-05-17 11:43 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (39.06 KB, patch)
2019-05-20 10:16 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (39.09 KB, patch)
2019-05-20 11:39 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2019-04-30 13:10:57 PDT
Add implementation of Remote Web Inspector client for WinCairo.
Comment 1 Basuke Suzuki 2019-05-01 14:58:48 PDT Comment hidden (obsolete)
Comment 2 Basuke Suzuki 2019-05-01 15:01:29 PDT Comment hidden (obsolete)
Comment 3 Ross Kirsling 2019-05-01 16:46:29 PDT Comment hidden (obsolete)
Comment 4 Basuke Suzuki 2019-05-01 16:59:13 PDT Comment hidden (obsolete)
Comment 5 Ross Kirsling 2019-05-16 16:16:04 PDT
Created attachment 370083 [details]
Patch
Comment 6 Christopher Reid 2019-05-16 18:15:40 PDT
Comment on attachment 370083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370083&action=review

Some informal comments

> Source/WebKit/UIProcess/win/RemoteInspectorClient.cpp:93
> +        RELEASE_LOG("Failed");

Is this release log needed?

> Source/WebKit/UIProcess/win/WebView.cpp:241
> +    m_page->setURLSchemeHandlerForScheme(RemoteInspectorProtocolHandler::create(*m_page), "inspector");

It would be nice if this scheme was just accessible from URL bar navigations and not links in pages. It's consistent with what GTK does currently so it might be okay.
Comment 7 Ross Kirsling 2019-05-17 11:43:41 PDT
Created attachment 370131 [details]
Patch
Comment 8 Ross Kirsling 2019-05-20 10:16:58 PDT
Created attachment 370258 [details]
Patch
Comment 9 Don Olmstead 2019-05-20 10:38:42 PDT
Comment on attachment 370258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370258&action=review

It looks like Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp is the only real Windows file specific file. I would probably put the rest into a socket directory to match whats happening in JavaScriptCore.

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173
> +#if !ENABLE(REMOTE_INSPECTOR) || (!PLATFORM(MAC) && !PLATFORM(GTK) && !PLATFORM(WIN_CAIRO))

It seems like this is basically asking if its inspectable from itself. Are there other places where this is present? If so I would say to make this into its own ENABLE or USE. If not I would do || PLATFORM(IOS) || PLATFORM(WPE) and maybe even add a comment about it.
Comment 10 Ross Kirsling 2019-05-20 11:39:55 PDT
Created attachment 370267 [details]
Patch
Comment 11 Don Olmstead 2019-05-20 12:35:10 PDT
Comment on attachment 370267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370267&action=review

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173
> +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC)

You're implementing this below. This is incorrect.

> Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101
> +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow()

Here's an implementation.
Comment 12 Ross Kirsling 2019-05-20 12:52:04 PDT
(In reply to Don Olmstead from comment #11)
> Comment on attachment 370267 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370267&action=review
> 
> > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173
> > +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC)
> 
> You're implementing this below. This is incorrect.
> 
> > Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101
> > +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow()
> 
> Here's an implementation.

It's not incorrect. RWIProxyMac is the only implementation that's not conditioned on ENABLE(REMOTE_INSPECTOR).
Comment 13 Don Olmstead 2019-05-20 12:54:22 PDT
(In reply to Ross Kirsling from comment #12)
> (In reply to Don Olmstead from comment #11)
> > Comment on attachment 370267 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=370267&action=review
> > 
> > > Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:173
> > > +#if !ENABLE(REMOTE_INSPECTOR) && !PLATFORM(MAC)
> > 
> > You're implementing this below. This is incorrect.
> > 
> > > Source/WebKit/UIProcess/win/RemoteWebInspectorProxyWin.cpp:101
> > > +WebPageProxy* RemoteWebInspectorProxy::platformCreateFrontendPageAndWindow()
> > 
> > Here's an implementation.
> 
> It's not incorrect. RWIProxyMac is the only implementation that's not
> conditioned on ENABLE(REMOTE_INSPECTOR).

My bad. I misread it as an || like the old one.
Comment 14 WebKit Commit Bot 2019-05-20 14:17:07 PDT
Comment on attachment 370267 [details]
Patch

Clearing flags on attachment: 370267

Committed r245536: <https://trac.webkit.org/changeset/245536>
Comment 15 WebKit Commit Bot 2019-05-20 14:17:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-05-20 14:18:26 PDT
<rdar://problem/50960229>