Added separate tabs for apps / extensions in dev_tools app.

Added separate tabs for apps / extensions in dev_tools app.
Minor css-tweaks to improve ui.



Add tabs for extensions / apps in dev_tools app.


BUG=149036
TBR=asargent@chromium.org

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

[asargent_no_longer_on_chrome, 2013-03-18 23:49:41]

chrome/browser/extensions/ LGTM

[Dan Beam, 2013-03-20 19:05:30]

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
File chrome/browser/resources/apps_debugger/css/items.css (right):

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/css/items.css:15: margin: 15px 0 15px;
margin: 15px 0;

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/css/items.css:55:
#app-settings-list:not(.empty-item-list) ~#no-apps,
s/~#/~ #/

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/css/items.css:136: left: 0;
I'm confused, have you tried this in RTL?  It seems like if you want it to stick
to the leading edge (left in LTR, right in RTL) you can remove the
html[dir='rtl'] block below and just say:

  left: 0;
  right: 0;

(both in the same rule) and left will apply in LTR and right will apply in RTL.

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/css/items.css:195: box-shadow: 0 0 0;
why isn't this:

  box-shadow: none;

also, #tab-panels?

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
File chrome/browser/resources/apps_debugger/js/items.js (right):

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/js/items.js:22: 
nit: no \n

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/js/items.js:23: // Initialize the tabs.
^ considering this is right below `initialize`, this comment either a) isn't
necessary, or b) should probably say something more along the lines of "Decorate
the tab box", but even that is pretty self-evident

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/js/items.js:24: cr.ui.decorate('tabbox',
cr.ui.TabBox);
nit: tab-box, IMO

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/js/items_list.js:62:
ItemsList.prototype.data_ = extensionList;
^ I'm confused as to what this is doing?

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/js/items_list.js:82: if
(item.name.toLowerCase().search(filter) < 0)
nit: if (filter && item.name.toLowerCase().search(filter) < 0)

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
File chrome/browser/resources/apps_debugger/main.html (right):

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/main.html:8: ii
^ remove

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/main.html:33: <tabbox id="tabbox">
^ is this the standard tab element naming?

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/main.html:36: <tab> Extensions </tab>
remove spaces, where's translations?

https://chromiumcodereview.appspot.com/12693022/diff/1/chrome/browser/resourc...
chrome/browser/resources/apps_debugger/main.html:45: <input id="search"
type="text" placeholder="type to search"
translations? this will conflict with your other patch...

[Gaurav, 2013-03-29 21:14:26]

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/css/items.css (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/css/items.css:15: margin: 15px 0 15px;
On 2013/03/20 19:05:30, Dan Beam wrote:
> margin: 15px 0;

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/css/items.css:55:
#app-settings-list:not(.empty-item-list) ~#no-apps,
On 2013/03/20 19:05:30, Dan Beam wrote:
> s/~#/~ #/

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/css/items.css:195: box-shadow: 0 0 0;
On 2013/03/20 19:05:30, Dan Beam wrote:
> why isn't this:
> 
>   box-shadow: none;
> 
> also, #tab-panels?

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items.js (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items.js:22: 
On 2013/03/20 19:05:30, Dan Beam wrote:
> nit: no \n

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items.js:23: // Initialize the tabs.
On 2013/03/20 19:05:30, Dan Beam wrote:
> ^ considering this is right below `initialize`, this comment either a) isn't
> necessary, or b) should probably say something more along the lines of
"Decorate
> the tab box", but even that is pretty self-evident

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items.js:24: cr.ui.decorate('tabbox',
cr.ui.TabBox);
On 2013/03/20 19:05:30, Dan Beam wrote:
> nit: tab-box, IMO

This only seem to work with "tabbox".

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items_list.js:62:
ItemsList.prototype.data_ = extensionList;
On 2013/03/20 19:05:30, Dan Beam wrote:
> ^ I'm confused as to what this is doing?

