DevTools: convert about:workers into about:inspect

DevTools: convert about:workers into about:inspect

BUG=118915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127931

[apavlov, 2012-03-19 16:16:47]

https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/resource...
File chrome/browser/resources/inspect/inspect.html (right):

https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/resource...
chrome/browser/resources/inspect/inspect.html:30: background-color: rgb(235,
239, 249);
Is there any consistent color format defined for Chromium CSS?

https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/inspect_ui.cc:340: virtual
~WorkerCreationDestructionListener() {
The style for this is to use "virtual ~...() {}" (closing brace on the same
line, no space in between).

https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/inspect_ui.cc:383: // Set up the chrome://workers/
source.
Is this comment still correct?

[pfeldman, 2012-03-20 11:28:41]

On 2012/03/19 16:16:47, apavlov wrote:
>
https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/resource...
> File chrome/browser/resources/inspect/inspect.html (right):
> 
>
https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/resource...
> chrome/browser/resources/inspect/inspect.html:30: background-color: rgb(235,
> 239, 249);
> Is there any consistent color format defined for Chromium CSS?
> 

I don't think so.

>
https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/inspect_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
> chrome/browser/ui/webui/inspect_ui.cc:340: virtual
> ~WorkerCreationDestructionListener() {
> The style for this is to use "virtual ~...() {}" (closing brace on the same
> line, no space in between).
> 

Done. 

>
https://chromiumcodereview.appspot.com/9724038/diff/1/chrome/browser/ui/webui...
> chrome/browser/ui/webui/inspect_ui.cc:383: // Set up the chrome://workers/
> source.
> Is this comment still correct?

Removed.

[yurys, 2012-03-20 12:42:06]

lgtm

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.js (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.js:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:160: ~InspectDataSource() {}
Should it be marked as virtual?

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:220: void
InspectDataSource::SendSharedWorkersData(int request_id,
We may want to rename this method as it now sends all information about all
inspectable instances.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:221: ListValue* rvh_list) {
style: wrong alignment

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui_browsertest.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui_browsertest.cc:38:
IN_PROC_BROWSER_TEST_F(InspectUITest, MAYBE_SharedWorkersList) {
Please add a test for other types of inspectable instances.

[pfeldman, 2012-03-20 13:22:10]

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.js (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.js:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/03/20 12:42:06, Yury Semikhatsky wrote:
> 2012

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:160: ~InspectDataSource() {}
On 2012/03/20 12:42:06, Yury Semikhatsky wrote:
> Should it be marked as virtual?

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:220: void
InspectDataSource::SendSharedWorkersData(int request_id,
On 2012/03/20 12:42:06, Yury Semikhatsky wrote:
> We may want to rename this method as it now sends all information about all
> inspectable instances.

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:221: ListValue* rvh_list) {
On 2012/03/20 12:42:06, Yury Semikhatsky wrote:
> style: wrong alignment

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui_browsertest.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui_browsertest.cc:38:
IN_PROC_BROWSER_TEST_F(InspectUITest, MAYBE_SharedWorkersList) {
On 2012/03/20 12:42:06, Yury Semikhatsky wrote:
> Please add a test for other types of inspectable instances.

Done.

[pfeldman, 2012-03-20 16:02:05]

Seeking for owners' stamp. This change renames files under
chrome/browser/ui/webui and chrome/browser/resources.

[csilv, 2012-03-20 18:22:02]

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.html (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:12: <style>
Please move CSS into a new file, inspect.css

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:15: font-size: 12px;
get rid of font-family, font-size... these are both set by the body element.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:34: }
order properties for img & .section alphabetically.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:54: <div class='list'
id="pages"></div>
order 'id' first, here and below

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.js (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.js:10: return
JSON.parse(xhr.responseText);
nit: 2 space indent

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:173: bool is_incognito,
nit, fix alignment on lines 173 & 174

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:221: ListValue* rvh_list) {
nit: fix alignment of line 221

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.h (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui_browsertest.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui_browsertest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: 2012

[pfeldman, 2012-03-20 19:18:47]

Thanks for the review!

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.html (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:12: <style>
On 2012/03/20 18:22:02, csilv wrote:
> Please move CSS into a new file, inspect.css

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:15: font-size: 12px;
On 2012/03/20 18:22:02, csilv wrote:
> get rid of font-family, font-size... these are both set by the body element.

As agreed offline, I'll leave these for now and remove ones from body.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:34: }
On 2012/03/20 18:22:02, csilv wrote:
> order properties for img & .section alphabetically.

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.html:54: <div class='list'
id="pages"></div>
On 2012/03/20 18:22:02, csilv wrote:
> order 'id' first, here and below

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
File chrome/browser/resources/inspect/inspect.js (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/resources/ins...
chrome/browser/resources/inspect/inspect.js:10: return
JSON.parse(xhr.responseText);
On 2012/03/20 18:22:02, csilv wrote:
> nit: 2 space indent

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:173: bool is_incognito,
On 2012/03/20 18:22:02, csilv wrote:
> nit, fix alignment on lines 173 & 174

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:221: ListValue* rvh_list) {
On 2012/03/20 18:22:02, csilv wrote:
> nit: fix alignment of line 221

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.h (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/20 18:22:02, csilv wrote:
> nit: 2012

Done.

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui_browsertest.cc (right):

http://codereview.chromium.org/9724038/diff/3001/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui_browsertest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/03/20 18:22:02, csilv wrote:
> nit: 2012

Done.

[csilv, 2012-03-20 19:58:02]

lgtm