Bug 21554 - Hovering the "src" attribute for an image should show the image dimensions
Summary: Hovering the "src" attribute for an image should show the image dimensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://screwlewse.com/?p=59
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-12 09:20 PDT by Timothy Hatcher
Modified: 2009-11-26 12:28 PST (History)
11 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2009-11-20 22:25 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (6.09 KB, patch)
2009-11-21 00:15 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with manual test (55.18 KB, patch)
2009-11-21 01:12 PST, Daniel Bates
timothy: review-
Details | Formatted Diff | Diff
Patch with manual test (56.32 KB, patch)
2009-11-25 13:33 PST, Daniel Bates
pfeldman: review-
Details | Formatted Diff | Diff
Patch with test case (57.69 KB, patch)
2009-11-25 17:35 PST, Daniel Bates
pfeldman: review-
Details | Formatted Diff | Diff
[IMAGE] Blank image of needed size for tests. (405 bytes, image/png)
2009-11-26 00:47 PST, Pavel Feldman
no flags Details
Patch with test case (10.91 KB, patch)
2009-11-26 01:11 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2008-10-12 09:20:51 PDT
From http://screwlewse.com/?p=59

"I want to see image dimensions when I hover over the markup instead of seeing the src, which I can see already right there in the markup."
Comment 1 Daniel Bates 2009-11-20 22:25:09 PST
Created attachment 43643 [details]
Patch
Comment 2 Daniel Bates 2009-11-20 22:28:53 PST
The patch I attached just shows the computed height and width on hover, but maybe we want to show the "natural" height and width of the image as well?
Comment 3 Daniel Bates 2009-11-21 00:15:31 PST
Created attachment 43645 [details]
Patch

Shows both the displayable and natural dimensions (if applicable) of the image when you hover over an images src.

Note, I only show the natural dimensions if they differ from the displayable dimensions.
Comment 4 Daniel Bates 2009-11-21 01:12:40 PST
Created attachment 43646 [details]
Patch with manual test

Swapped order of height x width shown in tooltip to be width x height so as to conform to the ordering seen in the title of the window when you open a single image in Safari.

Added a manual test case, /manual-tests/inspector/display-image-dimensions-on-hover-src.html
Comment 5 Timothy Hatcher 2009-11-22 08:29:31 PST
Comment on attachment 43646 [details]
Patch with manual test

> -        var title = this._nodeTitleInfo(this.representedObject, this.hasChildren, WebInspector.linkifyURL).title;
> +        var self = this;
> +        var callback = function(propertiesPayload) {
> +            var getPropertyValue = function(propertyName) {
> +                for (e in propertiesPayload)
> +                    if (propertiesPayload[e].name == propertyName)
> +                        return propertiesPayload[e].value.description;
> +                return null;
> +            };
> +            self._updateTitleWithRespectToNodeProperties(getPropertyValue);
> +        };
> +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);
> +        InjectedScriptAccess.getProperties(prototype, true, callback);

This is going to slow down the Elements panel, since it will delay updating the title waiting for the async InjectedScriptAccess.getProperties. (Slower in Chrome where the async is corss process. So we need to minimize these calls.)

We are paying a heavy cost of InjectedScriptAccess.getProperties for every DOM element just to filter it later for image elements. The InjectedScriptAccess.getProperties call was not designed for one-off property access, as you found, since it has .name properties. It is really only used for property inspection. I am not sure what the best solution is.

Also these should use named functions, with the brace o nthe next line like:

