Bug 128092 - Web Inspector: RuntimeManager should not use view object WebInspector.quickConsole
Summary: Web Inspector: RuntimeManager should not use view object WebInspector.quickCo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-02 18:52 PST by Brian Burg
Modified: 2016-06-27 17:23 PDT (History)
8 users (show)

See Also:


Attachments
patch (2.23 KB, patch)
2014-02-19 23:31 PST, Gergő Balogh
timothy: review-
Details | Formatted Diff | Diff
proposed patch (1.68 KB, patch)
2014-03-26 05:09 PDT, Gergő Balogh
no flags Details | Formatted Diff | Diff
Proposed Fix (15.67 KB, patch)
2016-06-27 16:39 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-02-02 18:52:19 PST
This seems to be a straightforward model/view layering violation. The "active" context identifier should live in the RuntimeManager instead.

With this fixed, we will be able to use RuntimeAgent.evaluateInInspectedWindow() within inspector-protocol tests that test the model, instead of blitzing raw "RuntimeAgent.evaluate" messages.
Comment 1 Radar WebKit Bug Importer 2014-02-02 18:52:33 PST
<rdar://problem/15966526>
Comment 2 Gergő Balogh 2014-02-19 23:31:07 PST
Created attachment 224720 [details]
patch
Comment 3 Timothy Hatcher 2014-02-20 08:35:20 PST
Comment on attachment 224720 [details]
patch

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

> Source/WebInspectorUI/UserInterface/RuntimeManager.js:76
> +        if (!contextId) {
> +            contextId = WebInspector.quickConsole.executionContextIdentifier;
> +        }

Passing in the contextId (preferably contextIdentifier) is the right approach.

The idea of this bug was to remove the use of WebInspector.quickConsole altogether, not just hide it behind an optional argument.

I also don't see where anyone is passing a context to evaluateInInspectedWindow in this patch, so nothing has really changed.
Comment 4 Brian Burg 2014-02-20 09:01:59 PST
I think the correct fix here is to store/load the last user-selected execution context on the WebInspector.Frame. The frame already stores WebInspector.ExecutionContext's and WebInspector.ExecutionContextList.
Comment 5 Timothy Hatcher 2014-02-20 09:09:30 PST
Yeah, that sounds good to me.
Comment 6 Gergő Balogh 2014-03-26 05:09:42 PDT
Created attachment 227844 [details]
proposed patch

I can not find any case when the value of contextID in the original version is not undefined. Maybe I miss something, but I think this is a left over from other fixes. Based on these guess I simple remove it.
Comment 7 Timothy Hatcher 2014-03-26 07:51:15 PDT
comtextId is pickable from a popup menu in the console input area when there are Safari extensions or frames on the page. Removing this line breaks that feature.
Comment 8 Brian Burg 2016-06-27 16:39:19 PDT
Created attachment 282186 [details]
Proposed Fix
Comment 9 Joseph Pecoraro 2016-06-27 17:22:12 PDT
Comment on attachment 282186 [details]
Proposed Fix

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

> LayoutTests/inspector/runtime/change-execution-context-identifier-expected.txt:16
> +Passphrase in selected frame: rosewater
> +PASS: The passphrase should match the phrase defined in the subframe.

Style: InspectorTest.expectThat would output PASS: ...
Comment 10 WebKit Commit Bot 2016-06-27 17:23:28 PDT
Comment on attachment 282186 [details]
Proposed Fix

Clearing flags on attachment: 282186

Committed r202522: <http://trac.webkit.org/changeset/202522>
Comment 11 WebKit Commit Bot 2016-06-27 17:23:32 PDT
All reviewed patches have been landed.  Closing bug.