Revert 94715 - Revert 94714 - Improve layout for bookmark-apps. Add treatment for small icons.

Revert 94715 - Revert 94714 - Improve layout for bookmark-apps. Add treatment for small icons.

Now with support for linux views!

Original review URL: http://codereview.chromium.org/7452008

BUG=none
TEST=none


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

[Evan Stade, 2011-08-02 17:57:33]

lgtm pending green trybot results (I guess
http://codereview.chromium.org/7452008 has those?)

[Greg Billock, 2011-08-02 18:10:31]

On 2011/08/02 17:57:33, Evan Stade wrote:
> lgtm pending green trybot results (I guess
> http://codereview.chromium.org/7452008 has those?)

Yeah, the trybots are stuck on py_function_tests that appear unrelated; all
other builds/tests look good including linux_view, which was the problem on the
previous one.

[commit-bot: I haz the power, 2011-08-02 19:46:10]

Can't apply patch for file chrome/browser/resources/ntp4/apps_page.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/ntp4/apps_page.js
Hunk #1 FAILED at 178.
Hunk #2 FAILED at 206.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/resources/ntp4/apps_page.js.rej

[Greg Billock, 2011-08-02 21:35:54]

On 2011/08/02 18:10:31, Greg Billock wrote:
> On 2011/08/02 17:57:33, Evan Stade wrote:
> > lgtm pending green trybot results (I guess
> > http://codereview.chromium.org/7452008 has those?)
> 
> Yeah, the trybots are stuck on py_function_tests that appear unrelated; all
> other builds/tests look good including linux_view, which was the problem on
the
> previous one.

Merged with your changes to apps_page.js. Want to look them over? I'm getting a
startup crash on ntp4 right now, so I haven't verified it still works yet.

[Evan Stade, 2011-08-02 21:47:14]

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

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:195: imgDiv.setAttribute('class',
'app-icon-div');
imgDiv.class =

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:280: this.appImg_.style.width =
this.appImg_.style.height = '32px';
define in css file

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:282:
this.imgDiv_.style['margin-left'] = margin + 'px';
marginLeft

but I feel like you should be able to use css to center, e.g., margin-left: auto
margin-right: auto ought to work.

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:284:
this.imgDiv_.style['margin-bottom'] = '4px';
define in css file

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:285: }
else on same line

[Greg Billock, 2011-08-03 19:05:50]

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

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:195: imgDiv.setAttribute('class',
'app-icon-div');
On 2011/08/02 21:47:14, Evan Stade wrote:
> imgDiv.class = 

Done. (className)

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:280: this.appImg_.style.width =
this.appImg_.style.height = '32px';
On 2011/08/02 21:47:14, Evan Stade wrote:
> define in css file

These should really come from the icon size we choose to use, but it gets set
here in the else clause, so I want to keep it explicit.

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:282:
this.imgDiv_.style['margin-left'] = margin + 'px';
Yeah, I moved these and got it looking better with the div size compared to the
recommended icon sizes, too.

On 2011/08/02 21:47:14, Evan Stade wrote:
> marginLeft
> 
> but I feel like you should be able to use css to center, e.g., margin-left:
auto
> margin-right: auto ought to work.

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:284:
this.imgDiv_.style['margin-bottom'] = '4px';
On 2011/08/02 21:47:14, Evan Stade wrote:
> define in css file

Done.

http://codereview.chromium.org/7553020/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:285: }
On 2011/08/02 21:47:14, Evan Stade wrote:
> else on same line

Done.

[Evan Stade, 2011-08-03 19:17:15]

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

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:30: border: 1px solid gray;
there's probably a better color than "grey" for this, i.e. a theme aware color,
but grey is ok for a first cut. Can you add a todo to fix it though?

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:34: width: 96px;
comment where the 96 comes from

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:42: margin-top: 15px;
alpahbetize

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

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:192: appImg.addEventListener('click',
this.onClick_.bind(this));
I think this is redundant in the useSmallIcon_ case

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:277: if (this.useSmallIcon_) {
yay reduced complexity. now you can use ternary operator.

[Greg Billock, 2011-08-03 21:15:49]

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

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:30: border: 1px solid gray;
On 2011/08/03 19:17:15, Evan Stade wrote:
> there's probably a better color than "grey" for this, i.e. a theme aware
color,
> but grey is ok for a first cut. Can you add a todo to fix it though?

Yes. I'd guess the style on this is subject to significant change once the UI
people see it. :-) All the border stuff, margins, etc, I just made up.

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:34: width: 96px;
On 2011/08/03 19:17:15, Evan Stade wrote:
> comment where the 96 comes from

Done.

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:42: margin-top: 15px;
On 2011/08/03 19:17:15, Evan Stade wrote:
> alpahbetize

Done.

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

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:192: appImg.addEventListener('click',
this.onClick_.bind(this));
On 2011/08/03 19:17:15, Evan Stade wrote:
> I think this is redundant in the useSmallIcon_ case

Moved inside non-small-icon case and removed the comment.

http://codereview.chromium.org/7553020/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:277: if (this.useSmallIcon_) {
On 2011/08/03 19:17:15, Evan Stade wrote:
> yay reduced complexity. now you can use ternary operator.

Done.

[Evan Stade, 2011-08-03 22:15:05]

lgtm

[Evan Stade, 2011-08-03 22:17:31]

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

http://codereview.chromium.org/7553020/diff/12002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:35: /* 96px is the suggested icon
size for Chrome apps */
this is almost correct: it's the minimum size we show an icon for for chrome
apps. I think the suggested size is 128px (you'll notice the app images get
expanded and shrunk as you change the amount of space available)

http://codereview.chromium.org/7553020/diff/12002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:43: -webkit-box-pack: center;
nit: -webkit-foo goes at the very end

[Greg Billock, 2011-08-03 22:30:12]

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

http://codereview.chromium.org/7553020/diff/12002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:35: /* 96px is the suggested icon
size for Chrome apps */
The suggestion is "make it 128x128, but only use the inner 96x96". I'm not sure
it matters that much to what we'll end up using, though.

On 2011/08/03 22:17:36, Evan Stade wrote:
> this is almost correct: it's the minimum size we show an icon for for chrome
> apps. I think the suggested size is 128px (you'll notice the app images get
> expanded and shrunk as you change the amount of space available)

http://codereview.chromium.org/7553020/diff/12002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.css:43: -webkit-box-pack: center;
On 2011/08/03 22:17:36, Evan Stade wrote:
> nit: -webkit-foo goes at the very end

Done.

[commit-bot: I haz the power, 2011-08-04 01:54:48]

Can't apply patch for file chrome/browser/ui/webui/ntp/app_launcher_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/ntp/app_launcher_handler.cc
Hunk #1 FAILED at 101.
Hunk #2 succeeded at 137 (offset 15 lines).
Hunk #3 succeeded at 672 (offset 12 lines).
Hunk #4 succeeded at 700 (offset 12 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/ntp/app_launcher_handler.cc.rej

[commit-bot: I haz the power, 2011-08-04 16:11:46]

Can't apply patch for file chrome/browser/ui/webui/ntp/app_launcher_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/ntp/app_launcher_handler.cc
Hunk #1 FAILED at 101.
Hunk #2 succeeded at 137 (offset 15 lines).
Hunk #3 succeeded at 678 (offset 18 lines).
Hunk #4 succeeded at 706 (offset 18 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/ntp/app_launcher_handler.cc.rej

[commit-bot: I haz the power, 2011-08-08 18:11:14]

Can't apply patch for file chrome/browser/ui/webui/ntp/app_launcher_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/ntp/app_launcher_handler.cc
Hunk #1 FAILED at 101.
Hunk #2 succeeded at 137 (offset 15 lines).
Hunk #3 succeeded at 687 (offset 27 lines).
Hunk #4 succeeded at 715 (offset 27 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/ntp/app_launcher_handler.cc.rej

[commit-bot: I haz the power, 2011-08-08 22:56:42]

Change committed as 95899