Improve layout for bookmark-apps. Add a treatment for small icons.

Improve layout for bookmark-apps. Add a treatment for small icons.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94714

[Evan Stade, 2011-07-26 22:26:00]

ready for review? could you sync to ToT if so?

[Greg Billock, 2011-07-27 01:44:58]

On 2011/07/26 22:26:00, Evan Stade wrote:
> ready for review? could you sync to ToT if so?

OK, after a pain in the neck rebase, I think this is ready to look at.

[Evan Stade, 2011-07-27 16:31:45]

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
chrome/browser/extensions/extensions_ui.cc:655:
extension_data->SetBoolean("icon_exists", icon_exists);
this is unused, right? Why set it?

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:182: this.use_small_icon_ = true;
useSmallIcon_

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:185: if (this.use_small_icon_)
ternary operator

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:191: if (this.use_small_icon_)
appImg.style.WebkitMaskImage = url(apImg.src)

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:200: imgDiv.style.display =
'-webkit-box';
all this style stuff should be in a css file, and all you'd do here is add a
class.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:202: imgDiv.style['vertical-align'] =
'middle';
notwithstanding my above comment, the way you'd do this is

imgDiv.style.WebkitBoxAlign = ...
imgDiv.style.verticalAlign = ...

basically everything after a hyphen gets capitalized

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extension_icon_source.cc:79: bool* exists) {
nit: set exists to true at the start of the function and only set to false if
(exists && GetIconURL == GURL())

[Greg Billock, 2011-07-27 19:52:24]

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
chrome/browser/extensions/extensions_ui.cc:655:
extension_data->SetBoolean("icon_exists", icon_exists);
On 2011/07/27 16:31:45, Evan Stade wrote:
> this is unused, right? Why set it?

I changed line 647 to the new signature. I thought the extension manager is
likely to want this, so put it in. I could just pass NULL though...

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:182: this.use_small_icon_ = true;
On 2011/07/27 16:31:45, Evan Stade wrote:
> useSmallIcon_

Done.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:185: if (this.use_small_icon_)
On 2011/07/27 16:31:45, Evan Stade wrote:
> ternary operator

Done.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:191: if (this.use_small_icon_)
On 2011/07/27 16:31:45, Evan Stade wrote:
> appImg.style.WebkitMaskImage = url(apImg.src)

I'm not sure this mask image is doing anything for us -- it's getting overridden
elsewhere to be 100% of the area, which is probably right. Shall I just scrap
this?

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:200: imgDiv.style.display =
'-webkit-box';
On 2011/07/27 16:31:45, Evan Stade wrote:
> all this style stuff should be in a css file, and all you'd do here is add a
> class.

Done.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:202: imgDiv.style['vertical-align'] =
'middle';
On 2011/07/27 16:31:45, Evan Stade wrote:
> notwithstanding my above comment, the way you'd do this is
> 
> imgDiv.style.WebkitBoxAlign = ...
> imgDiv.style.verticalAlign = ...
> 
> basically everything after a hyphen gets capitalized

ah HA! :-)

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extension_icon_source.cc:79: bool* exists) {
On 2011/07/27 16:31:45, Evan Stade wrote:
> nit: set exists to true at the start of the function and only set to false if
> (exists && GetIconURL == GURL())

Done.

[Evan Stade, 2011-07-27 23:04:31]

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
chrome/browser/extensions/extensions_ui.cc:655:
extension_data->SetBoolean("icon_exists", icon_exists);
On 2011/07/27 19:52:24, Greg Billock wrote:
> On 2011/07/27 16:31:45, Evan Stade wrote:
> > this is unused, right? Why set it?
> 
> I changed line 647 to the new signature. I thought the extension manager is
> likely to want this, so put it in. I could just pass NULL though...

I would just pass NULL for now instead of adding code that doesn't do anything.
You can file a bug if you think that the extension manager should use it
somehow.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:191: if (this.use_small_icon_)
On 2011/07/27 19:52:24, Greg Billock wrote:
> On 2011/07/27 16:31:45, Evan Stade wrote:
> > appImg.style.WebkitMaskImage = url(apImg.src)
> 
> I'm not sure this mask image is doing anything for us -- it's getting
overridden
> elsewhere to be 100% of the area, which is probably right. Shall I just scrap
> this?

yea I guess

[Greg Billock, 2011-07-28 16:59:59]

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/extensions/ex...
chrome/browser/extensions/extensions_ui.cc:655:
extension_data->SetBoolean("icon_exists", icon_exists);
Nah, showing the default 'huh?' icon is probably fine here.

On 2011/07/27 23:04:31, Evan Stade wrote:
> On 2011/07/27 19:52:24, Greg Billock wrote:
> > On 2011/07/27 16:31:45, Evan Stade wrote:
> > > this is unused, right? Why set it?
> > 
> > I changed line 647 to the new signature. I thought the extension manager is
> > likely to want this, so put it in. I could just pass NULL though...
> 
> I would just pass NULL for now instead of adding code that doesn't do
anything.
> You can file a bug if you think that the extension manager should use it
> somehow.

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:191: if (this.use_small_icon_)
On 2011/07/27 23:04:31, Evan Stade wrote:
> On 2011/07/27 19:52:24, Greg Billock wrote:
> > On 2011/07/27 16:31:45, Evan Stade wrote:
> > > appImg.style.WebkitMaskImage = url(apImg.src)
> > 
> > I'm not sure this mask image is doing anything for us -- it's getting
> overridden
> > elsewhere to be 100% of the area, which is probably right. Shall I just
scrap
> > this?
> 
> yea I guess

Done.

[Evan Stade, 2011-07-28 23:21:33]

lgtm

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.css (right):

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:30: display: -webkit-box;
alphabetize

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:186: : this.appData_.icon_big;
I think technically : goes at the end of the previous line

[Greg Billock, 2011-07-29 16:55:57]

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.css (right):

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:30: display: -webkit-box;
On 2011/07/28 23:21:33, Evan Stade wrote:
> alphabetize

Done.

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7452008/diff/11003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:186: : this.appData_.icon_big;
On 2011/07/28 23:21:33, Evan Stade wrote:
> I think technically : goes at the end of the previous line

OK.

[commit-bot: I haz the power, 2011-07-29 17:55:50]

Change committed as 94714

[Greg Billock, 2011-08-02 17:37:38]

On 2011/07/29 17:55:50, I haz the power (commit-bot) wrote:
> Change committed as 94714

Was rolled back -- there is another views file depending on this change. Now
fixed, and the trybot passes.