alternate ntp: implement searchbox api for instant overlay to adopt themes

alternate ntp: implement searchbox api for instant overlay to adopt themes

- add SearchBoxExtensionWrapper::GetThemeBackgroundInfo
  * returns theme background info: color, image url, image horizontal alignment, image vertical alignment, image tiling and image height
  * color is always filled, the rest are only filled if the theme has a custom background image that overlay needs to draw
  * should only be called when overlay is in ntp mode
  * when theme changes in ntp mode, BrowserInstantController caches the theme info; if mode is ntp, event is dispatched to onthemechanged callback, which should call GetThemeBackgroundInfo

- add SearchBoxExtensionWrapper::GetThemeAreaHeight
  * returns the height of the area that the entire theme background image should fill up
  * should only be called when overlay is in ntp mode and theme background image is not top-aligned; top-aligned images are always painted from overlay's top-left corner, so this value is irrelevant and will be invalid
  * change of this height is monitored when BrowserView lays out its children, then updated and cached in BrowserInstantController; if above conditions are met, event is dispatched to onthemeareaheightchanged callback, which should call GetThemeAreaHeight

- theme area height is also sent to preview when theme changes, 'cos the height may have changed with the previous theme but wasn't updated to preview

- theme info and area height are also sent to preview when search mode changes to ntp, 'cos they may have changed during the previous mode but not updated to preview

BUG=160957
TEST=the web page needs to be implemented before this can be manually tested


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

[kuan, 2012-11-15 20:56:20]

this is a first-draft implementation of providing theme info to searchbox api. 
the impl is only on the chrome native side, the web side will be done by jeremy
after this cl lands, so searchbox_api.js only has definitions.

the changes in different files are related and not big, so it's probably easier
to look at everything.  thx.

[kuan, 2012-11-15 20:59:21]

because resizing of browser window is platform-dependent, this impl is only for
cros/win only.

[jeremycho, 2012-11-15 21:26:23]

On 2012/11/15 20:59:21, kuan wrote:
> because resizing of browser window is platform-dependent, this impl is only
for
> cros/win only.

Thanks for getting this ready so quickly!  I'll work on testing this from my
end.  Is there anything I need to do besides patching this in?   I guess the
theme image won't be accessible, but I can fake that for now.

[kuan, 2012-11-15 21:35:30]

On 2012/11/15 21:26:23, jeremycho wrote:
> On 2012/11/15 20:59:21, kuan wrote:
> > because resizing of browser window is platform-dependent, this impl is only
> for
> > cros/win only.
> 
> Thanks for getting this ready so quickly!  I'll work on testing this from my
> end.  Is there anything I need to do besides patching this in?   I guess the
> theme image won't be accessible, but I can fake that for now.

yes, i think u can try to patch set 2 in; u'll hv to change searchbox_api.js.

[dhollowa, 2012-11-15 22:00:33]

Redacting myself... others will have this covered better.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:99: ThemeBackgroundInfo();
ctor/dtor are not needed.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:102: // The background color in RGBA format.
nit: Elaborate on the format of this string please.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:108: // The image horizontal alignment.
ditto

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:111: // The image vertical alignment.
ditto

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:114: // The tiling of the image.
ditto

[kuan, 2012-11-15 22:19:15]

i've addressed dhollowa@'s comments in patch set 4; #5 is a typo fix.  ptal. 
thx.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:99: ThemeBackgroundInfo();
On 2012/11/15 22:00:33, dhollowa wrote:
> ctor/dtor are not needed.

Done.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:102: // The background color in RGBA format.
On 2012/11/15 22:00:33, dhollowa wrote:
> nit: Elaborate on the format of this string please.

Done.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:108: // The image horizontal alignment.
On 2012/11/15 22:00:33, dhollowa wrote:
> ditto

Done.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:111: // The image vertical alignment.
On 2012/11/15 22:00:33, dhollowa wrote:
> ditto

Done.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:114: // The tiling of the image.
On 2012/11/15 22:00:33, dhollowa wrote:
> ditto

Done.

[kuan, 2012-11-15 22:55:17]

