Bug 209940

Summary: Web Inspector: increase the auto-inspect debugger timeout delay to account for slower networks/devices
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Devin Rousso
Reported 2020-04-02 16:49:11 PDT
Right now, when auto-inspecting, we only pause for a maximum of 1s which may not be enough time for messages from the frontend to be received, especially if there are a lot of things that need to be done (e.g. loading breakpoints from `indexedDB`).
Attachments
Patch (3.30 KB, patch)
2020-04-02 16:50 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (3.19 KB, patch)
2020-04-03 11:08 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2020-04-02 16:49:23 PDT
<rdar://problem/61120919>
Devin Rousso
Comment 2 2020-04-02 16:50:34 PDT
Created attachment 395322 [details] Patch
Joseph Pecoraro
Comment 3 2020-04-02 19:17:03 PDT
Comment on attachment 395322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395322&action=review r=me > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:160 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ Would still be good to pull this out as a constant so its easier to read.
Devin Rousso
Comment 4 2020-04-03 11:08:29 PDT
Created attachment 395397 [details] Patch
EWS
Comment 5 2020-04-03 11:30:36 PDT
Committed r259479: <https://trac.webkit.org/changeset/259479> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395397 [details].
Blaze Burg
Comment 6 2020-04-03 11:50:25 PDT
Comment on attachment 395397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395397&action=review > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:-145 > - LOG_ERROR("Skipping Automatic Inspection Candidate with pageId(%u) because we are already paused waiting for pageId(%u)", targetIdentifier, m_automaticInspectionCandidateTargetIdentifier); I don't think this change is correct. Why WTFLogAlways? We have RELEASE_LOG_ERROR and RELEASE_LOG_ERROR_IF, which should do the same thing but go through os_log and won't get lost in stderr/stdout.
Devin Rousso
Comment 7 2020-04-03 12:14:01 PDT
Comment on attachment 395397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395397&action=review >> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:-145 >> - LOG_ERROR("Skipping Automatic Inspection Candidate with pageId(%u) because we are already paused waiting for pageId(%u)", targetIdentifier, m_automaticInspectionCandidateTargetIdentifier); > > I don't think this change is correct. Why WTFLogAlways? We have RELEASE_LOG_ERROR and RELEASE_LOG_ERROR_IF, which should do the same thing but go through os_log and won't get lost in stderr/stdout. JSC doesn't have any of the necessary things to make `RELEASE_LOG_ERROR` work, namely a `LOG_CHANNEL_PREFIX` and any logging channels. I suppose I could make one, but that seemed like a lot of overkill for something that should hopefully not happen.
Blaze Burg
Comment 8 2020-04-03 14:39:48 PDT
Comment on attachment 395397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395397&action=review >>> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:-145 >>> - LOG_ERROR("Skipping Automatic Inspection Candidate with pageId(%u) because we are already paused waiting for pageId(%u)", targetIdentifier, m_automaticInspectionCandidateTargetIdentifier); >> >> I don't think this change is correct. Why WTFLogAlways? We have RELEASE_LOG_ERROR and RELEASE_LOG_ERROR_IF, which should do the same thing but go through os_log and won't get lost in stderr/stdout. > > JSC doesn't have any of the necessary things to make `RELEASE_LOG_ERROR` work, namely a `LOG_CHANNEL_PREFIX` and any logging channels. I suppose I could make one, but that seemed like a lot of overkill for something that should hopefully not happen. Oh, forgot about that. Carry on!
Note You need to log in before you can comment on or make changes to this bug.