Bug 59749 – Web Inspector: upstream Remote Debugging frontend page support code.

• Status: RESOLVED
• Resolution: INVALID
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 91570 [details]
[patch] initial version

[Ilya Tikhonovsky]

Created attachment 91577 [details]
frontpage sample

[Joseph Pecoraro]

Comment on attachment 91570 [details]
[patch] initial version

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;

> Source/WebKit/chromium/src/js/frontend.css:48
> +    margin-left:220px;

Nit (style): Add a space between the property + value like the others.

> 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.

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 =).

> Source/WebKit/chromium/src/js/frontend.js:56
> +    var list = document.getElementById('items').appendChild(frontendRef);

Nit: Remove the local variable. It is never used.

[Ilya Tikhonovsky]

Created attachment 91591 [details]
[patch] second version. comments addressed.

(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.

[Ilya Tikhonovsky]

Created attachment 91593 [details]
sample frontend page

[Ilya Tikhonovsky]

Created attachment 91594 [details]
sample frontend page

[Pavel Feldman]

Comment on attachment 91591 [details]
[patch] second version. comments addressed.

Remote debugging landing page should be platform-specific, branded and belong to the code that makes page discovery. There is not much code to reuse, but the flexibility suffers a lot.