fyi, patch set #6 is rebased to resolve conflicts that jeremy encountered when
patching the cl.

[jeremycho, 2012-11-16 00:08:41]

On 2012/11/15 22:55:17, kuan wrote:
> fyi, patch set #6 is rebased to resolve conflicts that jeremy encountered when
> patching the cl.

Great!  The API seems to be working on the GWS side.  The callbacks are being
registered and the values look reasonable.

[David Black, 2012-11-16 00:57:17]

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox/searchbox_api.js (right):

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox/searchbox_api.js:183:
this.__defineGetter__('themeAreaHeight', GetThemeAreaHeight);
Why isn't the theme area height part of the returned theme background info
object?

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:445:
info->Set(v8::String::New("color_rgba"),
These Javascript object properties should use camel-case, not underscores.

[kuan, 2012-11-16 01:10:03]

i've addressed dcblack's comments in patch set 7.  ptal.  thx.

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox/searchbox_api.js (right):

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox/searchbox_api.js:183:
this.__defineGetter__('themeAreaHeight', GetThemeAreaHeight);
On 2012/11/16 00:57:17, David Black wrote:
> Why isn't the theme area height part of the returned theme background info
> object?

the height changes a lot more: when browser window is resized, devtools is
opened/docked/undocked/closed, download shelf is opened/closed.
the theme changes a lot less: on initialization, when user manually changes it.
so jeremy suggested we do it in 2 separate api's.

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/5018/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:445:
info->Set(v8::String::New("color_rgba"),
On 2012/11/16 00:57:17, David Black wrote:
> These Javascript object properties should use camel-case, not underscores.

Done.

[kuan, 2012-11-16 01:59:51]

fyi, i've reinstated the ctor/dtor for ThemeBackgroundInfo in patch set 8, 'cos
clang doesn't build without them.

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
chrome/common/instant_types.h:99: ThemeBackgroundInfo();
On 2012/11/15 22:19:16, kuan wrote:
> On 2012/11/15 22:00:33, dhollowa wrote:
> > ctor/dtor are not needed.
> 
> Done.

actually, i need them.  clang doesn't build without them.

[jeremycho, 2012-11-16 04:06:16]

From our offline discussion - the way I'm calculating the theme height is a
little hacky (loading it into an image which doesn't actually get rendered).  If
it's not too difficult, I think it would be cleaner if you can pass it in
ThemeBackgroundInfo.

On 2012/11/16 01:59:51, kuan wrote:
> fyi, i've reinstated the ctor/dtor for ThemeBackgroundInfo in patch set 8,
'cos
> clang doesn't build without them.
> 
>
https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_types.h
> File chrome/common/instant_types.h (right):
> 
>
https://codereview.chromium.org/11413018/diff/4001/chrome/common/instant_type...
> chrome/common/instant_types.h:99: ThemeBackgroundInfo();
> On 2012/11/15 22:19:16, kuan wrote:
> > On 2012/11/15 22:00:33, dhollowa wrote:
> > > ctor/dtor are not needed.
> > 
> > Done.
> 
> actually, i need them.  clang doesn't build without them.

[kuan, 2012-11-16 04:28:39]

i've addressed jeremy's comments in patch set 9 - add image height to
ThemeBackgroundInfo.
jeremy, pls note that when u patch this set in, u need to modify the field names
of ThemeBackgroundInfo from underscore to camel case in ur gws code.

[jeremycho, 2012-11-16 06:07:37]

Thanks kuan.  I've got a demo that you can test using
--instant-url=http://murakami.mtv.corp.google.com:9111/webhp?espv=1

Because GWS can't access the themes backgrounds, I've hardcoded it to only
support cirque du soleli (top align), beautiful landscape (bottom), and Ferrari
F1 (center).  

There are still bugs I need to iron out.  Also, if you resize the browser height
quickly, you'll notice a delay between when GWS and Chrome update the
background.  I don't know if there's much we can do about that.

On 2012/11/16 04:28:39, kuan wrote:
> i've addressed jeremy's comments in patch set 9 - add image height to
> ThemeBackgroundInfo.
> jeremy, pls note that when u patch this set in, u need to modify the field
names
> of ThemeBackgroundInfo from underscore to camel case in ur gws code.

[sky, 2012-11-19 15:07:31]

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:39:
"-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?%s) 1x)";
nit: indent 2 more.

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view.cc:1866: prev_content_height !=
contents_container_->height()) {
Why do you need to check pref_content_height? Can't you do this always?

[kuan, 2012-11-19 16:59:41]

i've addressed all comments in patch set 11, which unfortunately also includes
rebasing.  ptal.  thx.

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:39:
"-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?%s) 1x)";
On 2012/11/19 15:07:31, sky wrote:
> nit: indent 2 more.

Done.

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/11413018/diff/4008/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view.cc:1866: prev_content_height !=
contents_container_->height()) {
On 2012/11/19 15:07:31, sky wrote:
> Why do you need to check pref_content_height? Can't you do this always?

if page is ntp and theme image is not top-aligned,
BrowserInstantController::SetContentHeight would send the event to the renderer
process, which would then dispatch it to the js callback.  since the value is
not dispatched everytime and the last dispatched value is not remembered, i
can't check if value has changed in SetContentHeight b4 dispatching it.  that
means i'll hv to check it here - won't dispatching the same value every time be
an overkill?

[kuan, 2012-11-19 17:03:12]

hi chris, cld u provide owner's approval for render_messages.h?  thx!

[sky, 2012-11-19 17:26:02]

Makes sense, LGTM

[sreeram, 2012-11-19 18:12:01]

LGTM with nits

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:565: if (loader_.get())
Use "if (GetPreviewContents())" instead of "if (loader_.get())", here and on
line 570 below. The rationale is that, in the off chance that the loader has
released its preview contents and we get here (say because of something
browser_->CommitInstant() does), it won't crash.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:707:
browser_->UpdateThemeRelatedInfoForPreview();
Nit: "related" seems unnecessary. Just "UpdateThemeInfoForPreview".

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:142: void OnThemeChanged(const
ThemeBackgroundInfo& theme_info);
Nit: Drop the "On". Just ThemeChanged() and "ThemeAreaHeightChanged" are clear
enough, I think.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:299:
instant()->OnThemeChanged(theme_info_);
Nit: instant() -> instant_ (it works on both lines above).

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:325:
instant()->OnThemeAreaHeightChanged(theme_area_height_);
Why bother with the notify variable? Why not just this:

theme_area_height_ = height;
if (!instant_ || browser_->... || theme_info_.image_url... ||
theme_image_alignment & ThemeService::ALIGN_TOP)
  return;

instant_->OnThemeAreaHeightChanged(theme_area_height_);

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_type...
chrome/common/instant_types.h:96: // Specifies information required for NTP
instant overlay to adopt the browser's
Nit: instant -> Instant

https://codereview.chromium.org/11413018/diff/8041/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/common/render_messa...
chrome/common/render_messages.h:178: IPC_STRUCT_TRAITS_END()
Move this one block down, to preserve alpha order.

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:215: active_tab_is_ntp_ = false;
Clear the newly added variables here?

[palmer, 2012-11-19 18:26:51]

The basic idea makes sense, but the string-based implementation concerns me from
a security perspective (XSS and CSS injection from an untrustworthy source). We
need to write IPC endpoints as if the other side was trying to trick us. I think
you can make this code very safe and also more efficient, with not too much
work, by making the theme info struct contain mostly primitive data types, and
modifying the code that uses it to suit.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:39:
"-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?%s) 1x)";
String interpolation is generally a security danger-sign. See below for details.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:252:
base::DoubleToString(SkColorGetA(background_color) / 255.0).
This is you creating an honest message, but generally we consider that IPCs
could come from any source, potentially malicious. CSS injection could happen to
receiver of a message like this.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:258:
base::StringPrintf(kCSSBackgroundImageFormat,
For example:

-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123)
1x);evil-css-here(url:javascript:blah
blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) 1x)

I.e. the injection string would be "abc123)
1x);evil-css-here(url:javascript:blah
blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) ".

