Bug 146498 - Web Inspector: Option+Click not jumping to resource
Summary: Web Inspector: Option+Click not jumping to resource
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-30 23:58 PDT by Joseph Pecoraro
Modified: 2015-07-07 10:26 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.33 KB, patch)
2015-07-06 20:01 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (12.02 KB, patch)
2015-07-07 01:21 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 Joseph Pecoraro 2015-06-30 23:58:34 PDT
* SUMMARY
Option+Click not jumping to resource.

* STEPS TO REPRODUCE
1. Inspect <body> on <http://bogojoker.com>
2. In the Styles sidebar, Option+Click anything in the CSS (or the pattern.png resource)
  => no jump to resource
Comment 1 Radar WebKit Bug Importer 2015-06-30 23:59:04 PDT
<rdar://problem/21627454>
Comment 2 Devin Rousso 2015-07-06 20:01:27 PDT
Created attachment 256271 [details]
Patch
Comment 3 Timothy Hatcher 2015-07-06 21:07:21 PDT
Comment on attachment 256271 [details]
Patch

The force tab stuff was added because I was seeing tab switches during cookie restore. Maybe we don't need it now because cookie restore is only done when the tab is selected?
Comment 4 Joseph Pecoraro 2015-07-06 22:50:00 PDT
Comment on attachment 256271 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:355
> +                    if (this._delegate && typeof this._delegate.tokenTrackingControllerHighlightedRangeWasClicked === "function")
> +                        this._delegate.tokenTrackingControllerHighlightedRangeWasClicked(this);

This is not commented in the ChangeLog. We couldn't determine why the setTimeout was needed... but it looks like it was there to deal with CodeMirror. In any case, if you didn't see a regression with this change it seems find to make.
Comment 5 Joseph Pecoraro 2015-07-06 22:59:42 PDT
Comment on attachment 256271 [details]
Patch

Doing some quick searches I found a few call sites that were missed. (Maybe you just searched WebInspector.foo and missed these which don't 100% match that call site signature):

Base/Main.js
1535:    this.showMainFrameDOMTree(event.data.node, true);

Base/Main.js
1138:        this.showSourceCodeForFrame(frame.id, true);

Thanks for looking into this!
Comment 6 Joseph Pecoraro 2015-07-06 23:05:03 PDT
(In reply to comment #3)
> Comment on attachment 256271 [details]
> Patch
> 
> The force tab stuff was added because I was seeing tab switches during
> cookie restore. Maybe we don't need it now because cookie restore is only
> done when the tab is selected?

That was exactly what I was thinking and why I suggested this approach to Devin. I cannot think of any case where we don't want to switch to a relevant tab when trying to show a new content view. The idea with window.event was clever, but I can't think of cases where we would trigger switching content views that wasn't somehow user gesture triggered. Even if we end up needing to keep/restore this parameter, I'd rather see it negated, and have the default behavior allow for switching tabs.
Comment 7 Devin Rousso 2015-07-07 01:21:10 PDT
Created attachment 256290 [details]
Patch
Comment 8 WebKit Commit Bot 2015-07-07 10:26:52 PDT
Comment on attachment 256290 [details]
Patch

Clearing flags on attachment: 256290

Committed r186466: <http://trac.webkit.org/changeset/186466>
Comment 9 WebKit Commit Bot 2015-07-07 10:26:57 PDT
All reviewed patches have been landed.  Closing bug.