> +        function callback(propertiesPayload)
> +        {
> +            function getPropertyValue(propertyName)
> +            {

> +                            if (node.nodeName.toLowerCase() == "img") {

Use === in cases like this.

> +                                tooltipText = offsetWidth + "px x " + offsetHeight + "px";

This should use \u00d7 for the multiplication sign, not "x". I don't think "px" is needed twice. You could do "123x456 pixels" like Safari, but that will need to be localized with a WebInspector.UIString. Or just leave off the units.

> +                                if (offsetHeight != naturalHeight || offsetWidth != naturalWidth)

Use !==.

> +                                    tooltipText += " (natural: " + naturalWidth + "px x " + naturalHeight + "px)";

This text will need to be localized, and should not be appened to another string that might be localized. Other languages mgiht want to show them in a different order.

So: WebInspector.UIString("%d\u00d7%d", naturalWidth, naturalHeight)
Comment 6 Timothy Hatcher 2009-11-22 08:30:41 PST
I'm curiosu what Pavel thinks about the InjectedScriptAccess.getProperties code.
Comment 7 Pavel Feldman 2009-11-22 09:31:24 PST
Comment on attachment 43646 [details]
Patch with manual test

> +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);

No need to call this prototype - it was real prototype in the code you took it from, while in here it is just a node wrapper you create.
"new WebInspector.ObjectProxy(this.representedObject.id)" would also do.

> +        InjectedScriptAccess.getProperties(prototype, true, callback);
> +    },
> +
> +    _updateTitleWithRespectToNodeProperties: function(getPropertyValue)
> +    {

This looks too complex to keep it in here. I'd suggest at least introducing WebInspector.ObjectProxy.getPropertiesAsync(objectProxy, callback) that would make properties request and invoke given callback with the result object that would essentially be a map of name/values. You will then take the ones you need from there.

I can also imagine us showing tips for other kinds of the nodes, so it might make sense to make this code a bit more optimized. Getting all the properties and wrapping them might be too expensive. I'd suggest introducing InjectedScriptAccess.getPropertiesSimple(objectWrapper, ["array", "of", "names", "to", "query", "for"], callback); that would work for basic properties only (ones with primitive value types) and would not wrap them with ObjectPropertyProxy, i.e. simply send JSON object in return. I can do this part for you if you find explanation unclear.
Comment 8 Pavel Feldman 2009-11-22 12:17:02 PST
By the way, why manual test? You should be able to verify generated tip against expectations using inspector testing harness. See LayoutTests/inspector/styles-iframe.html as an expample.

1. You don't need runAfterIframeIsLoaded since you have no iframes in your case, just use its continuation.
2. Rename frontend_dumpStyles to frontend_dumpTooltip or something
3. You call frontend_expandDOMSubtree(WebInspector.domAgent.document); as in that example in order to expand the whole dom tree in elements panel
4. You continue execution in testController.runAfterPendingDispatches callback. It is safe to traverse WebInspector.domAgent.document structure there. In the styles-iframe test, IFRAME node's id is resolved. You would need to resolve IMG's one.
5. Last bit is to call your method calculating tooltip for node with that id asynchronously and to report result into testController.notifyDone (as in the example).

Send me a note if you need help!
Comment 9 Daniel Bates 2009-11-22 13:10:02 PST
(In reply to comment #5)
> (From update of attachment 43646 [details])
> > -        var title = this._nodeTitleInfo(this.representedObject, this.hasChildren, WebInspector.linkifyURL).title;
> > +        var self = this;
> > +        var callback = function(propertiesPayload) {
> > +            var getPropertyValue = function(propertyName) {
> > +                for (e in propertiesPayload)
> > +                    if (propertiesPayload[e].name == propertyName)
> > +                        return propertiesPayload[e].value.description;
> > +                return null;
> > +            };
> > +            self._updateTitleWithRespectToNodeProperties(getPropertyValue);
> > +        };
> > +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);
> > +        InjectedScriptAccess.getProperties(prototype, true, callback);
> 
> This is going to slow down the Elements panel, since it will delay updating the
> title waiting for the async InjectedScriptAccess.getProperties. (Slower in
> Chrome where the async is corss [sic] process. So we need to minimize these calls.)
> 
> We are paying a heavy cost of InjectedScriptAccess.getProperties for every DOM
> element just to filter it later for image elements. The
> InjectedScriptAccess.getProperties call was not designed for one-off property
> access, as you found, since it has .name properties. It is really only used for
> property inspection. I am not sure what the best solution is.
> 

Spoke to Pavel on IRC today (11/22). Will implement WebInspector.ObjectProxy.getPropertiesAsync (as Pavel suggested <https://bugs.webkit.org/show_bug.cgi?id=21554#c7>) and file a bug report so that we can further optimize property access.

> Also these should use named functions, with the brace o nthe next line like:
> 
> > +        function callback(propertiesPayload)
> > +        {
> > +            function getPropertyValue(propertyName)
> > +            {
> 
> > +                            if (node.nodeName.toLowerCase() == "img") {
> 
> Use === in cases like this.

Will change.

> > +                                tooltipText = offsetWidth + "px x " + offsetHeight + "px";
> 
> This should use \u00d7 for the multiplication sign, not "x". I don't think "px"
> is needed twice. You could do "123x456 pixels" like Safari, but that will need
> to be localized with a WebInspector.UIString. Or just leave off the units.

Will change to conform to Safari (i.e. "123x456 pixels").

> > +                                if (offsetHeight != naturalHeight || offsetWidth != naturalWidth)
> 
> Use !==.

Will change.

> > +                                    tooltipText += " (natural: " + naturalWidth + "px x " + naturalHeight + "px)";
> 
> This text will need to be localized, and should not be appened to another
> string that might be localized. Other languages mgiht want to show them in a
> different order.

I thought to append this to the tooltip only if the displayable size differed from the natural size.

> So: WebInspector.UIString("%d\u00d7%d", naturalWidth, naturalHeight)

So, I take it you only want to show the natural dimensions? Not the displayable dimensions?
Comment 10 Daniel Bates 2009-11-22 13:11:28 PST
(In reply to comment #8)
> By the way, why manual test? You should be able to verify generated tip against
> expectations using inspector testing harness. See
> LayoutTests/inspector/styles-iframe.html as an expample.
> 
> 1. You don't need runAfterIframeIsLoaded since you have no iframes in your
> case, just use its continuation.
> 2. Rename frontend_dumpStyles to frontend_dumpTooltip or something
> 3. You call frontend_expandDOMSubtree(WebInspector.domAgent.document); as in
> that example in order to expand the whole dom tree in elements panel
> 4. You continue execution in testController.runAfterPendingDispatches callback.
> It is safe to traverse WebInspector.domAgent.document structure there. In the
> styles-iframe test, IFRAME node's id is resolved. You would need to resolve
> IMG's one.
> 5. Last bit is to call your method calculating tooltip for node with that id
> asynchronously and to report result into testController.notifyDone (as in the
> example).
> 
> Send me a note if you need help!

I'll look into this.
Comment 11 Timothy Hatcher 2009-11-22 15:33:18 PST
You can show both sizes, you will just need two localized strings.

"%d\u00d7%d pixels"

and

"%d\u00d7%d pixels (Natural: %d\u00d7%d pixels)"
Comment 12 Daniel Bates 2009-11-25 13:33:16 PST
Created attachment 43866 [details]
Patch with manual test

Updated patch based on Timothy's and Pavel's comments.
Comment 13 Pavel Feldman 2009-11-25 14:10:14 PST
Comment on attachment 43866 [details]
Patch with manual test

A handful of style nits, otherwise good to go. Please put // TODO: Replace with InjectedScriptAccess.getBasicProperties(obj, [names]) in place of the extended Q&A block. That will save few bytes on loading / parsing.

>  
> +WebInspector.ElementsTreeElement.createTooltipForImageNode = function(properties)
> +{

It is Ok to define this one of prototype even though it does not use 'this'. We do that in other places.

> +    var tooltipText = null;
> +    var offsetHeight = properties["offsetHeight"];

You can use properties.offsetHeight (and there is no need in explicit intermediate variable I guess).


>  WebInspector.ElementsTreeElement.prototype = {
>      get highlighted()
>      {
> @@ -787,14 +801,30 @@ WebInspector.ElementsTreeElement.prototy
>          if (this._editing)
>              return;
>  

> +            return callback();

No need to return it, just call it.

> +        var objectProxy = new WebInspector.ObjectProxy(this.representedObject.id);
> +        WebInspector.ObjectProxy.getPropertiesAsync(objectProxy, callback);

It would be even better if you provided the names of the properties needed here, so that it would
be easier to understand what the 'properties' object you carry is needed for.



> +        var result = [];
> +        for (e in propertiesPayload)

We typically iterate over arrays using index 'for (var i = 0; i < properties.length; ++i)'
Comment 14 Daniel Bates 2009-11-25 17:35:59 PST
Created attachment 43881 [details]
Patch with test case

Updated patch based on Timothy's and Pavel's comments.

Replaced manual test case with DRT layout test based on inspector/styles-iframe.html
Comment 15 Pavel Feldman 2009-11-26 00:45:15 PST
Comment on attachment 43881 [details]
Patch with test case

Thanks for addressing the comments. It now looks really good and clean. Test is very good too. Couple of minot nits and re are ready to land:

img-tooltip.html should be called elements-img-tooltip (we try to group things by panels)

> +    function createPropertiesMapThenCallback(propertiesPayload)
> +    {

Since the interaction between front-end and injected script is asynchronous, node that you passed into getProperties might
be already deleted. You should be ready for that. 'propertiesPayload' will be 'false' in that case and you should pass it further
as undefined or []. You should then be able to handle missing properties when generating tooltip.

> +
> +    var innerMapping = WebInspector.domAgent._idToDOMNode;
> +    for (var nodeId in innerMapping) {
> +        if (innerMapping[nodeId].nodeName === "IMG") {
> +            WebInspector.ElementsTreeElement.prototype.createTooltipForImageNode(innerMapping[nodeId], callback);
> +            break;
> +        }
> +    }
> +}

Nit: You should do return in place of break and call testController.notifyDone(); after the loop. This way when elements panel breaks and has no IMG, you will not need to wait for timieout.

You probably need to store img inline in order not to wait for load event, but you should make it more simple. I'll attach a white rectangle of the same size to the bug. it is 80 times smaller.
Comment 16 Pavel Feldman 2009-11-26 00:47:46 PST
Created attachment 43907 [details]
[IMAGE] Blank image of needed size for tests.
Comment 17 Daniel Bates 2009-11-26 01:11:41 PST
Created attachment 43908 [details]
Patch with test case

Updated patch based on Pavel's comments.
Comment 18 Pavel Feldman 2009-11-26 05:51:04 PST
Comment on attachment 43908 [details]
Patch with test case

Thanks for doing that!
Comment 19 WebKit Commit Bot 2009-11-26 10:09:11 PST
Comment on attachment 43908 [details]
Patch with test case

Rejecting patch 43908 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11668 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4970 tests run.
86.81s total testing time

4969 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output
Comment 20 Pavel Feldman 2009-11-26 10:14:30 PST
Comment on attachment 43908 [details]
Patch with test case

Flaky test?
Comment 21 WebKit Commit Bot 2009-11-26 10:22:14 PST
Comment on attachment 43908 [details]
Patch with test case

Rejecting patch 43908 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11668 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4970 tests run.
89.19s total testing time

4969 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output
Comment 22 Eric Seidel (no email) 2009-11-26 11:46:20 PST
Failures caused by bug 31919.
Comment 23 Eric Seidel (no email) 2009-11-26 12:10:32 PST
Comment on attachment 43908 [details]
Patch with test case

I've moved the commit-queue to another machine for now. Let's try again.
Comment 24 WebKit Commit Bot 2009-11-26 12:28:06 PST
Comment on attachment 43908 [details]
Patch with test case

Clearing flags on attachment: 43908

Committed r51420: <http://trac.webkit.org/changeset/51420>
Comment 25 WebKit Commit Bot 2009-11-26 12:28:13 PST
All reviewed patches have been landed.  Closing bug.