(I don't know if that is the best way to neutralize the trailing "1x)" that
kCSSBackgroundImageFormat specifies, but hopefully you see the point.)

Getting JavaScript execution from CSS injection is harder than regular XSS, but
it has been possible. Generally, we treat CSS as code and don't like to
interpolate inputs from untrustworthy sources into CSS.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:272:
theme_info_.image_horizontal_alignment = UTF8ToUTF16(alignment);
These conversions are another efficiency reason to prefer primitive types when
packing and unpacking these structs and IPCs.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:275: if (theme_image_alignment_&
ThemeService::ALIGN_TOP)
Oh, you've already got enums for this. So ignore my example enums, and use these
in the IPC messages, packed by the sender here:

if (theme_image_alignment_& ThemeService::ALIGN_TOP)
    theme_info_.image_vertical_alignment = ThemeService::ALIGN_TOP;
else if ( /* bottom */)
    // ...
else
    // CENTER...

(I.e. this packing code would become simpler and smaller)

...and unpacked and interpreted by the receiver:

switch (theme_background_info.image_vertical_alignment) {
    case ThemeService::ALIGN_BOTTOM:
        alignment = ThemeService::kAlignmentBottom;
    //...
    default:
        // rather than explode, use ALIGN_CENTER as default
}

It is now safe to interpolate the static strings ThemeService::kAlignment* into
your CSS, because the attacker cannot influence them. We selected which one to
use from a simple switch statement with a reasonable default. We did not use an
arbitrary string from across the IPC boundary.

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_type...
chrome/common/instant_types.h:102: // The background color in RGBA format
"rgba(R,G,B,A)" where R, G and B are
The use of string16s and the formatting requirements make me think you are going
to interpolate these strings directly into CSS/JS. Since these values might come
from an untrustworthy source, That Would Be Bad: arbitrary URLs, XSS and CSS
injection, giant meaningless strings, strings that induce parse failures, and so
on.

