Bug 177661 - Web Inspector: consolidate code that hosts the Inspector page inside a WKWebView
Summary: Web Inspector: consolidate code that hosts the Inspector page inside a WKWebView
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: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-29 09:21 PDT by BJ Burg
Modified: 2017-10-20 10:31 PDT (History)
6 users (show)

See Also:

Attachments
Combined Patch (92.30 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[1/6] Staged Patch (36.23 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[2/6] Staged Patch (21.91 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[3/6] Staged Patch (5.30 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[5/6] Staged Patch (28.24 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[6/6] Staged Patch (7.38 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[7/7] Staged Patch (6.29 KB, patch)
2017-10-03 16:16 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[4/6] Staged Patch (14.58 KB, patch)
2017-10-03 16:22 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Combined Patch (60.57 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[1/5] Staged Patch (36.22 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[2/5] Staged Patch (21.84 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[3/5] Staged Patch (5.30 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[4/5] Staged Patch (10.62 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[5/5] Staged Patch (28.18 KB, patch)
2017-10-05 13:13 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Combined Patch (60.57 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[1/5] Staged Patch (36.22 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[2/5] Staged Patch (21.84 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[3/5] Staged Patch (5.30 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[4/5] Staged Patch (10.62 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[5/5] Staged Patch (28.22 KB, patch)
2017-10-05 13:15 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Combined Patch (60.57 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[1/5] Staged Patch (36.22 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[2/5] Staged Patch (21.84 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[3/5] Staged Patch (5.30 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[4/5] Staged Patch (10.62 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
[5/5] Staged Patch (28.22 KB, patch)
2017-10-05 13:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Combined Patch (86.76 KB, patch)
2017-10-06 17:26 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Combined Patch (86.91 KB, patch)
2017-10-06 20:07 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
For EWS (86.92 KB, patch)
2017-10-09 12:36 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Show Obsolete (28) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-09-29 09:21:01 PDT 
Goal: reduce duplicated code between WebInspectorProxy and RemoteWebInspectorProxy, and keep all WebView-related integrations contained to one class so they are easier to change in the future.
Comment 1 Radar WebKit Bug Importer 2017-09-29 09:21:35 PDT 
<rdar://problem/34740286>
Comment 2 BJ Burg 2017-10-03 16:16:20 PDT 
Created attachment 322604 [details]
Combined Patch
Comment 3 BJ Burg 2017-10-03 16:16:25 PDT 
Created attachment 322605 [details]
[1/6] Staged Patch
Comment 4 BJ Burg 2017-10-03 16:16:29 PDT 
Created attachment 322606 [details]
[2/6] Staged Patch
Comment 5 BJ Burg 2017-10-03 16:16:33 PDT 
Created attachment 322607 [details]
[3/6] Staged Patch
Comment 6 BJ Burg 2017-10-03 16:16:41 PDT 
Created attachment 322608 [details]
[5/6] Staged Patch
Comment 7 BJ Burg 2017-10-03 16:16:45 PDT 
Created attachment 322609 [details]
[6/6] Staged Patch
Comment 8 BJ Burg 2017-10-03 16:16:50 PDT 
Created attachment 322610 [details]
[7/7] Staged Patch
Comment 9 BJ Burg 2017-10-03 16:22:19 PDT 
Created attachment 322612 [details]
[4/6] Staged Patch

webkit-patch didn't like this one, adding manually.
Comment 10 BJ Burg 2017-10-03 16:22:57 PDT 
I'll fix the WPE oopsie.
Comment 11 Joseph Pecoraro 2017-10-03 16:52:07 PDT 
Comment on attachment 322606 [details]
[2/6] Staged Patch

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

> Source/WebKit/UIProcess/wpe/WebInspectorProxyWPE.cpp:47
> +    notImplemented()

This would need semicolon.
Comment 12 Joseph Pecoraro 2017-10-03 16:57:17 PDT 
Comment on attachment 322609 [details]
[6/6] Staged Patch

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

> Source/WebKit/ChangeLog:10
> +        First, rename some helpers so its clear whether the argument is the inspector page
> +        or the inspected page. At many of the call sites, both of these are being used and

I do not think this rename makes sense. The level is not specific to being an inspected page. Every Page has an inspection level. The level of the Inspected Page will be 1, the Frontend Page for that will be 2, and a Multi-level Frontend Page will be higher. Every page has a level, the caller is the one responsible to supply the page they are interested in getting the level for.
Comment 13 BJ Burg 2017-10-04 15:56:17 PDT 
(In reply to Joseph Pecoraro from comment #12)
> Comment on attachment 322609 [details]
> [6/6] Staged Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322609&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        First, rename some helpers so its clear whether the argument is the inspector page
> > +        or the inspected page. At many of the call sites, both of these are being used and
> 
> I do not think this rename makes sense. The level is not specific to being
> an inspected page. Every Page has an inspection level. The level of the
> Inspected Page will be 1, the Frontend Page for that will be 2, and a
> Multi-level Frontend Page will be higher. Every page has a level, the caller
> is the one responsible to supply the page they are interested in getting the
> level for.

I had thought that every argument to this was an inspected WebPageProxy, but on further review some of them are not. I will drop this renaming.
Comment 14 BJ Burg 2017-10-05 13:13:22 PDT 
Created attachment 322890 [details]
Combined Patch
Comment 15 BJ Burg 2017-10-05 13:13:26 PDT 
Created attachment 322891 [details]
[1/5] Staged Patch
Comment 16 BJ Burg 2017-10-05 13:13:30 PDT 
Created attachment 322892 [details]
[2/5] Staged Patch
Comment 17 BJ Burg 2017-10-05 13:13:34 PDT 
Created attachment 322893 [details]
[3/5] Staged Patch
Comment 18 BJ Burg 2017-10-05 13:13:38 PDT 
Created attachment 322894 [details]
[4/5] Staged Patch
Comment 19 BJ Burg 2017-10-05 13:13:42 PDT 
Created attachment 322895 [details]
[5/5] Staged Patch
Comment 20 BJ Burg 2017-10-05 13:15:25 PDT 
Created attachment 322896 [details]
Combined Patch
Comment 21 BJ Burg 2017-10-05 13:15:30 PDT 
Created attachment 322897 [details]
[1/5] Staged Patch
Comment 22 BJ Burg 2017-10-05 13:15:34 PDT 
Created attachment 322898 [details]
[2/5] Staged Patch
Comment 23 BJ Burg 2017-10-05 13:15:38 PDT 
Created attachment 322899 [details]
[3/5] Staged Patch
Comment 24 BJ Burg 2017-10-05 13:15:42 PDT 
Created attachment 322900 [details]
[4/5] Staged Patch
Comment 25 BJ Burg 2017-10-05 13:15:46 PDT 
Created attachment 322901 [details]
[5/5] Staged Patch
Comment 26 BJ Burg 2017-10-05 13:26:26 PDT 
Created attachment 322905 [details]
Combined Patch
Comment 27 BJ Burg 2017-10-05 13:26:31 PDT 
Created attachment 322906 [details]
[1/5] Staged Patch
Comment 28 BJ Burg 2017-10-05 13:26:35 PDT 
Created attachment 322907 [details]
[2/5] Staged Patch
Comment 29 BJ Burg 2017-10-05 13:26:39 PDT 
Created attachment 322908 [details]
[3/5] Staged Patch
Comment 30 BJ Burg 2017-10-05 13:26:43 PDT 
Created attachment 322909 [details]
[4/5] Staged Patch
Comment 31 BJ Burg 2017-10-05 13:26:47 PDT 
Created attachment 322910 [details]
[5/5] Staged Patch
Comment 32 BJ Burg 2017-10-05 13:42:56 PDT 
I have rebased this several times and our bots are none the wiser.
Comment 33 BJ Burg 2017-10-06 17:26:27 PDT 
Created attachment 323073 [details]
Combined Patch
Comment 34 Joseph Pecoraro 2017-10-06 17:36:36 PDT 
Comment on attachment 323073 [details]
Combined Patch

r=me
Comment 35 BJ Burg 2017-10-06 20:04:47 PDT 
Mac-32bit needs  && WK_API_ENABLED in some places, will upload another combined patch.
Comment 36 BJ Burg 2017-10-06 20:07:19 PDT 
Created attachment 323080 [details]
Combined Patch
Comment 37 BJ Burg 2017-10-09 12:36:09 PDT 
Created attachment 323198 [details]
For EWS