It initializes the data in ItemsList fow which the items are created in
createNode_()

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items_list.js:82: if
(item.name.toLowerCase().search(filter) < 0)
On 2013/03/20 19:05:30, Dan Beam wrote:
> nit: if (filter && item.name.toLowerCase().search(filter) < 0)

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/main.html (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/main.html:8: ii
On 2013/03/20 19:05:30, Dan Beam wrote:
> ^ remove

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/main.html:33: <tabbox id="tabbox">
On 2013/03/20 19:05:30, Dan Beam wrote:
> ^ is this the standard tab element naming?

Not sure, it is used in some code. Changing it to "tab-box"

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/main.html:36: <tab> Extensions </tab>
On 2013/03/20 19:05:30, Dan Beam wrote:
> remove spaces, where's translations?

Done.

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/main.html:45: <input id="search"
type="text" placeholder="type to search"
On 2013/03/20 19:05:30, Dan Beam wrote:
> translations? this will conflict with your other patch...

Done.

[Dan Beam, 2013-03-29 21:48:13]

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items_list.js:62:
ItemsList.prototype.data_ = extensionList;
On 2013/03/29 21:14:26, Gaurav wrote:
> On 2013/03/20 19:05:30, Dan Beam wrote:
> > ^ I'm confused as to what this is doing?
> 
> It initializes the data in ItemsList fow which the items are created in
> createNode_()

This should be setting this on an instance, not on the prototype (which affects
all future instances), AFAIK.

[Dan Beam, 2013-04-03 22:56:24]

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items.js (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items.js:35: });
^ why was this deleted?

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/css/items.css (right):

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/css/items.css:143: left: 0.5em;
^ this is still wrong (0.5em in RTL doesn't match either left or right in LTR)
what are you trying to accomplish here? sticking to both edges of something?

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:81: * ItemsList
constructor.
^ this comment isn't useful (it's easily derivable from @constructor and class
name). please explain the point of needing an items list class.

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:86: this.itemsTabNode_ =
itemsTabNode;
assert(this.itemsTabNode_);

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:100: * @type {!Object}
@type {!HTMLElement}

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:101: *@private
* @private

[Gaurav, 2013-04-04 17:50:58]

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items.js (right):

https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items.js:35: });
On 2013/04/03 22:56:24, Dan Beam wrote:
> ^ why was this deleted?
Instead of writing a permissions overlay ui, I reused the permissions dialog
supported by apis directly.

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/css/items.css (right):

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/css/items.css:143: left: 0.5em;
On 2013/04/03 22:56:24, Dan Beam wrote:
> ^ this is still wrong (0.5em in RTL doesn't match either left or right in LTR)
> what are you trying to accomplish here? sticking to both edges of something?

Sorry, this was not required. Deleting it.

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:81: * ItemsList
constructor.
On 2013/04/03 22:56:24, Dan Beam wrote:
> ^ this comment isn't useful (it's easily derivable from @constructor and class
> name). please explain the point of needing an items list class.

Used a class to group the functionality of creating the items (apps and
extensions). Is it better to do it without a class ?

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:86: this.itemsTabNode_ =
itemsTabNode;
On 2013/04/03 22:56:24, Dan Beam wrote:
> assert(this.itemsTabNode_);

Done.

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:100: * @type {!Object}
On 2013/04/03 22:56:24, Dan Beam wrote:
> @type {!HTMLElement}

Done.

https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:101: *@private
On 2013/04/03 22:56:24, Dan Beam wrote:
> * @private

Done.

[Gaurav, 2013-04-05 20:06:40]