It seems to me it would better (more efficient, strongly typed, no parsers
needed, smaller structs, smaller IPC messages) to make most of this struct's
members be ints and enums. Right? For example:

int color_r;
int color_g;
int color_b;
double color_a;

// For selecting the background image. Must contain only alphabetic characters
[or whatever]; write a validator and call it before using it). Not string16;
part of the validation is making sure it is ASCII.
string theme_id;

enum HorizontalAlignment {
    LEFT,
    RIGHT,
    CENTER
};
// uint8 for space efficiency.
uint8 image_horizontal_alignment;

enum VerticalAlignment {
    // ...
};
uint8 image_vertical_alignment;

enum ImageTiling {
    // ...
};
uint8 image_tiling;

// Unsigned because negative heights are meaningless; as small as possible for
space efficiency.
uint16 image_height;

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:82: return theme_info_;
Validate the theme_info_ struct's members here. The ints and uint8s and such
require only a sane default: case in a switch, but the theme_id string needs to
be truly theme ID-shaped.

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:413:
info->Set(v8::String::New("colorRgba"),
Right, here is where your switch/case statements on ints would go. Validate the
theme_id here if it has not already been validated.

[kuan, 2012-11-19 22:06:07]

i've addressed sreeram's and chris's commens in patch set 12.  ptal.  thx.

chris, i did as per ur suggestion.  however, i encounter a presubmit error when
i try to upload the cl:
Illegal include: "chrome/browser/themes/theme_service.h"
Because of "-chrome" from chrome's include_rules.

i've a feeling i can't include chrome/browser/* headers in chrome/renderer
files??
does this i mean i have to repeat the definitions of ThemeService alignment and
tiling enum and string values for IPC?

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:565: if (loader_.get())
On 2012/11/19 18:12:02, sreeram wrote:
> Use "if (GetPreviewContents())" instead of "if (loader_.get())", here and on
> line 570 below. The rationale is that, in the off chance that the loader has
> released its preview contents and we get here (say because of something
> browser_->CommitInstant() does), it won't crash.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:707:
browser_->UpdateThemeRelatedInfoForPreview();
On 2012/11/19 18:12:02, sreeram wrote:
> Nit: "related" seems unnecessary. Just "UpdateThemeInfoForPreview".

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:142: void OnThemeChanged(const
ThemeBackgroundInfo& theme_info);
On 2012/11/19 18:12:02, sreeram wrote:
> Nit: Drop the "On". Just ThemeChanged() and "ThemeAreaHeightChanged" are clear
> enough, I think.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:39:
"-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?%s) 1x)";
On 2012/11/19 18:26:51, Chris P. wrote:
> String interpolation is generally a security danger-sign. See below for
details.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:252:
base::DoubleToString(SkColorGetA(background_color) / 255.0).
On 2012/11/19 18:26:51, Chris P. wrote:
> This is you creating an honest message, but generally we consider that IPCs
> could come from any source, potentially malicious. CSS injection could happen
to
> receiver of a message like this.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:258:
base::StringPrintf(kCSSBackgroundImageFormat,
On 2012/11/19 18:26:51, Chris P. wrote:
> For example:
> 
> -webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123)
> 1x);evil-css-here(url:javascript:blah
> blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) 1x)
> 
> I.e. the injection string would be "abc123)
> 1x);evil-css-here(url:javascript:blah
> blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) ".
> 
> (I don't know if that is the best way to neutralize the trailing "1x)" that
> kCSSBackgroundImageFormat specifies, but hopefully you see the point.)
> 
> Getting JavaScript execution from CSS injection is harder than regular XSS,
but
> it has been possible. Generally, we treat CSS as code and don't like to
> interpolate inputs from untrustworthy sources into CSS.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:272:
theme_info_.image_horizontal_alignment = UTF8ToUTF16(alignment);
On 2012/11/19 18:26:51, Chris P. wrote:
> These conversions are another efficiency reason to prefer primitive types when
> packing and unpacking these structs and IPCs.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:275: if (theme_image_alignment_&
ThemeService::ALIGN_TOP)
i did as per ur suggestion.  it works, EXCEPT when i try to upload the cl, i get
a presubmit error:
Illegal include: "chrome/browser/themes/theme_service.h"
Because of "-chrome" from chrome's include_rules.

i've a feeling i can't include chrome/browser files in chrome/renderer files??
does this i mean i have to repeat the definitions of ThemeService enum values
for IPC?

On 2012/11/19 18:26:51, Chris P. wrote:
> Oh, you've already got enums for this. So ignore my example enums, and use
these
> in the IPC messages, packed by the sender here:
> 
> if (theme_image_alignment_& ThemeService::ALIGN_TOP)
>     theme_info_.image_vertical_alignment = ThemeService::ALIGN_TOP;
> else if ( /* bottom */)
>     // ...
> else
>     // CENTER...
> 
> (I.e. this packing code would become simpler and smaller)
> 
> ...and unpacked and interpreted by the receiver:
> 
> switch (theme_background_info.image_vertical_alignment) {
>     case ThemeService::ALIGN_BOTTOM:
>         alignment = ThemeService::kAlignmentBottom;
>     //...
>     default:
>         // rather than explode, use ALIGN_CENTER as default
> }
> 
> It is now safe to interpolate the static strings ThemeService::kAlignment*
into
> your CSS, because the attacker cannot influence them. We selected which one to
> use from a simple switch statement with a reasonable default. We did not use
an
> arbitrary string from across the IPC boundary.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:299:
instant()->OnThemeChanged(theme_info_);
On 2012/11/19 18:12:02, sreeram wrote:
> Nit: instant() -> instant_ (it works on both lines above).

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:325:
instant()->OnThemeAreaHeightChanged(theme_area_height_);
On 2012/11/19 18:12:02, sreeram wrote:
> Why bother with the notify variable? Why not just this:
> 
> theme_area_height_ = height;
> if (!instant_ || browser_->... || theme_info_.image_url... ||
> theme_image_alignment & ThemeService::ALIGN_TOP)
>   return;
> 
> instant_->OnThemeAreaHeightChanged(theme_area_height_);

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_type...
chrome/common/instant_types.h:96: // Specifies information required for NTP
instant overlay to adopt the browser's
On 2012/11/19 18:12:02, sreeram wrote:
> Nit: instant -> Instant

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/common/instant_type...
chrome/common/instant_types.h:102: // The background color in RGBA format
"rgba(R,G,B,A)" where R, G and B are
i tried w/ uint8, but it's not supported in the ipc macros.

