Completing high dpi support for Instant Extended.

Completing high dpi support for Instant Extended.

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

[pedro (no code reviews), 2013-07-01 22:48:50]

Hey Samarth,

Could you please take a look at this CL? I had to split
https://codereview.chromium.org/15388002/ into two CLs and this is the second
part of it, which includes the SearchBox related changes for the high dpi
support in Instant Extended.

[samarth, 2013-07-19 21:50:06]

This is probably good as-is, but please ping once you've synced past your other
change and I'll take a look.

Thanks,
Samarth

[pedro (no code reviews), 2013-07-23 21:35:12]

On 2013/07/19 21:50:06, samarth wrote:
> This is probably good as-is, but please ping once you've synced past your
other
> change and I'll take a look.
> 
> Thanks,
> Samarth

Ping.

[samarth, 2013-07-24 00:11:04]

Looks good! Just a few more comments.

Thanks,
Samarth

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:98: function
GenerateThumbnailURL(renderViewId, rid) {
GenerateFaviconURL

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

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:54: bool
GetInstantRestrictedIDFromPath(int render_view_id,
Please add a comment with that this does.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:129: return
internal::GetInstantRestrictedIDFromPath(render_view_id, path, id);
You're already in namespace internal so the qualifier is not necessary.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:174: std::string item_url = "";
= "" is unnecessary.

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

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:125:
obj->Set(v8::String::New("faviconUrl"),
No reason to fill this out here, right?

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:126:
GenerateFaviconURL(render_view_id, restricted_id));
You can kill this function too.

[pedro (no code reviews), 2013-07-24 01:10:55]

Hey Samarth, please take another look.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:98: function
GenerateThumbnailURL(renderViewId, rid) {
On 2013/07/24 00:11:05, samarth wrote:
> GenerateFaviconURL

Done.

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

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:54: bool
GetInstantRestrictedIDFromPath(int render_view_id,
On 2013/07/24 00:11:05, samarth wrote:
> Please add a comment with that this does.

Done.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:129: return
internal::GetInstantRestrictedIDFromPath(render_view_id, path, id);
On 2013/07/24 00:11:05, samarth wrote:
> You're already in namespace internal so the qualifier is not necessary.

Done.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:174: std::string item_url = "";
On 2013/07/24 00:11:05, samarth wrote:
> = "" is unnecessary.

Done.

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

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:125:
obj->Set(v8::String::New("faviconUrl"),
On 2013/07/24 00:11:05, samarth wrote:
> No reason to fill this out here, right?

Right, no reason to fill this out here, since we're filling it elsewhere.

https://codereview.chromium.org/18344010/diff/4001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:126:
GenerateFaviconURL(render_view_id, restricted_id));
On 2013/07/24 00:11:05, samarth wrote:
> You can kill this function too.

Done.

[samarth, 2013-07-24 17:58:46]

LGTM. Nice work!

[pedro (no code reviews), 2013-07-24 20:40:36]

Thanks Samarth! I just sync'ed and will submit the code right away.

[commit-bot: I haz the power, 2013-07-24 20:42:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/18344010/2...

[commit-bot: I haz the power, 2013-07-25 09:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/18344010/2...

[commit-bot: I haz the power, 2013-07-25 09:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/18344010/2...

[commit-bot: I haz the power, 2013-07-25 16:22:52]

Change committed as 213643