Issue 404953004: DevTools: Refactor linkifyRawLocation to use fallback url

Issue 404953004: DevTools: Refactor linkifyRawLocation to use fallback url (Closed)

Created:
6 years, 5 months ago by sergeyv

Modified:
6 years, 4 months ago

Reviewers:
vsevik, apavlov, aandrey, pfeldman

CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

DevTools: Refactor linkifyRawLocation to use fallback url BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179235

Patch Set 1 #

Total comments: 5

Patch Set 2 : Address vsevik's comments #

Patch Set 3 : Rebase on master #

Patch Set 4 : Rebase on master #

Total comments: 2

Patch Set 5 : Address aandrey's comments #

Total comments: 10

Patch Set 6 : Address aandrey's comments #2 #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+83 lines, -88 lines)			Patch
M	LayoutTests/inspector-enabled/sources/debugger/linkifier.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/inspector/console/console-error-on-call-frame-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/inspector/extensions/extensions-console-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/inspector/jump-to-previous-editing-location.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/components/ObjectPopoverHelper.js	View	1	1 chunk	+4 lines, -6 lines	0 comments	Download
M	Source/devtools/front_end/console/ConsoleViewMessage.js	View	1 2 3 4	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/devtools/front_end/elements/EventListenersSidebarPane.js	View	1	1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/devtools/front_end/network/NetworkPanel.js	View	1 2 3 4 5	2 chunks	+2 lines, -3 lines	0 comments	Download
M	Source/devtools/front_end/profiler/CPUProfileDataGrid.js	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/profiler/CanvasProfileView.js	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/profiler/HeapSnapshotGridNodes.js	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/profiler/HeapSnapshotView.js	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/sdk/DOMModel.js	View	1 2 3	2 chunks	+14 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/sdk/DebuggerModel.js	View	1 2 3 4	4 chunks	+5 lines, -16 lines	0 comments	Download
M	Source/devtools/front_end/sdk/InspectorBackend.js	View		1 chunk	+8 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/sdk/Linkifier.js	View	1 2 3 4 5	2 chunks	+21 lines, -27 lines	0 comments	Download
M	Source/devtools/front_end/sdk/Target.js	View	1 2	1 chunk	+8 lines, -0 lines	0 comments	Download
M	Source/devtools/front_end/timeline/TimelineUIUtils.js	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/devtools/front_end/timeline/TimelineUIUtilsImpl.js	View	1 2 3 4 5	2 chunks	+3 lines, -11 lines	0 comments	Download
M	Source/devtools/front_end/timeline/TracingTimelineUIUtils.js	View	1 2 3 4 5	2 chunks	+2 lines, -10 lines	0 comments	Download

Messages

Total messages: 30 (0 generated)

Expand Messages | Collapse Messages

apavlov

https://codereview.chromium.org/404953004/diff/1/Source/devtools/front_end/sdk/Target.js File Source/devtools/front_end/sdk/Target.js (right): https://codereview.chromium.org/404953004/diff/1/Source/devtools/front_end/sdk/Target.js#newcode218 Source/devtools/front_end/sdk/Target.js:218: isDead: function() I'd advise against using terms having strongly ...

6 years, 5 months ago (2014-07-20 09:50:36 UTC) #2

vsevik

> Get rid of linkifyRawLocation method Why not keep it as a helper shortcut?

6 years, 5 months ago (2014-07-21 06:54:39 UTC) #3

vsevik

https://codereview.chromium.org/404953004/diff/1/Source/devtools/front_end/sdk/Linkifier.js File Source/devtools/front_end/sdk/Linkifier.js (right): https://codereview.chromium.org/404953004/diff/1/Source/devtools/front_end/sdk/Linkifier.js#newcode174 Source/devtools/front_end/sdk/Linkifier.js:174: linkifyLocation: function(target, sourceURL, lineNumber, columnNumber, classes) This method seems ...

6 years, 5 months ago (2014-07-21 06:54:46 UTC) #4

vsevik

https://chromiumcodereview.appspot.com/404953004/diff/1/Source/devtools/front_end/sdk/Linkifier.js File Source/devtools/front_end/sdk/Linkifier.js (left): https://chromiumcodereview.appspot.com/404953004/diff/1/Source/devtools/front_end/sdk/Linkifier.js#oldcode186 Source/devtools/front_end/sdk/Linkifier.js:186: linkifyRawLocation: function(rawLocation, classes) Could we keep this method and ...

6 years, 5 months ago (2014-07-21 07:24:28 UTC) #5

sergeyv