On 2012/11/19 18:26:51, Chris P. wrote:
> The use of string16s and the formatting requirements make me think you are
going
> to interpolate these strings directly into CSS/JS. Since these values might
come
> from an untrustworthy source, That Would Be Bad: arbitrary URLs, XSS and CSS
> injection, giant meaningless strings, strings that induce parse failures, and
so
> on.
> 
> It seems to me it would better (more efficient, strongly typed, no parsers
> needed, smaller structs, smaller IPC messages) to make most of this struct's
> members be ints and enums. Right? For example:
> 
> int color_r;
> int color_g;
> int color_b;
> double color_a;
> 
> // For selecting the background image. Must contain only alphabetic characters
> [or whatever]; write a validator and call it before using it). Not string16;
> part of the validation is making sure it is ASCII.
> string theme_id;
> 
> enum HorizontalAlignment {
>     LEFT,
>     RIGHT,
>     CENTER
> };
> // uint8 for space efficiency.
> uint8 image_horizontal_alignment;
> 
> enum VerticalAlignment {
>     // ...
> };
> uint8 image_vertical_alignment;
> 
> enum ImageTiling {
>     // ...
> };
> uint8 image_tiling;
> 
> // Unsigned because negative heights are meaningless; as small as possible for
> space efficiency.
> uint16 image_height;

