Bug 200109 - Web Inspector: Page: don't allow the domain to be disabled
Summary: Web Inspector: Page: don't allow the domain to be disabled
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 200110 200587
  Show dependency treegraph
 
Reported: 2019-07-24 20:42 PDT by Devin Rousso
Modified: 2019-08-17 00:11 PDT (History)
14 users (show)

See Also:


Attachments
Patch (28.14 KB, patch)
2019-07-24 20:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.23 KB, patch)
2019-07-24 21:15 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.55 KB, patch)
2019-07-24 21:21 PDT, Devin Rousso
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (367.75 KB, application/zip)
2019-07-24 22:05 PDT, EWS Watchlist
no flags Details
Patch (19.13 KB, patch)
2019-07-24 22:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.27 MB, application/zip)
2019-07-24 23:16 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (1.43 MB, application/zip)
2019-07-24 23:31 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews102 for mac-highsierra (2.20 MB, application/zip)
2019-07-24 23:33 PDT, EWS Watchlist
no flags Details
Patch (31.69 KB, patch)
2019-08-01 16:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (3.29 MB, application/zip)
2019-08-01 19:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews102 for mac-highsierra (3.62 MB, application/zip)
2019-08-01 20:39 PDT, EWS Watchlist
no flags Details
Patch (31.35 KB, patch)
2019-08-02 01:15 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (31.18 KB, patch)
2019-08-08 16:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-24 20:42:34 PDT
.
Comment 1 Devin Rousso 2019-07-24 20:59:53 PDT
Created attachment 374861 [details]
Patch
Comment 2 EWS Watchlist 2019-07-24 21:02:14 PDT Comment hidden (obsolete)
Comment 3 Devin Rousso 2019-07-24 21:15:51 PDT
Created attachment 374865 [details]
Patch
Comment 4 Devin Rousso 2019-07-24 21:21:55 PDT
Created attachment 374867 [details]
Patch
Comment 5 EWS Watchlist 2019-07-24 22:05:13 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-07-24 22:05:15 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 2019-07-24 22:12:53 PDT
Created attachment 374873 [details]
Patch
Comment 8 EWS Watchlist 2019-07-24 23:16:35 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-07-24 23:16:36 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-07-24 23:31:06 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-07-24 23:31:08 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-07-24 23:33:09 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-07-24 23:33:11 PDT Comment hidden (obsolete)
Comment 14 Joseph Pecoraro 2019-07-29 12:04:21 PDT
Comment on attachment 374873 [details]
Patch

This potentially changes the order in which the frontend receives events. Previously they would come as part of `Page.enable` and now they are just sent immediately when a frontend connects.

I don't think this should cause problems, the frontend should have been able to setup domain observers before it lets backend messages in:

    // Signal that the frontend is now ready to receive messages.
    WI.whenTargetsAvailable().then(() => {
        InspectorFrontendAPI.loadCompleted();
    });

But I can't explain why so many tests are failing with lines like:

    CONSOLE MESSAGE: line 1: ReferenceError: Can't find variable: InspectorFrontendAPI
    ...

There are also many cases of tests directly performing this that would need to be updated:

    page/media-query-list-listener-exception.html
    13:    InspectorProtocol.sendCommand("Page.enable", {});

    page/frameScheduledNavigation.html
    16:    InspectorProtocol.sendCommand("Page.enable", {});

    page/frameScheduledNavigation-async-delegates.html
    19:    InspectorProtocol.sendCommand("Page.enable", {});

    page/archive.html
    7:    InspectorProtocol.sendCommand("Page.enable", {});

    page/frameStartedLoading.html
    17:    InspectorProtocol.sendCommand("Page.enable", {});

    timeline/line-column.html
    39:    InspectorProtocol.sendCommand("Page.enable");

A smaller step forward might be just removing `disable`. Almost this entire patch could be reused.
Comment 15 Joseph Pecoraro 2019-07-29 12:45:40 PDT
Also note that ITMLKit would need to rework their code now that `Page.enable` no longer exists.
Comment 16 Devin Rousso 2019-08-01 16:42:50 PDT
Created attachment 375365 [details]
Patch
Comment 17 EWS Watchlist 2019-08-01 19:42:20 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-08-01 19:42:22 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-08-01 20:39:50 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-08-01 20:39:51 PDT Comment hidden (obsolete)
Comment 21 Devin Rousso 2019-08-02 01:15:00 PDT
Created attachment 375399 [details]
Patch

The tests were failing because `WebCore::ScriptController::executeScript` doesn't actually do anything if it's paused, whereas `WebCore::ScriptController::evaluate` does.
Comment 22 BJ Burg 2019-08-08 11:08:06 PDT
Comment on attachment 375399 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:79
> +            // COMPATIBILITY (iOS 13): Page.enable was removed.

Should this say iOS 13 or 13.1?
Comment 23 Devin Rousso 2019-08-08 11:14:28 PDT
Comment on attachment 375399 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:79
>> +            // COMPATIBILITY (iOS 13): Page.enable was removed.
> 
> Should this say iOS 13 or 13.1?

From what I've come to understand, we put the last iOS version that did NOT support this change, meaning that if we drop support for it, we could remove the check.
Comment 24 WebKit Commit Bot 2019-08-08 11:59:47 PDT Comment hidden (obsolete)
Comment 25 Devin Rousso 2019-08-08 16:18:37 PDT
Created attachment 375854 [details]
Patch
Comment 26 WebKit Commit Bot 2019-08-08 18:10:47 PDT
Comment on attachment 375854 [details]
Patch

Clearing flags on attachment: 375854

Committed r248454: <https://trac.webkit.org/changeset/248454>
Comment 27 WebKit Commit Bot 2019-08-08 18:10:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2019-08-08 18:11:22 PDT
<rdar://problem/54105611>
Comment 29 Truitt Savell 2019-08-09 11:49:20 PDT
It looks like the changes in https://trac.webkit.org/changeset/248454/webkit

broke inspector/css/force-page-appearance.html on Debug and release wk1 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fforce-page-appearance.html

I was able to reproduce this locally by running the test on 248454 and it fails, on 248452 is passes. 

Can this be looked at today?
Comment 30 Devin Rousso 2019-08-09 13:07:20 PDT
(In reply to Truitt Savell from comment #29)
> It looks like the changes in https://trac.webkit.org/changeset/248454/webkit
> 
> broke inspector/css/force-page-appearance.html on Debug and release wk1 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fcss%2Fforce-page-appearance.html
> 
> I was able to reproduce this locally by running the test on 248454 and it
> fails, on 248452 is passes. 
> 
> Can this be looked at today?
Looking now.
Comment 31 Devin Rousso 2019-08-09 14:15:07 PDT
(In reply to Devin Rousso from comment #30)
> (In reply to Truitt Savell from comment #29)
> > It looks like the changes in https://trac.webkit.org/changeset/248454/webkit
> > 
> > broke inspector/css/force-page-appearance.html on Debug and release wk1 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=inspector%2Fcss%2Fforce-page-appearance.html
> > 
> > I was able to reproduce this locally by running the test on 248454 and it fails, on 248452 is passes. 
> > 
> > Can this be looked at today?
> Looking now.
<https://webkit.org/b/200587>