6 years, 5 months ago (2014-07-21 12:06:54 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/404953004/20001

6 years, 5 months ago (2014-07-22 12:37:40 UTC) #9

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 5 months ago (2014-07-22 12:37:51 UTC) #10

commit-bot: I haz the power

Failed to apply patch for Source/devtools/front_end/sdk/Linkifier.js: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 5 months ago (2014-07-22 12:37:52 UTC) #11

Failed to apply patch for Source/devtools/front_end/sdk/Linkifier.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/devtools/front_end/sdk/Linkifier.js
  Hunk #1 FAILED at 142.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/devtools/front_end/sdk/Linkifier.js.rej

Patch:       Source/devtools/front_end/sdk/Linkifier.js
Index: Source/devtools/front_end/sdk/Linkifier.js
diff --git a/Source/devtools/front_end/sdk/Linkifier.js
b/Source/devtools/front_end/sdk/Linkifier.js
index
9e25ecc72ac3958e854af0a702440325c586047a..f01b0ee9751e2249f204980b75cff536c83b749d
100644
--- a/Source/devtools/front_end/sdk/Linkifier.js
+++ b/Source/devtools/front_end/sdk/Linkifier.js
@@ -142,57 +142,36 @@ WebInspector.Linkifier.prototype = {
 
     /**
      * @param {?WebInspector.Target} target
-     * @param {string} scriptId
+     * @param {?string} scriptId
      * @param {string} sourceURL
      * @param {number} lineNumber
      * @param {number=} columnNumber
      * @param {string=} classes
-     * @return {?Element}
+     * @return {!Element}
      */
-    linkifyLocationByScriptId: function(target, scriptId, sourceURL,
lineNumber, columnNumber, classes)
+    linkifyScriptLocation: function(target, scriptId, sourceURL, lineNumber,
columnNumber, classes)
     {
-        var rawLocation = target ?
target.debuggerModel.createRawLocationByScriptId(scriptId, sourceURL,
lineNumber, columnNumber || 0) : null;
+        var rawLocation = target && !target.isDetached() ?
target.debuggerModel.createRawLocationByScriptId(scriptId, sourceURL,
lineNumber, columnNumber || 0) : null;
         var fallbackAnchor = WebInspector.linkifyResourceAsNode(sourceURL,
lineNumber, classes);
         if (!rawLocation)
             return fallbackAnchor;
 
-        var anchor = this.linkifyRawLocation(rawLocation, classes);
+        var anchor = this._createAnchor(classes);
+        var liveLocation =
rawLocation.createLiveLocation(this._updateAnchor.bind(this, anchor));
+        this._liveLocationsByTarget.get(rawLocation.target()).push({anchor:
anchor, location: liveLocation});
         anchor.__fallbackAnchor = fallbackAnchor;
         return anchor;
-
-    },
-
-    /**
-     * @param {!WebInspector.Target} target
-     * @param {string} sourceURL
-     * @param {number} lineNumber
-     * @param {number=} columnNumber
-     * @param {string=} classes
-     * @return {?Element}
-     */
-    linkifyLocation: function(target, sourceURL, lineNumber, columnNumber,
classes)
-    {
-        var rawLocation =
target.debuggerModel.createRawLocationByURL(sourceURL, lineNumber, columnNumber
|| 0);
-        if (!rawLocation)
-            return WebInspector.linkifyResourceAsNode(sourceURL, lineNumber,
classes);
-        return this.linkifyRawLocation(rawLocation, classes);
     },
 
     /**
      * @param {!WebInspector.DebuggerModel.Location} rawLocation
+     * @param {string} fallbackUrl
      * @param {string=} classes
-     * @return {?Element}
+     * @return {!Element}
      */
-    linkifyRawLocation: function(rawLocation, classes)
+    linkifyRawLocation: function(rawLocation, fallbackUrl, classes)
     {
-        // FIXME: this check should not be here.
-        var script =
rawLocation.target().debuggerModel.scriptForId(rawLocation.scriptId);
-        if (!script)
-            return null;
-        var anchor = this._createAnchor(classes);
-        var liveLocation =
rawLocation.createLiveLocation(this._updateAnchor.bind(this, anchor));
-        this._liveLocationsByTarget.get(rawLocation.target()).push({anchor:
anchor, location: liveLocation});
-        return anchor;
+        return this.linkifyScriptLocation(rawLocation.target(),
rawLocation.scriptId, fallbackUrl, rawLocation.lineNumber,
rawLocation.columnNumber, classes);
     },
 
     /**

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/404953004/40001

6 years, 4 months ago (2014-07-29 11:36:49 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 4 months ago (2014-07-29 11:37:02 UTC) #14

commit-bot: I haz the power

Failed to apply patch for Source/devtools/front_end/network/NetworkPanel.js: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 4 months ago (2014-07-29 11:37:03 UTC) #15

Failed to apply patch for Source/devtools/front_end/network/NetworkPanel.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/devtools/front_end/network/NetworkPanel.js
  Hunk #1 FAILED at 1039.
  Hunk #2 succeeded at 2774 (offset 34 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/devtools/front_end/network/NetworkPanel.js.rej

Patch:       Source/devtools/front_end/network/NetworkPanel.js
Index: Source/devtools/front_end/network/NetworkPanel.js
diff --git a/Source/devtools/front_end/network/NetworkPanel.js
b/Source/devtools/front_end/network/NetworkPanel.js
index
e2b30a94c6bdda99474559e4025dcf66b32b2541..212c101fea9b4631b79e8fa9803de26211c51d36
100644
--- a/Source/devtools/front_end/network/NetworkPanel.js
+++ b/Source/devtools/front_end/network/NetworkPanel.js
@@ -1039,7 +1039,7 @@ WebInspector.NetworkLogView.prototype = {
             var row = document.createElement("tr");
             row.createChild("td").textContent = stackFrame.functionName ||
WebInspector.UIString("(anonymous function)");
             row.createChild("td").textContent = " @ ";
-           
row.createChild("td").appendChild(this._linkifier.linkifyRawLocation(rawLocation));
+           
row.createChild("td").appendChild(this._linkifier.linkifyRawLocation(rawLocation,
stackFrame.url));
             framesTable.appendChild(row);
         }
         return framesTable;
@@ -2740,7 +2740,7 @@ WebInspector.NetworkDataGridNode.prototype = {
 
         case WebInspector.NetworkRequest.InitiatorType.Script:
             if (!this._linkifiedInitiatorAnchor) {
-                this._linkifiedInitiatorAnchor =
this._linkifier.linkifyLocation(request.target(), initiator.url,
initiator.lineNumber - 1, initiator.columnNumber - 1);
+                this._linkifiedInitiatorAnchor =
this._linkifier.linkifyScriptLocation(request.target(), null, initiator.url,
initiator.lineNumber - 1, initiator.columnNumber - 1);
                 this._linkifiedInitiatorAnchor.title = "";
             }
             cell.appendChild(this._linkifiedInitiatorAnchor);

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/404953004/60001

6 years, 4 months ago (2014-07-29 16:26:38 UTC) #17

aandrey

https://codereview.chromium.org/404953004/diff/60001/Source/devtools/front_end/console/ConsoleViewMessage.js File Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/404953004/diff/60001/Source/devtools/front_end/console/ConsoleViewMessage.js#newcode283 Source/devtools/front_end/console/ConsoleViewMessage.js:283: // FIXME(62725): stack trace line/column numbers are one-based. I ...

6 years, 4 months ago (2014-07-29 16:50:22 UTC) #19

aandrey

On 2014/07/29 16:50:22, aandrey wrote: > https://codereview.chromium.org/404953004/diff/60001/Source/devtools/front_end/console/ConsoleViewMessage.js > File Source/devtools/front_end/console/ConsoleViewMessage.js (right): > > https://codereview.chromium.org/404953004/diff/60001/Source/devtools/front_end/console/ConsoleViewMessage.js#newcode283 > ...

6 years, 4 months ago (2014-07-29 16:53:32 UTC) #20

sergeyv

6 years, 4 months ago (2014-07-30 09:31:13 UTC) #21

aandrey

lgtm https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/components/ObjectPopoverHelper.js File Source/devtools/front_end/components/ObjectPopoverHelper.js (right): https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/components/ObjectPopoverHelper.js#newcode88 Source/devtools/front_end/components/ObjectPopoverHelper.js:88: title.appendChild(link); if (link) https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/network/NetworkPanel.js File Source/devtools/front_end/network/NetworkPanel.js (left): https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/network/NetworkPanel.js#oldcode1038 ...

6 years, 4 months ago (2014-07-30 10:01:24 UTC) #22

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/404953004/80001

6 years, 4 months ago (2014-07-30 10:12:19 UTC) #24

aandrey

On 2014/07/30 10:12:19, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...

6 years, 4 months ago (2014-07-30 10:18:38 UTC) #26

sergeyv

https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/components/ObjectPopoverHelper.js File Source/devtools/front_end/components/ObjectPopoverHelper.js (right): https://codereview.chromium.org/404953004/diff/80001/Source/devtools/front_end/components/ObjectPopoverHelper.js#newcode88 Source/devtools/front_end/components/ObjectPopoverHelper.js:88: title.appendChild(link); On 2014/07/30 14:01:23, aandrey wrote: > if (link) ...

6 years, 4 months ago (2014-07-30 13:43:01 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/404953004/100001

6 years, 4 months ago (2014-07-30 13:44:36 UTC) #29

Message was sent while issue was closed.

Change committed as 179235

Expand Messages | Collapse Messages