Bug 200109

Summary: Web Inspector: Page: don't allow the domain to be disabled
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, tsavell, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200854
Bug Depends on:    
Bug Blocks: 200110, 200587    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Patch
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews116 for mac-highsierra
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Patch
none
Patch none

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>