https://codereview.chromium.org/11413018/diff/8041/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/common/render_messa...
chrome/common/render_messages.h:178: IPC_STRUCT_TRAITS_END()
On 2012/11/19 18:12:02, sreeram wrote:
> Move this one block down, to preserve alpha order.

Done.

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:82: return theme_info_;
On 2012/11/19 18:26:51, Chris P. wrote:
> Validate the theme_info_ struct's members here. The ints and uint8s and such
> require only a sane default: case in a switch, but the theme_id string needs
to
> be truly theme ID-shaped.

i validate everything in SearchBoxExtensions.

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:215: active_tab_is_ntp_ = false;
On 2012/11/19 18:12:02, sreeram wrote:
> Clear the newly added variables here?

Done.  also added to initializer list.

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/8041/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:413:
info->Set(v8::String::New("colorRgba"),
On 2012/11/19 18:26:51, Chris P. wrote:
> Right, here is where your switch/case statements on ints would go. Validate
the
> theme_id here if it has not already been validated.

Done, except i have the problem that i mentioned earlier abt including
theme_service.h in this file.

[palmer, 2012-11-19 22:23:19]

Looks pretty good, thank you! One last format string issue, and you need a
validator for the Theme ID, but it's just about ready, from a security
perspective.

You may have to duplicate the constants since you can't include the header —
however, perhaps the constants can be defined in a header at a higher level
(e.g. chrome/foo.h) that both calling sites can legally include? Try that first
before duplicating the constants.

https://codereview.chromium.org/11413018/diff/55/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/55/chrome/common/instant_types....
chrome/common/instant_types.h:109: int color_a;
Wasn't A supposed to be a double between 0 and 1? (Having it be an int or a
double is fine by me for security purposes, of course. Do whatever works.)

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:440:
base::DoubleToString(theme_info.color_a / 255.0).c_str())));
Seems strange to convert this to a string. Can you change
kCSSBackgroundColorFormat to have %f instead of %s, and then just do
theme_info.color_a / 255.0 (without calling DoubleToString)?

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:450:
theme_info.theme_id.c_str());
Where does this get validated to ensure that it is truly theme ID-shaped?

Your calling code creates it correctly by calling GetThemeID, but attackers are
not necesarily constrained to sending only real theme IDs.

[sreeram, 2012-11-19 22:41:33]

On Mon, Nov 19, 2012 at 2:23 PM,  <palmer@chromium.org> wrote:
> You may have to duplicate the constants since you can't include the header —
> however, perhaps the constants can be defined in a header at a higher level
> (e.g. chrome/foo.h) that both calling sites can legally include? Try that
> first
> before duplicating the constants.

