Reproducible on the image resource load records... It sounds like we are trying to show popup from popup. We should include image preview in the original timeline popup instead.
Created attachment 158815 [details] [IMAGE] Screenshot with the patch applied
Created attachment 158819 [details] Patch
Comment on attachment 158819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158819&action=review > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:739 > + _buildImagePreviewElementAndContinue: function(callback) > + { > + WebInspector.buildImagePreviewContents(null, null, this.url, false, returnContent); > + > + function returnContent(contentElement) > + { > + callback(contentElement); > + } > }, I'd replace a call to this with just WebInspector.buildImagePreviewContents(null, null, this.url, false, callback);
Created attachment 158825 [details] Patch
Comment on attachment 158825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158825&action=review > Source/WebCore/inspector/front-end/UIUtils.js:1100 > + if (naturalWidth > naturalHeight) { > + if (naturalWidth > maxImageWidth) { > + imageElement.style.width = maxImageWidth + "px"; > + imageElement.style.height = (naturalHeight * maxImageWidth / naturalWidth) + "px"; > + } > + } else { > + if (naturalHeight > maxImageHeight) { > + imageElement.style.width = (naturalWidth * maxImageHeight / naturalHeight) + "px"; > + imageElement.style.height = maxImageHeight + "px"; > + } > + } Can we do this with CSS, using max-width & max-height instead?
Created attachment 158829 [details] Patch
Comment on attachment 158829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158829&action=review > Source/WebCore/inspector/front-end/ElementsPanel.js:434 > + var listItem = relatedObject.enclosingNodeOrSelfWithNodeNameInArray(["li"]); enclosingNodeOrSelfWithNodeName()? > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:641 > + generatePopupContent: function(callback) > + { > + WebInspector.buildImagePreviewContents(null, null, this.url, false, this._generatePopupContentWithImagePreview.bind(this, callback)); > + }, I think we should only do this for the record types where we expect images and where URL is valid (i.e. network events etc.) > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:643 > + _generatePopupContentWithImagePreview: function(callback, previewElement) Please annotate. > Source/WebCore/inspector/front-end/UIUtils.js:1041 > +WebInspector.buildImagePreviewContents = function(relatedObject, dimensionsProvider, imageURL, showDimensions, userCallback) Please annotate. I'd suggest we also drop showDimensions parameter and use presence of dimenstionsProvider instead. Actually, given that we invoke dimensionsProvider() first thing, I'd suggest we leave this to the caller and let him invoke this with the dimensions, provided caller has them -- this would simplify control flow a bit in my view.
Comment on attachment 158829 [details] Patch r- as per caseq's recommendations.
Created attachment 158838 [details] Patch
Comment on attachment 158838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158838&action=review > Source/WebCore/inspector/front-end/ElementsPanel.js:434 > + WebInspector.buildImagePreviewContents.bind(WebInspector, anchor.href, true, showPopover); Is this intentional? > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:877 > + this.record = record; We also use PopupContentHelper for frames that are not records, so it should not be aware of the record. Can we have this logic elsewhere? > Source/WebCore/inspector/front-end/UIUtils.js:1047 > + * @param {Object=} precomputedDimensions > + */ > +WebInspector.buildImagePreviewContents = function(imageURL, showDimensions, userCallback, precomputedDimensions) Drop precomputed, just dimensions -- it's cleaner. > Source/WebCore/inspector/front-end/UIUtils.js:1068 > + imageElement.addEventListener("load", buildContent.bind(null, imageElement, precomputedDimensions), false); > + imageElement.addEventListener("error", buildContent.bind(null, null, null), false); > + resource.populateImageSource(imageElement); > + > + /** > + * @param {Object=} nodeDimensions > + */ > + function buildContent(imageElement, nodeDimensions) > + { > + if (!imageElement) { > + userCallback(); > + return; > + } So we only invoke buildContent() with a null imageElement if we had an error -- I'd suggest we directly invoke userCallback on error instead and remove this check. We could also remove both parameters, as these would always be visible in the outer lexical context. Hence we would not have to bind() as well.
Created attachment 159086 [details] Patch
(In reply to comment #10) > (From update of attachment 158838 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158838&action=review > > > Source/WebCore/inspector/front-end/ElementsPanel.js:434 > > + WebInspector.buildImagePreviewContents.bind(WebInspector, anchor.href, true, showPopover); > > Is this intentional? It's an unfortunate copy-paste... > > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:877 > > + this.record = record; > > We also use PopupContentHelper for frames that are not records, so it should not be aware of the record. Can we have this logic elsewhere? Fixed the way we had agreed offline. > > Source/WebCore/inspector/front-end/UIUtils.js:1047 > > + * @param {Object=} precomputedDimensions > > + */ > > +WebInspector.buildImagePreviewContents = function(imageURL, showDimensions, userCallback, precomputedDimensions) > > Drop precomputed, just dimensions -- it's cleaner. As agreed offline, this will make the reader think that the "showDimensions" is related to this parameter (i.e. the former can be superfluous if the latter is null), while this is not the case. > > Source/WebCore/inspector/front-end/UIUtils.js:1068 > > + imageElement.addEventListener("load", buildContent.bind(null, imageElement, precomputedDimensions), false); > > + imageElement.addEventListener("error", buildContent.bind(null, null, null), false); > > + resource.populateImageSource(imageElement); > > + > > + /** > > + * @param {Object=} nodeDimensions > > + */ > > + function buildContent(imageElement, nodeDimensions) > > + { > > + if (!imageElement) { > > + userCallback(); > > + return; > > + } > > So we only invoke buildContent() with a null imageElement if we had an error -- I'd suggest we directly invoke userCallback on error instead and remove this check. We could also remove both parameters, as these would always be visible in the outer lexical context. Hence we would not have to bind() as well. This will result in the userCalback() invocation with the ErrorEvent object as the first argument, which is wrong. Fixed by using an intermediate callback.
Comment on attachment 159086 [details] Patch LGTM
Committed r125882: <http://trac.webkit.org/changeset/125882>