[webkit-reviews] review requested: [Bug 59749] Web Inspector: upstream Remote Debugging frontend page support code. : [Attachment 91591] [patch] second version. comments addressed.

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 59749: Web Inspector: upstream Remote Debugging frontend page support code.
https://bugs.webkit.org/show_bug.cgi?id=59749

Attachment 91591: [patch] second version. comments addressed.
https://bugs.webkit.org/attachment.cgi?id=91591&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(In reply to comment #3)
> (From update of attachment 91570 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=91570&action=review
> 
> Its tough to review this without the html file, but maybe the
> chromium inspector developers are more familiar with the
> situation. Looks good to me.
> 
> > Source/WebKit/chromium/src/js/frontend.css:38
> > +	 border-color: lightgray;
> > +	 border-style: solid;
> > +	 border-width: 6px;
> 
> Nit (style): Use the shorthand here?
> 
>   border: 6px solid lightgray;

done.


> > Source/WebKit/chromium/src/js/frontend.css:48
> > +	 margin-left:220px;
> 
> Nit (style): Add a space between the property + value like the others.

done.

> 
> > Source/WebKit/chromium/src/js/frontend.js:43
> > +	 item.style.cssText = 'background-image:url(' + thumbnailUrl + ')';
> 
> Does thumbnailUrl need to be URL encoded to remove possible bad
> characters (like a single quote in this case), or has that been done
> already? My guess is it might be a datauri, but might not always
> be in the future.

really it must be an url.

> 
> Nit: Normally inspector code uses double quotes around strings, but
> this is in WebKit/chromium, and I'm not sure if style is different. I'm
> happy as long as it is consistent, which it is =).

done.

> 
> > Source/WebKit/chromium/src/js/frontend.js:56
> > +	 var list = document.getElementById('items').appendChild(frontendRef);
> 
> Nit: Remove the local variable. It is never used.

done.