On 2013/04/04 17:50:58, Gaurav wrote:
>
https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
> File chrome/browser/resources/apps_debugger/js/items.js (right):
> 
>
https://codereview.chromium.org/12693022/diff/1/chrome/browser/resources/apps...
> chrome/browser/resources/apps_debugger/js/items.js:35: });
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > ^ why was this deleted?
> Instead of writing a permissions overlay ui, I reused the permissions dialog
> supported by apis directly.
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> File chrome/browser/resources/apps_debugger/css/items.css (right):
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/apps_debugger/css/items.css:143: left: 0.5em;
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > ^ this is still wrong (0.5em in RTL doesn't match either left or right in
LTR)
> > what are you trying to accomplish here? sticking to both edges of something?
> 
> Sorry, this was not required. Deleting it.
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> File chrome/browser/resources/apps_debugger/js/items_list.js (right):
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/apps_debugger/js/items_list.js:81: * ItemsList
> constructor.
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > ^ this comment isn't useful (it's easily derivable from @constructor and
class
> > name). please explain the point of needing an items list class.
> 
> Used a class to group the functionality of creating the items (apps and
> extensions). Is it better to do it without a class ?
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/apps_debugger/js/items_list.js:86: this.itemsTabNode_
=
> itemsTabNode;
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > assert(this.itemsTabNode_);
> 
> Done.
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/apps_debugger/js/items_list.js:100: * @type {!Object}
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > @type {!HTMLElement}
> 
> Done.
> 
>
https://codereview.chromium.org/12693022/diff/10001/chrome/browser/resources/...
> chrome/browser/resources/apps_debugger/js/items_list.js:101: *@private
> On 2013/04/03 22:56:24, Dan Beam wrote:
> > * @private
> 
> Done.

ping

[Dan Beam, 2013-04-05 20:27:03]

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
chrome/browser/resources/apps_debugger/js/items_list.js:85:
assert(this.itemsTabNode_);
this.itemsTabNode_ = itemsTabNode;
assert(this.itemsTabNode_);

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
chrome/browser/resources/apps_debugger/js/items_list.js:100: * @type
{!HtmlElement}
@type {HTMLElement}

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
chrome/browser/resources/apps_debugger/js/items_list.js:103: itemsTabNode_:
Object,
itemsTabNode_: null,

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
File chrome/browser/resources/apps_debugger/main.html (right):

https://chromiumcodereview.appspot.com/12693022/diff/23001/chrome/browser/res...
chrome/browser/resources/apps_debugger/main.html:40: <button id =
"update-items-now"
s/ = /=/

[Gaurav, 2013-04-05 22:17:37]

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:85:
assert(this.itemsTabNode_);
On 2013/04/05 20:27:03, Dan Beam wrote:
> this.itemsTabNode_ = itemsTabNode;
> assert(this.itemsTabNode_);

Done.

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:100: * @type
{!HtmlElement}
On 2013/04/05 20:27:03, Dan Beam wrote:
> @type {HTMLElement}

Done.

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:103: itemsTabNode_:
Object,
On 2013/04/05 20:27:03, Dan Beam wrote:
> itemsTabNode_: null,

Done.

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/main.html (right):

https://codereview.chromium.org/12693022/diff/23001/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/main.html:40: <button id =
"update-items-now"
On 2013/04/05 20:27:03, Dan Beam wrote:
> s/ = /=/

Done.

[Dan Beam, 2013-04-05 22:22:37]

lgtm

https://codereview.chromium.org/12693022/diff/29002/chrome/browser/resources/...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/12693022/diff/29002/chrome/browser/resources/...
chrome/browser/resources/apps_debugger/js/items_list.js:80: /**
it'd still be useful to put a comment that explains why you need this class/what
it does

[commit-bot: I haz the power, 2013-04-05 22:53:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/12693022/32001

[commit-bot: I haz the power, 2013-04-05 22:53:29]

Presubmit check for 12693022-32001 failed and returned exit status 1.

INFO:root:Found 7 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org or dbeam@chromium.org

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/resources/apps_debugger/js/main_scripts.js, line 11, 82 chars

Found JavaScript style violations in
chrome/browser/resources/apps_debugger/js/items_list.js:
  line 2: E0010: Missing semicolon at end of line
i// Use of this source code is governed by a BSD-style license that can be
^

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-04-05 23:37:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/12693022/17011

[commit-bot: I haz the power, 2013-04-06 05:01:40]

Change committed as 192707