This is exactly what chrome/common/instant_types.h is for.

[kuan, 2012-11-20 00:12:44]

i've addressed all comments in patch set 14 (using sreeram's advice for the
constants).  ptal.   thx.

i didn't move the constants out of  ThemeService 'cos they're used in other
places, and they're too many to change in this cl.

https://codereview.chromium.org/11413018/diff/55/chrome/common/instant_types.h
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/11413018/diff/55/chrome/common/instant_types....
chrome/common/instant_types.h:109: int color_a;
On 2012/11/19 22:23:20, Chris P. wrote:
> Wasn't A supposed to be a double between 0 and 1? (Having it be an int or a
> double is fine by me for security purposes, of course. Do whatever works.)

the original type is an int, which i convert to double when i format the string.

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:440:
base::DoubleToString(theme_info.color_a / 255.0).c_str())));
On 2012/11/19 22:23:20, Chris P. wrote:
> Seems strange to convert this to a string. Can you change
> kCSSBackgroundColorFormat to have %f instead of %s, and then just do
> theme_info.color_a / 255.0 (without calling DoubleToString)?

the comment just above base::StringPrintf explains why DoubleToString is used.

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:450:
theme_info.theme_id.c_str());
On 2012/11/19 22:23:20, Chris P. wrote:
> Where does this get validated to ensure that it is truly theme ID-shaped?
> 
> Your calling code creates it correctly by calling GetThemeID, but attackers
are
> not necesarily constrained to sending only real theme IDs.

in the line just above StringPrintf, i call IdIsValid which validates the
theme_id.
ThemeService uses the id() of the theme extension as the theme id, so i use
Extension::IdIsValid to validate the theme id.

[kuan, 2012-11-20 15:54:32]

fyi, patch set 15 is rebased to resolve conflicts for instant files.

