Updated icons in Resources/Application panel

Updated icons in Resources/Application panel

BUG=612090
NOTRY=true

Committed: https://crrev.com/2294d7602983c1183ae966d995d15d3b695bf4d2
Cr-Commit-Position: refs/heads/master@{#394604}

[chowse, 2016-05-17 17:11:58]

chowse@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-05-17 17:16:52]

On 2016/05/17 17:11:59, chowse wrote:

We should use sprites to speed things up. You should either add those into
toolbarButtonGlyphs.svg or create a new sprite image beside.

[chowse, 2016-05-17 19:37:43]

On 2016/05/17 17:16:52, pfeldman wrote:
> On 2016/05/17 17:11:59, chowse wrote:
> 
> We should use sprites to speed things up. You should either add those into
> toolbarButtonGlyphs.svg or create a new sprite image beside.

Unless we have some scripts to generate the sprites and their corresponding CSS,
this seems like a big maintenance tax for negligible gain:

- Local requests don't have the latency or round-trip overhead that network
requests have. On my machine, individual DevTools icons are all fetched in <1ms
and in parallel. No FOUC.
- The payload savings are minimal. After feeding the images through ImgOptim and
zipping them, a sprite with 8 images was only 500b smaller than individual
images.

And it comes at the cost of:

- Having to modify the existing sprite image manually, including calculating
offsets by hand, which is very error-prone.
- Making it much harder to reason about the asset from its CSS
(background-offsets don't make it clear which icon is being referenced).
- Making changes more difficult to track (a change to a sprite is harder to
decipher than adding or modifying an individual file in source control).

Are there other things I'm overlooking?

[pfeldman, 2016-05-17 19:51:12]

> Unless we have some scripts to generate the sprites and their corresponding
CSS,
> this seems like a big maintenance tax for negligible gain:
> 

Scripts for sprites - yes. Scripts for css offsets - no.

> - Local requests don't have the latency or round-trip overhead that network
> requests have. On my machine, individual DevTools icons are all fetched in
<1ms
> and in parallel. No FOUC.

Every ms counts. This adds to perceived latency a lot + we run off cloud where
latency is much higher.

> - The payload savings are minimal. After feeding the images through ImgOptim
and
> zipping them, a sprite with 8 images was only 500b smaller than individual
> images.
> 

This is not a concern.

> And it comes at the cost of:
> 
> - Having to modify the existing sprite image manually, including calculating
> offsets by hand, which is very error-prone.
> - Making it much harder to reason about the asset from its CSS
> (background-offsets don't make it clear which icon is being referenced).
> - Making changes more difficult to track (a change to a sprite is harder to
> decipher than adding or modifying an individual file in source control).
> 
> Are there other things I'm overlooking?

We understand this all and decided to pay the price for the sake of speed and
perceived latency. You calculate offset once and that's it. Millions of users
spend less time loading as a result. It is worth a minute of your time.

[chowse, 2016-05-17 23:44:13]

Patchset #3 (id:40001) has been deleted

[chowse, 2016-05-18 00:06:41]

On 2016/05/17 19:51:12, pfeldman wrote:
> > Unless we have some scripts to generate the sprites and their corresponding
> CSS,
> > this seems like a big maintenance tax for negligible gain:
> > 
> 
> Scripts for sprites - yes. Scripts for css offsets - no.
> 
> > - Local requests don't have the latency or round-trip overhead that network
> > requests have. On my machine, individual DevTools icons are all fetched in
> <1ms
> > and in parallel. No FOUC.
> 
> Every ms counts. This adds to perceived latency a lot + we run off cloud where
> latency is much higher.
> 
> > - The payload savings are minimal. After feeding the images through ImgOptim
> and
> > zipping them, a sprite with 8 images was only 500b smaller than individual
> > images.
> > 
> 
> This is not a concern.
> 
> > And it comes at the cost of:
> > 
> > - Having to modify the existing sprite image manually, including calculating
> > offsets by hand, which is very error-prone.
> > - Making it much harder to reason about the asset from its CSS
> > (background-offsets don't make it clear which icon is being referenced).
> > - Making changes more difficult to track (a change to a sprite is harder to
> > decipher than adding or modifying an individual file in source control).
> > 
> > Are there other things I'm overlooking?
> 
> We understand this all and decided to pay the price for the sake of speed and
> perceived latency. You calculate offset once and that's it. Millions of users
> spend less time loading as a result. It is worth a minute of your time.

Done. All assets are now sprited. Updating toolbarButtonGlyphs would've required
changing the sprite's dimensions, which would require updating dozens of CSS
files, so I placed the icons (which are only used in this panel) in a new
sprite. Also deleted all the previously used (and unsprited) icons and confirmed
they weren't used elsewhere.

[pfeldman, 2016-05-18 05:36:50]

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js
(right):

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:59:
this.localStorageListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Local
Storage"), "LocalStorage", ["localstorage-tree-item", "table-tree-item",
"resource-tree-item"]);
localstorage-tree-item is unused.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:61:
this.sessionStorageListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Session
Storage"), "SessionStorage", ["sessionstorage-tree-item", "table-tree-item",
"resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:65:
this.databasesListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Web SQL"),
"Databases", ["websql-tree-item", "database-tree-item", "resource-tree-item"]);
websql-tree-item is unused.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:73:
this.applicationCacheListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Application
Cache"), "ApplicationCache", ["appcache-tree-item", "table-tree-item",
"resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:439:
var domStorageTreeElement = new WebInspector.DOMStorageTreeElement(this,
domStorage, (domStorage.isLocalStorage ? "local-storage-tree-item" :
"session-storage-tree-item"));
no need for icons on actual elements?

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1295:
WebInspector.StorageCategoryTreeElement.call(this, storagePanel,
WebInspector.UIString("Cache Storage"), "CacheStorage",
["service-worker-cache-tree-item", "database-tree-item", "resource-tree-item"]);
Remove service-worker-cache-tree-item

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1405:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, cache.cacheName + "
- " + cache.securityOrigin, ["service-worker-cache-tree-item",
"table-tree-item", "resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1476:
WebInspector.BaseStorageTreeElement.call(this, storagePanel,
WebInspector.UIString("Service Workers"), ["service-worker-tree-item",
"resource-tree-item"], false, false);
here and below: no need to specify "false, false", they are default.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1581:
WebInspector.StorageCategoryTreeElement.call(this, storagePanel,
WebInspector.UIString("IndexedDB"), "IndexedDB", ["indexed-db-tree-item",
"database-tree-item", "resource-tree-item"]);
Remove indexed-db-tree-item

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1707:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, databaseId.name + "
- " + databaseId.securityOrigin, ["indexed-db-tree-item", "database-tree-item",
"resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1814:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, objectStore.name,
["indexed-db-tree-item", "table-tree-item", "resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1945:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, index.name,
["indexed-db-tree-item", "index-tree-item", "resource-tree-item"]);
ditto

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:2012:
WebInspector.BaseStorageTreeElement.call(this, storagePanel,
domStorage.securityOrigin ? domStorage.securityOrigin :
WebInspector.UIString("Local Files"), [className, "table-tree-item",
"resource-tree-item"]);
no need for className anymore

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css
(right):

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css:104:
background: #fff !important;
white for consistency.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css:115:
/*
We don't land comments.

[chowse, 2016-05-18 17:17:42]

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js
(right):

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:59:
this.localStorageListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Local
Storage"), "LocalStorage", ["localstorage-tree-item", "table-tree-item",
"resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> localstorage-tree-item is unused.

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:61:
this.sessionStorageListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Session
Storage"), "SessionStorage", ["sessionstorage-tree-item", "table-tree-item",
"resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:65:
this.databasesListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Web SQL"),
"Databases", ["websql-tree-item", "database-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> websql-tree-item is unused.

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:73:
this.applicationCacheListTreeElement = new
WebInspector.StorageCategoryTreeElement(this, WebInspector.UIString("Application
Cache"), "ApplicationCache", ["appcache-tree-item", "table-tree-item",
"resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:439:
var domStorageTreeElement = new WebInspector.DOMStorageTreeElement(this,
domStorage, (domStorage.isLocalStorage ? "local-storage-tree-item" :
"session-storage-tree-item"));
On 2016/05/18 05:36:50, pfeldman wrote:
> no need for icons on actual elements?

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1295:
WebInspector.StorageCategoryTreeElement.call(this, storagePanel,
WebInspector.UIString("Cache Storage"), "CacheStorage",
["service-worker-cache-tree-item", "database-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:49, pfeldman wrote:
> Remove service-worker-cache-tree-item 

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1405:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, cache.cacheName + "
- " + cache.securityOrigin, ["service-worker-cache-tree-item",
"table-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1476:
WebInspector.BaseStorageTreeElement.call(this, storagePanel,
WebInspector.UIString("Service Workers"), ["service-worker-tree-item",
"resource-tree-item"], false, false);
On 2016/05/18 05:36:50, pfeldman wrote:
> here and below: no need to specify "false, false", they are default.

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1581:
WebInspector.StorageCategoryTreeElement.call(this, storagePanel,
WebInspector.UIString("IndexedDB"), "IndexedDB", ["indexed-db-tree-item",
"database-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> Remove indexed-db-tree-item
> 

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1707:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, databaseId.name + "
- " + databaseId.securityOrigin, ["indexed-db-tree-item", "database-tree-item",
"resource-tree-item"]);
On 2016/05/18 05:36:49, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1814:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, objectStore.name,
["indexed-db-tree-item", "table-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1945:
WebInspector.BaseStorageTreeElement.call(this, storagePanel, index.name,
["indexed-db-tree-item", "index-tree-item", "resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:2012:
WebInspector.BaseStorageTreeElement.call(this, storagePanel,
domStorage.securityOrigin ? domStorage.securityOrigin :
WebInspector.UIString("Local Files"), [className, "table-tree-item",
"resource-tree-item"]);
On 2016/05/18 05:36:50, pfeldman wrote:
> no need for className anymore

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css
(right):

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css:104:
background: #fff !important;
On 2016/05/18 05:36:50, pfeldman wrote:
> white for consistency.

Done.

https://codereview.chromium.org/1979163003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/resourcesSidebar.css:115:
/*
On 2016/05/18 05:36:50, pfeldman wrote:
> We don't land comments.

Done.

[pfeldman, 2016-05-18 18:09:30]

lgtm

[chowse, 2016-05-18 18:51:42]

The CQ bit was checked by chowse@chromium.org

[commit-bot: I haz the power, 2016-05-18 18:52:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1979163003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1979163003/80001

[pfeldman, 2016-05-18 23:38:41]

Description was changed from

==========
Updated icons in Resources/Application panel

BUG=612090
==========

to

==========
Updated icons in Resources/Application panel

BUG=612090
NOTRY=true
==========

[pfeldman, 2016-05-18 23:38:46]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2016-05-18 23:38:52]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2016-05-18 23:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1979163003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1979163003/80001

[commit-bot: I haz the power, 2016-05-18 23:45:59]

Description was changed from

==========
Updated icons in Resources/Application panel

BUG=612090
NOTRY=true
==========

to

==========
Updated icons in Resources/Application panel

BUG=612090
NOTRY=true
==========

[commit-bot: I haz the power, 2016-05-18 23:46:02]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-18 23:48:49]

Description was changed from

==========
Updated icons in Resources/Application panel

BUG=612090
NOTRY=true
==========

to

==========
Updated icons in Resources/Application panel

BUG=612090
NOTRY=true

Committed: https://crrev.com/2294d7602983c1183ae966d995d15d3b695bf4d2
Cr-Commit-Position: refs/heads/master@{#394604}
==========

[commit-bot: I haz the power, 2016-05-18 23:48:50]

Patchset 4 (id:??) landed as
https://crrev.com/2294d7602983c1183ae966d995d15d3b695bf4d2
Cr-Commit-Position: refs/heads/master@{#394604}