Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2205853005:
[Devtools] Regression fix for double toolbar for images in network (Closed)
|
Created:
4 years, 4 months ago by allada Modified:
4 years, 4 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Regression fix for double toolbar for images in network
Fixed minor regression caused by:
https://codereview.chromium.org/2204803002/
Where going from preview to response and back would produce more bottom
toolbars every time it was shown.
BUG=630729, 633903
Committed: https://crrev.com/2c1074256b6a37cdb124cd895cd3b0c8777150a0
Cr-Commit-Position: refs/heads/master@{#409868}
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix #
Total comments: 6
Patch Set 3 : [Devtools] Regression fix for double toolbar for images in network #Patch Set 4 : [Devtools] Regression fix for double toolbar for images in network #
Total comments: 6
Messages
Total messages: 20 (7 generated)
Description was changed from ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 ========== to ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 ==========
allada@chromium.org changed reviewers: + lushnikov@chromium.org
PTL
https://codereview.chromium.org/2205853005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:73: var isShowing = this._previewView.isShowing(); Why does this code run everytime you switch to the network panel? This doesn't look right.
https://codereview.chromium.org/2205853005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:61: this.innerView = this._previewView; this is not needed https://codereview.chromium.org/2205853005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:72: this._previewView = view; it looks to me that this._previewView is equal to this.innerView (and almost never used). Let's simplify this!
PTL
https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:51: } let's do a fast return here to reduce code nesting https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:54: this._emptyWidget.detach(); this.innerView = null; https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:58: if (!this.innerView) let's get rid of this.innerView!
PTL https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:51: } On 2016/08/03 18:42:15, lushnikov wrote: > let's do a fast return here to reduce code nesting Done. https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:54: this._emptyWidget.detach(); On 2016/08/03 18:42:14, lushnikov wrote: > this.innerView = null; Done. https://codereview.chromium.org/2205853005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:58: if (!this.innerView) On 2016/08/03 18:42:14, lushnikov wrote: > let's get rid of this.innerView! Done.
lgtm https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:52: this._previewView = this._emptyWidget; remove https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:59: this._previewView = null; remove https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:154: _createPreviewView: function(callback) let's rename this into _getPreviewView: it doesn't have a "create" semantics; instead, it sometimes fetches something.
https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js (right): https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:52: this._previewView = this._emptyWidget; On 2016/08/03 20:09:05, lushnikov wrote: > remove This is here because it tries to get the content and if it fails it will try to get the content again next call and uses an empty widget until it's ready. Example would be a websocket or a request that does not have any content yet show empty widget and try to get content again when able to. https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:59: this._previewView = null; On 2016/08/03 20:09:05, lushnikov wrote: > remove needed because of above. https://codereview.chromium.org/2205853005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestPreviewView.js:154: _createPreviewView: function(callback) On 2016/08/03 20:09:05, lushnikov wrote: > let's rename this into _getPreviewView: it doesn't have a "create" semantics; > instead, it sometimes fetches something. It is a wrapper for other create functions. This function is used to create whatever view needed based on the content.
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 ========== to ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 ========== to ========== [Devtools] Regression fix for double toolbar for images in network Fixed minor regression caused by: https://codereview.chromium.org/2204803002/ Where going from preview to response and back would produce more bottom toolbars every time it was shown. BUG=630729,633903 Committed: https://crrev.com/2c1074256b6a37cdb124cd895cd3b0c8777150a0 Cr-Commit-Position: refs/heads/master@{#409868} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2c1074256b6a37cdb124cd895cd3b0c8777150a0 Cr-Commit-Position: refs/heads/master@{#409868} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