[sreeram, 2012-11-20 17:37:58]

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
Wait, why do we need to send the theme info the preview _every_ time the mode
changes to NTP? I thought we only need to send if either the theme info changes
(Observe() is called) or if Instant creates a new preview
(UpdateThemeInfoForPreview() is called).

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.h:34: public
content::NotificationObserver {
Nit: Swap the two lines above to keep alpha order.

[kuan, 2012-11-20 17:52:24]

i've addressed sreeram's comments in patch set 16.

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
On 2012/11/20 17:37:58, sreeram wrote:
> Wait, why do we need to send the theme info the preview _every_ time the mode
> changes to NTP? I thought we only need to send if either the theme info
changes
> (Observe() is called) or if Instant creates a new preview
> (UpdateThemeInfoForPreview() is called).

i don't send the theme-related info when the mode is not NTP (@255, @271), so if
theme or theme area height changes during non-ntp modes, instant overlay doesn't
get updated, which means it needs the latest theme-related info when mode
switches to NTP.

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.h:34: public
content::NotificationObserver {
On 2012/11/20 17:37:58, sreeram wrote:
> Nit: Swap the two lines above to keep alpha order.

Done.

[sreeram, 2012-11-20 17:55:10]

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
On 2012/11/20 17:52:24, kuan wrote:
> i don't send the theme-related info when the mode is not NTP (@255, @271), so
if
> theme or theme area height changes during non-ntp modes, instant overlay
doesn't
> get updated, which means it needs the latest theme-related info when mode
> switches to NTP.

I think that's wasteful. The common case will not be the theme changing. The
common case will be the user creating an NTP. It's wasteful to send these huge
data structures over to the preview when in fact, nothing has changed. Why not
send the info to the preview when the theme changes? The preview already knows
when it's supposed to show the NTP, so it can cache the theme info until it
needs to use it.

[kuan, 2012-11-20 18:07:35]

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
On 2012/11/20 17:55:11, sreeram wrote:
> On 2012/11/20 17:52:24, kuan wrote:
> > i don't send the theme-related info when the mode is not NTP (@255, @271),
so
> if
> > theme or theme area height changes during non-ntp modes, instant overlay
> doesn't
> > get updated, which means it needs the latest theme-related info when mode
> > switches to NTP.
> 
> I think that's wasteful. The common case will not be the theme changing. The
> common case will be the user creating an NTP. It's wasteful to send these huge
> data structures over to the preview when in fact, nothing has changed. Why not
> send the info to the preview when the theme changes? The preview already knows
> when it's supposed to show the NTP, so it can cache the theme info until it
> needs to use it.

the structure i'm sending over consists of 4 short words, 1 int, and a url.
if i were to do as u say, the theme area height would be an overkill to send
when mode is not ntp, 'cos that happens while the window is being resized.
shld i send the theme info as long as theme changes, but the theme area height
only when mode switches to ntp?

[sreeram, 2012-11-20 18:16:28]

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
On 2012/11/20 18:07:36, kuan wrote:
> the structure i'm sending over consists of 4 short words, 1 int, and a url.

Ok, never mind then. (I thought incorrectly that you were sending the entire
background image as a string.)

However, do swap the lines above so that you send the theme info _before_
calling instant_.SearchModeChanged(). This way, the preview can be sure that
whenever it receives a modechanged to NTP, it can immediately layout based on
the last received theme info.

[sreeram, 2012-11-20 18:39:00]

LGTM

https://codereview.chromium.org/11413018/diff/4023/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/4023/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:661:
browser_->UpdateThemeInfoForPreview();
Likewise, swap these two lines as well.

[kuan, 2012-11-20 18:50:47]

i've addressed sreeram's comments in patch set 17.
i've also fixed to not set theme info fields if there's no theme background
image, so that jeremy can simply check for "undefined" in his gws js code (we've
discussed this offline).

ptal.  thx.

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/1030/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:169:
UpdateThemeInfoForPreview();
On 2012/11/20 18:16:29, sreeram wrote:
> On 2012/11/20 18:07:36, kuan wrote:
> > the structure i'm sending over consists of 4 short words, 1 int, and a url.
> 
> Ok, never mind then. (I thought incorrectly that you were sending the entire
> background image as a string.)
> 
> However, do swap the lines above so that you send the theme info _before_
> calling instant_.SearchModeChanged(). This way, the preview can be sure that
> whenever it receives a modechanged to NTP, it can immediately layout based on
> the last received theme info.

Done.

https://codereview.chromium.org/11413018/diff/4023/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11413018/diff/4023/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:661:
browser_->UpdateThemeInfoForPreview();
On 2012/11/20 18:39:00, sreeram wrote:
> Likewise, swap these two lines as well.

Done.

[palmer, 2012-11-20 19:55:30]

Thanks! LGTM.

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:440:
base::DoubleToString(theme_info.color_a / 255.0).c_str())));
> > theme_info.color_a / 255.0 (without calling DoubleToString)?
> 
> the comment just above base::StringPrintf explains why DoubleToString is used.

But I had thought that %f followed the same locale rules? According to the
printf(3) man page, it does (if you use e.g. %'.f), but maybe base::StringPrintf
doesn't follow that convention.

I think it's OK, since at this point the potentially-untrustworthy input is just
a float.

https://codereview.chromium.org/11413018/diff/55/chrome/renderer/searchbox/se...
chrome/renderer/searchbox/searchbox_extension.cc:450:
theme_info.theme_id.c_str());
> in the line just above StringPrintf, i call IdIsValid which validates the
> theme_id.
> ThemeService uses the id() of the theme extension as the theme id, so i use
> Extension::IdIsValid to validate the theme id.

Sorry, my bad. :)

[commit-bot: I haz the power, 2012-11-20 19:58:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11413018/4025

[commit-bot: I haz the power, 2012-11-20 21:16:26]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-20 21:19:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11413018/4025

[commit-bot: I haz the power, 2012-11-20 23:41:10]

Change committed as 168908

[katsmeowka.kw48, 2015-10-15 12:03:05]

katsmeowka.kw48@gmail.com changed reviewers:
+ katsmeowka.kw48@gmail.com

[brmeyerswork, 2016-11-15 08:23:49]

brmeyerswork@gmail.com changed reviewers:
+ brmeyerswork@gmail.com

[brmeyerswork, 2016-11-15 08:23:50]

lgtm