177661 – Web Inspector: consolidate code that hosts the Inspector page inside a WKWebView

RESOLVED FIXED 177661

Web Inspector: consolidate code that hosts the Inspector page inside a WKWebView

https://bugs.webkit.org/show_bug.cgi?id=177661

Summary Web Inspector: consolidate code that hosts the Inspector page inside a WKWebView

Blaze Burg

Reported 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.

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

Radar WebKit Bug Importer

Comment 1 2017-09-29 09:21:35 PDT

<rdar://problem/34740286>

Blaze Burg

Comment 2 2017-10-03 16:16:20 PDT

Created attachment 322604 [details] Combined Patch

Blaze Burg

Comment 3 2017-10-03 16:16:25 PDT

Created attachment 322605 [details] [1/6] Staged Patch

Blaze Burg

Comment 4 2017-10-03 16:16:29 PDT

Created attachment 322606 [details] [2/6] Staged Patch

Blaze Burg

Comment 5 2017-10-03 16:16:33 PDT

Created attachment 322607 [details] [3/6] Staged Patch

Blaze Burg

Comment 6 2017-10-03 16:16:41 PDT

Created attachment 322608 [details] [5/6] Staged Patch

Blaze Burg

Comment 7 2017-10-03 16:16:45 PDT

Created attachment 322609 [details] [6/6] Staged Patch

Blaze Burg

Comment 8 2017-10-03 16:16:50 PDT

Created attachment 322610 [details] [7/7] Staged Patch

Blaze Burg

Comment 9 2017-10-03 16:22:19 PDT

Created attachment 322612 [details] [4/6] Staged Patch webkit-patch didn't like this one, adding manually.

Blaze Burg

Comment 10 2017-10-03 16:22:57 PDT

I'll fix the WPE oopsie.

Joseph Pecoraro

Comment 11 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.

Joseph Pecoraro

Comment 12 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.

Blaze Burg

Comment 13 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.

Blaze Burg

Comment 14 2017-10-05 13:13:22 PDT

Created attachment 322890 [details] Combined Patch

Blaze Burg

Comment 15 2017-10-05 13:13:26 PDT

Created attachment 322891 [details] [1/5] Staged Patch

Blaze Burg

Comment 16 2017-10-05 13:13:30 PDT

Created attachment 322892 [details] [2/5] Staged Patch

Blaze Burg

Comment 17 2017-10-05 13:13:34 PDT

Created attachment 322893 [details] [3/5] Staged Patch

Blaze Burg

Comment 18 2017-10-05 13:13:38 PDT

Created attachment 322894 [details] [4/5] Staged Patch

Blaze Burg

Comment 19 2017-10-05 13:13:42 PDT

Created attachment 322895 [details] [5/5] Staged Patch

Blaze Burg

Comment 20 2017-10-05 13:15:25 PDT

Created attachment 322896 [details] Combined Patch

Blaze Burg

Comment 21 2017-10-05 13:15:30 PDT

Created attachment 322897 [details] [1/5] Staged Patch

Blaze Burg

Comment 22 2017-10-05 13:15:34 PDT

Created attachment 322898 [details] [2/5] Staged Patch

Blaze Burg

Comment 23 2017-10-05 13:15:38 PDT

Created attachment 322899 [details] [3/5] Staged Patch

Blaze Burg

Comment 24 2017-10-05 13:15:42 PDT

Created attachment 322900 [details] [4/5] Staged Patch

Blaze Burg

Comment 25 2017-10-05 13:15:46 PDT

Created attachment 322901 [details] [5/5] Staged Patch

Blaze Burg

Comment 26 2017-10-05 13:26:26 PDT

Created attachment 322905 [details] Combined Patch

Blaze Burg

Comment 27 2017-10-05 13:26:31 PDT

Created attachment 322906 [details] [1/5] Staged Patch

Blaze Burg

Comment 28 2017-10-05 13:26:35 PDT

Created attachment 322907 [details] [2/5] Staged Patch

Blaze Burg

Comment 29 2017-10-05 13:26:39 PDT

Created attachment 322908 [details] [3/5] Staged Patch

Blaze Burg

Comment 30 2017-10-05 13:26:43 PDT

Created attachment 322909 [details] [4/5] Staged Patch

Blaze Burg

Comment 31 2017-10-05 13:26:47 PDT

Created attachment 322910 [details] [5/5] Staged Patch

Blaze Burg

Comment 32 2017-10-05 13:42:56 PDT

I have rebased this several times and our bots are none the wiser.

Blaze Burg

Comment 33 2017-10-06 17:26:27 PDT

Created attachment 323073 [details] Combined Patch

Joseph Pecoraro

Comment 34 2017-10-06 17:36:36 PDT

Comment on attachment 323073 [details] Combined Patch r=me

Blaze Burg

Comment 35 2017-10-06 20:04:47 PDT

Mac-32bit needs && WK_API_ENABLED in some places, will upload another combined patch.

Blaze Burg

Comment 36 2017-10-06 20:07:19 PDT

Created attachment 323080 [details] Combined Patch

Blaze Burg

Comment 37 2017-10-09 12:36:09 PDT

Created attachment 323198 [details] For EWS

Blaze Burg

Comment 38 2017-10-20 10:31:26 PDT

Committed: https://trac.webkit.org/changeset/223770/webkit https://trac.webkit.org/changeset/223771/webkit https://trac.webkit.org/changeset/223772/webkit https://trac.webkit.org/changeset/223773/webkit https://trac.webkit.org/changeset/223774/webkit

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Blaze Burg

Reported

2017-09-29 09:21 PDT

Modified

2017-10-20 10:31 PDT History

CC List

6 users Show

URL

Keywords InRadar

Depends on

Blocks