Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10809063:
Adding Javascript support for the Extended Searchbox API. (Closed)
|
Created:
8 years, 5 months ago by Shishir Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, David Black, tonyg Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding Javascript support for the Extended Searchbox API.
This CL only adds the IPCs and renderer side hooks for the extended API, but does not make the API functional as the browser side changes are not implemented. They will be implemented in a future CL which will be flag protected.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151642
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixing compile issue on android #Patch Set 3 : Remove test for older API, removed in this CL. #
Total comments: 10
Patch Set 4 : Addressing sky's comments. #
Total comments: 3
Patch Set 5 : Renaming InstantSuggestionParts to InstantAutocompleteResults. #Patch Set 6 : Rebasing to incorporate Sreeram's changes. #Patch Set 7 : Adding missing file from last upload. #
Total comments: 30
Patch Set 8 : Addressing Sreeram's changes and some naming changes. #Patch Set 9 : Syncing to pickup moved searchbox code. #
Total comments: 14
Patch Set 10 : Addressing Sreeram's comments. #Patch Set 11 : Fixing Windows Build. #
Total comments: 10
Patch Set 12 : Addressing Sreeram's comments. #Patch Set 13 : Fixing string conversion. #
Total comments: 2
Patch Set 14 : Removing clear method. #Messages
Total messages: 41 (0 generated)
Please take a look.
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { Access to these new functions should be guarded behind --enable-instant-extended-api. http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:407: v8::Handle<v8::Value> SearchBoxExtensionWrapper::SetSuggestions( It would probably be best to make a new ExtendedSetSuggestions function that is used if --enable-instant-extended-api is true, and just use the old code otherwise. We don't want to accidentally break existing Chrome-Instant users. (You can do the diversion up in GetNativeFunction.)
Adding sky for: chrome/common/.. changes. CCing tonyg for removal of older searchbox api. Please take a look.
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { On 2012/07/24 00:28:40, dcblack wrote: > Access to these new functions should be guarded behind > --enable-instant-extended-api. It's painful to guard them properly since it introduces a dependency on chrome/browser/ui/search (for IsInstantExtendedAPIEnabled), which is not allowed here in chrome/renderer. It's not necessary to guard them, since these methods will only work with a recognized Instant provider. Since the only Instant provider right now is Google, and since it doesn't misuse these methods, I think we are safe even without the guards. http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:407: v8::Handle<v8::Value> SearchBoxExtensionWrapper::SetSuggestions( On 2012/07/24 00:28:40, dcblack wrote: > It would probably be best to make a new ExtendedSetSuggestions function that is > used if --enable-instant-extended-api is true, and just use the old code > otherwise. We don't want to accidentally break existing Chrome-Instant users. > (You can do the diversion up in GetNativeFunction.) Same reason as before with respect to the guard. Besides, I think it's easy to make sure that this function works fine with both old and new Instant APIs.
http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.cc File chrome/common/instant_types.cc (right): http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.cc:7: InstantNativeSuggestionsParts::InstantNativeSuggestionsParts() { Make order match header (meaning InstantSuggestion should be first). And initialize is_search and relevance. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.cc:13: InstantSuggestion::InstantSuggestion() : behavior(INSTANT_COMPLETE_NOW), wrap this to next line (like you did at 20). http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:41: InstantSuggestion(const std::string& in_text, nuke in_ from all these. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:69: // The relevance score of this match. Document what 'relevance' means here, and in particular what the range of this is. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:78: // As a percentage of the size of the containing (parent) view. size or height?
http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.cc File chrome/common/instant_types.cc (right): http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.cc:7: InstantNativeSuggestionsParts::InstantNativeSuggestionsParts() { On 2012/07/24 21:14:32, sky wrote: > Make order match header (meaning InstantSuggestion should be first). And > initialize is_search and relevance. Done. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.cc:13: InstantSuggestion::InstantSuggestion() : behavior(INSTANT_COMPLETE_NOW), On 2012/07/24 21:14:32, sky wrote: > wrap this to next line (like you did at 20). Done. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:41: InstantSuggestion(const std::string& in_text, On 2012/07/24 21:14:32, sky wrote: > nuke in_ from all these. Done. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:69: // The relevance score of this match. On 2012/07/24 21:14:32, sky wrote: > Document what 'relevance' means here, and in particular what the range of this > is. Mentioned that it is the same relevance stored in AutocompleteMatch. http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... chrome/common/instant_types.h:78: // As a percentage of the size of the containing (parent) view. On 2012/07/24 21:14:32, sky wrote: > size or height? Can be height or width. Made clear in comments.
On Tue, Jul 24, 2012 at 2:31 PM, <shishir@chromium.org> wrote: > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.cc > File chrome/common/instant_types.cc (right): > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... > chrome/common/instant_types.cc:7: > InstantNativeSuggestionsParts::InstantNativeSuggestionsParts() { > On 2012/07/24 21:14:32, sky wrote: >> >> Make order match header (meaning InstantSuggestion should be first). > > And >> >> initialize is_search and relevance. > > > Done. > > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... > chrome/common/instant_types.cc:13: > InstantSuggestion::InstantSuggestion() : behavior(INSTANT_COMPLETE_NOW), > On 2012/07/24 21:14:32, sky wrote: >> >> wrap this to next line (like you did at 20). > > > Done. > > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types.h > File chrome/common/instant_types.h (right): > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... > chrome/common/instant_types.h:41: InstantSuggestion(const std::string& > in_text, > On 2012/07/24 21:14:32, sky wrote: >> >> nuke in_ from all these. > > > Done. > > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... > chrome/common/instant_types.h:69: // The relevance score of this match. > On 2012/07/24 21:14:32, sky wrote: >> >> Document what 'relevance' means here, and in particular what the range > > of this >> >> is. > > > Mentioned that it is the same relevance stored in AutocompleteMatch. > > > http://codereview.chromium.org/10809063/diff/2004/chrome/common/instant_types... > chrome/common/instant_types.h:78: // As a percentage of the size of the > containing (parent) view. > On 2012/07/24 21:14:32, sky wrote: >> >> size or height? > > > Can be height or width. Made clear in comments. Does width really make sense here? I thought it was always height, but I admit I could completely wrong. -Scott
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { Then that guarding code should be moved out to a common location. We can't expose things like GetSessionContext to anyone, us included, before we resolve the security questions around them. Also, remember that Bing, Yandex, or Baidu could implement this API on their search pages tomorrow and it would just start working, and they would have access to private user data through these functions. Not good. On 2012/07/24 18:09:22, sreeram wrote: > On 2012/07/24 00:28:40, dcblack wrote: > > Access to these new functions should be guarded behind > > --enable-instant-extended-api. > > It's painful to guard them properly since it introduces a dependency on > chrome/browser/ui/search (for IsInstantExtendedAPIEnabled), which is not allowed > here in chrome/renderer. > > It's not necessary to guard them, since these methods will only work with a > recognized Instant provider. Since the only Instant provider right now is > Google, and since it doesn't misuse these methods, I think we are safe even > without the guards.
On 2012/07/24 22:26:02, sky wrote:
> Does width really make sense here? I thought it was always height, but
> I admit I could completely wrong.
We do want to be able to set the width as well. The height-setting is needed for
two upcoming features ("zero suggest" and "URL previewing"), but we are looking
further into the future where we'll be adding "back to search", which will set
the preview width. If you are more comfortable, we can call this
InstantHeightUnits for now. When we eventually add back-to-search, we can rename
the enum to InstantSizeUnits to cover both width and height.
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { On 2012/07/24 22:34:56, dcblack wrote: > We can't > expose things like GetSessionContext to anyone, us included, before we resolve > the security questions around them. Agreed. However, this CL doesn't expose the session context. The parts of InstantLoader that actually supply the values for session context are absent. When we fill in those gaps, we'll be sure to either flag guard them (which would be easy, since that's all in chrome/browser/... land) and/or resolve the security questions. > Also, remember that Bing, Yandex, or Baidu > could implement this API on their search pages tomorrow and it would just start > working, and they would have access to private user data through these > functions. Not good. As above, this CL doesn't actually provide any of the private data yet. Apart from that, there's also the fact that Chrome currently ships with only one Instant-supporting engine, which is Google. Since there's no UI to set or change the instant_url for any engine, this means that it can't actually be used by Bing, Yandex, etc.
On Tue, Jul 24, 2012 at 3:50 PM, <sreeram@chromium.org> wrote: > On 2012/07/24 22:26:02, sky wrote: >> >> Does width really make sense here? I thought it was always height, but >> I admit I could completely wrong. > > > We do want to be able to set the width as well. The height-setting is needed > for > two upcoming features ("zero suggest" and "URL previewing"), but we are > looking > further into the future where we'll be adding "back to search", which will > set > the preview width. If you are more comfortable, we can call this > InstantHeightUnits for now. When we eventually add back-to-search, we can > rename > the enum to InstantSizeUnits to cover both width and height. Leave it as is. -Scott
http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { 'native' is a bit confusing here. I think this should be InstantHostSuggestionParts or event InstantOmniboxSuggestionParts.
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { From what I can tell from the GetSessionContext function below, it doesn't depend on any loader code at all and should just work as-is. Am I missing something about how that code works? Anyway, flag-guarding exposing the functions in the first place seems much safer and more correct, and furthermore we'll need access that that flag in this file anyway in the future when we need to load the new version of the JS below in SearchBoxExtension::Get(). On 2012/07/24 22:56:27, sreeram wrote: > On 2012/07/24 22:34:56, dcblack wrote: > > We can't > > expose things like GetSessionContext to anyone, us included, before we resolve > > the security questions around them. > > Agreed. However, this CL doesn't expose the session context. The parts of > InstantLoader that actually supply the values for session context are absent. > When we fill in those gaps, we'll be sure to either flag guard them (which would > be easy, since that's all in chrome/browser/... land) and/or resolve the > security questions. > > > Also, remember that Bing, Yandex, or Baidu > > could implement this API on their search pages tomorrow and it would just > start > > working, and they would have access to private user data through these > > functions. Not good. > > As above, this CL doesn't actually provide any of the private data yet. Apart > from that, there's also the fact that Chrome currently ships with only one > Instant-supporting engine, which is Google. Since there's no UI to set or change > the instant_url for any engine, this means that it can't actually be used by > Bing, Yandex, etc.
http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { On 2012/07/24 23:00:23, sky wrote: > 'native' is a bit confusing here. I think this should be > InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. The naming is to be consistent with the JS API: OnNativeSuggestions. In code we use NativeSuggestions throughout for this reason. Do we want to consider renaming the API?
On Tue, Jul 24, 2012 at 4:28 PM, <shishir@chromium.org> wrote: > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h > File chrome/common/instant_types.h (right): > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... > chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { > On 2012/07/24 23:00:23, sky wrote: >> >> 'native' is a bit confusing here. I think this should be >> InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. > > > The naming is to be consistent with the JS API: OnNativeSuggestions. In > code we use NativeSuggestions throughout for this reason. Do we want to > consider renaming the API? > > http://codereview.chromium.org/10809063/ I'm not familiar with the code you're referring to so I can't comment. -Scott
On 2012/07/24 23:43:36, sky wrote: > On Tue, Jul 24, 2012 at 4:28 PM, <mailto:shishir@chromium.org> wrote: > > > > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h > > File chrome/common/instant_types.h (right): > > > > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... > > chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { > > On 2012/07/24 23:00:23, sky wrote: > >> > >> 'native' is a bit confusing here. I think this should be > >> InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. > > > > > > The naming is to be consistent with the JS API: OnNativeSuggestions. In > > code we use NativeSuggestions throughout for this reason. Do we want to > > consider renaming the API? > > > > http://codereview.chromium.org/10809063/ > > I'm not familiar with the code you're referring to so I can't comment. > In that case, I would like to leave the naming as is to be consistent with the API and rest of the code. > -Scott
On Tue, Jul 24, 2012 at 4:57 PM, <shishir@chromium.org> wrote: > On 2012/07/24 23:43:36, sky wrote: > >> On Tue, Jul 24, 2012 at 4:28 PM, <mailto:shishir@chromium.org> wrote: >> > >> > > > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h >> >> > File chrome/common/instant_types.h (right): >> > >> > > > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... >> >> > chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { >> > On 2012/07/24 23:00:23, sky wrote: >> >> >> >> 'native' is a bit confusing here. I think this should be >> >> InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. >> > >> > >> > The naming is to be consistent with the JS API: OnNativeSuggestions. In >> > code we use NativeSuggestions throughout for this reason. Do we want to >> > consider renaming the API? >> > >> > http://codereview.chromium.org/10809063/ > > >> I'm not familiar with the code you're referring to so I can't comment. > > > > In that case, I would like to leave the naming as is to be consistent with > the > API and rest of the code. The API is being added now, right? This is the right time to rename if we're going to. -Scott > >> -Scott > > > > > http://codereview.chromium.org/10809063/
http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/1/chrome/renderer/searchbox_exte... chrome/renderer/searchbox_extension.cc:240: } else if (name->Equals(v8::String::New("GetNativeSuggestions"))) { On 2012/07/24 23:05:07, dcblack wrote: > From what I can tell from the GetSessionContext function below, it doesn't > depend on any loader code at all and should just work as-is. Am I missing > something about how that code works? All of the accessors (current_url, last_query, or for that matter, value, verbatim, etc), will just return blank strings or zeros or false unless an InstantLoader has sent meaningful data to the renderer. So, in fact, any web page can (today) already see the chrome.searchbox API (just go to any arbitrary webpage, open the dev tools, and notice that chrome.searchbox is available). Since there's no InstantLoader feeding the API with real values however, the API is useless to such pages. Similarly, if an arbitrary page tries to call SetSuggestions, etc, those will be dropped on the floor (the IPC will be sent, but will not be received or accepted by InstantLoader; cf: the page_id checks in instant_loader.cc). > Anyway, flag-guarding exposing the > functions in the first place seems much safer and more correct, and furthermore > we'll need access that that flag in this file anyway in the future when we need > to load the new version of the JS below in SearchBoxExtension::Get(). The new API is backward compatible, so loading the new JS from a file doesn't need to be flag guarded either. I'd agree to flag guard these if it was either easy or necessary. We'll have to jump through a lot of hoops to do so, however. Since the guard isn't necessary, I think the hoop-jumping isn't necessary either.
http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { On 2012/07/24 23:28:07, Shishir wrote: > On 2012/07/24 23:00:23, sky wrote: > > 'native' is a bit confusing here. I think this should be > > InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. > > The naming is to be consistent with the JS API: OnNativeSuggestions. In code we > use NativeSuggestions throughout for this reason. Do we want to consider > renaming the API? I agree that "native" is confusing. When I first joined the extended API project, I constantly had to reinterpret it in my head. For that matter, I don't like the "Parts" thing in the name either. How about InstantAutocompleteResults (since they come from the autocomplete system) or InstantOmniboxResults? Shishir: We don't have to rename the JS side. For example, where the JS says "setNonNativeDropdownHeight", we turn it into "SetInstantPreviewHeight" (which is clearer to me, at least).
On 2012/07/25 17:48:23, sreeram wrote: > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h > File chrome/common/instant_types.h (right): > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... > chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { > On 2012/07/24 23:28:07, Shishir wrote: > > On 2012/07/24 23:00:23, sky wrote: > > > 'native' is a bit confusing here. I think this should be > > > InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. > > > > The naming is to be consistent with the JS API: OnNativeSuggestions. In code > we > > use NativeSuggestions throughout for this reason. Do we want to consider > > renaming the API? > > I agree that "native" is confusing. When I first joined the extended API > project, I constantly had to reinterpret it in my head. For that matter, I don't > like the "Parts" thing in the name either. > > How about InstantAutocompleteResults (since they come from the autocomplete > system) or InstantOmniboxResults? > > Shishir: We don't have to rename the JS side. For example, where the JS says > "setNonNativeDropdownHeight", we turn it into "SetInstantPreviewHeight" (which > is clearer to me, at least). So based on feedback, I am changing: ChromeViewMsg_SearchBoxNativeSuggestions to ChromeViewMsg_SearchBoxAutocompleteSuggestions and InstantNativeSuggestionsParts to InstantAutocompleteSuggestions
Renamed InstantSuggestionParts as suggested.
What will the JS API use for names? -Scott On Wed, Jul 25, 2012 at 12:31 PM, <shishir@chromium.org> wrote: > On 2012/07/25 17:48:23, sreeram wrote: > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_types.h >> >> File chrome/common/instant_types.h (right): > > > > http://codereview.chromium.org/10809063/diff/11003/chrome/common/instant_type... >> >> chrome/common/instant_types.h:52: struct InstantNativeSuggestionsParts { >> On 2012/07/24 23:28:07, Shishir wrote: >> > On 2012/07/24 23:00:23, sky wrote: >> > > 'native' is a bit confusing here. I think this should be >> > > InstantHostSuggestionParts or event InstantOmniboxSuggestionParts. >> > >> > The naming is to be consistent with the JS API: OnNativeSuggestions. In >> > code >> we >> > use NativeSuggestions throughout for this reason. Do we want to consider >> > renaming the API? > > >> I agree that "native" is confusing. When I first joined the extended API >> project, I constantly had to reinterpret it in my head. For that matter, I > > don't >> >> like the "Parts" thing in the name either. > > >> How about InstantAutocompleteResults (since they come from the >> autocomplete >> system) or InstantOmniboxResults? > > >> Shishir: We don't have to rename the JS side. For example, where the JS >> says >> "setNonNativeDropdownHeight", we turn it into "SetInstantPreviewHeight" >> (which >> is clearer to me, at least). > > > So based on feedback, I am changing: > > ChromeViewMsg_SearchBoxNativeSuggestions to > ChromeViewMsg_SearchBoxAutocompleteSuggestions > and > InstantNativeSuggestionsParts to InstantAutocompleteSuggestions > > > http://codereview.chromium.org/10809063/
On 2012/07/25 21:16:21, sky wrote: > What will the JS API use for names? > > -Scott The JS Api name will remain the same: OnNativeSuggestions.
Rebased to sreeram's changes. Please take a look.
Tony should review the searchbox related changes and Sreeram the instant side.
On 2012/08/09 21:45:47, sky wrote: > Tony should review the searchbox related changes and Sreeram the instant side. Tony is ooo for most of August. Is there someone else who can look at this?
If not Darin, maybe he could suggest someone. -Scott On Thu, Aug 9, 2012 at 2:57 PM, <shishir@chromium.org> wrote: > On 2012/08/09 21:45:47, sky wrote: >> >> Tony should review the searchbox related changes and Sreeram the instant >> side. > > > Tony is ooo for most of August. Is there someone else who can look at this? > > https://chromiumcodereview.appspot.com/10809063/
You should update the CL description to say that this CL just adds the IPCs and renderer side hooks for the extended API, but functionally there's no effect because it doesn't include any of the browser side changes (which will come in a later CL and be flag guarded). http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:202: InstantCompleteBehavior last_complete_behavior_; You can replace these two with: InstantSuggestion last_suggestion_; http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader_delegate.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:10: #include "base/string16.h" You can remove this header include. http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:22: const std::vector<InstantSuggestion>& suggestion) = 0; suggestion -> suggestions http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... File chrome/common/render_messages.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... chrome/common/render_messages.h:302: IPC_MESSAGE_ROUTED1(ChromeViewMsg_SearchBoxInstantAutocompleteResults, Drop the "Instant" from the IPC name: ChromeViewMsg_SearchBoxAutocompleteResults http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... chrome/common/render_messages.h:317: IPC_MESSAGE_ROUTED0(ChromeViewMsg_SearchBoxBlurred) Delete these four APIs (LastQuery, CurrentURL, Focused and Blurred). They are not needed at the moment, and we'll only get to them much later, after the basic extended API work is done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.cc File chrome/renderer/searchbox.cc (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.cc:7: #include "base/utf_string_conversions.h" We don't need this header, right? http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.h File chrome/renderer/searchbox.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:41: int rid_base() const { return rid_base_; } "RID" is too cryptic. Let's make it "results_base" (which would go well with "results_index"). http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:50: bool is_focused() const { return is_focused_; } As with the IPCs, let's drop the last_query(), current_url() and is_focused() parts of the API for now. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:64: void OnInstantAutocompleteResults( OnInstantAutocompleteResults -> OnAutocompleteResults http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:81: std::vector<InstantAutocompleteResult> instant_autocomplete_results_; Drop "instant_": std::vector<InstantAutocompleteResult> autocomplete_results_; http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLRequest.h" No need for this include. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:91: static const char kDispatchNativeSuggestionsEventScript[] = kDispatchNativeSuggestionsEventScript -> kDispatchAutocompleteResultsEventScript http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:171: static v8::Handle<v8::Value> GetNativeSuggestions(const v8::Arguments& args); Don't you need to also make changes to chrome/renderer/resources/extensions/searchbox_api.js? http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:203: static v8::Handle<v8::Value> NavigateContentWindow(const v8::Arguments& args); Get rid of this also. Not needed in the near future. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... File chrome/renderer/searchbox_extension.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.h:36: static void DispatchNativeSuggestions(WebKit::WebFrame* frame); DispatchNativeSuggestions -> DispatchAutocompleteResults
http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:202: InstantCompleteBehavior last_complete_behavior_; On 2012/08/09 22:13:51, sreeram wrote: > You can replace these two with: > InstantSuggestion last_suggestion_; Done. http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader_delegate.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:10: #include "base/string16.h" On 2012/08/09 22:13:51, sreeram wrote: > You can remove this header include. Done. http://codereview.chromium.org/10809063/diff/33002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:22: const std::vector<InstantSuggestion>& suggestion) = 0; On 2012/08/09 22:13:51, sreeram wrote: > suggestion -> suggestions Done. http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... File chrome/common/render_messages.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... chrome/common/render_messages.h:302: IPC_MESSAGE_ROUTED1(ChromeViewMsg_SearchBoxInstantAutocompleteResults, On 2012/08/09 22:13:51, sreeram wrote: > Drop the "Instant" from the IPC name: ChromeViewMsg_SearchBoxAutocompleteResults Done. http://codereview.chromium.org/10809063/diff/33002/chrome/common/render_messa... chrome/common/render_messages.h:317: IPC_MESSAGE_ROUTED0(ChromeViewMsg_SearchBoxBlurred) On 2012/08/09 22:13:51, sreeram wrote: > Delete these four APIs (LastQuery, CurrentURL, Focused and Blurred). They are > not needed at the moment, and we'll only get to them much later, after the basic > extended API work is done. Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.cc File chrome/renderer/searchbox.cc (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.cc:7: #include "base/utf_string_conversions.h" On 2012/08/09 22:13:51, sreeram wrote: > We don't need this header, right? Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.h File chrome/renderer/searchbox.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:41: int rid_base() const { return rid_base_; } On 2012/08/09 22:13:51, sreeram wrote: > "RID" is too cryptic. Let's make it "results_base" (which would go well with > "results_index"). Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:50: bool is_focused() const { return is_focused_; } On 2012/08/09 22:13:51, sreeram wrote: > As with the IPCs, let's drop the last_query(), current_url() and is_focused() > parts of the API for now. Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:64: void OnInstantAutocompleteResults( On 2012/08/09 22:13:51, sreeram wrote: > OnInstantAutocompleteResults -> OnAutocompleteResults Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox.... chrome/renderer/searchbox.h:81: std::vector<InstantAutocompleteResult> instant_autocomplete_results_; On 2012/08/09 22:13:51, sreeram wrote: > Drop "instant_": std::vector<InstantAutocompleteResult> autocomplete_results_; Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... File chrome/renderer/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLRequest.h" On 2012/08/09 22:13:51, sreeram wrote: > No need for this include. Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:91: static const char kDispatchNativeSuggestionsEventScript[] = On 2012/08/09 22:13:51, sreeram wrote: > kDispatchNativeSuggestionsEventScript -> kDispatchAutocompleteResultsEventScript Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:171: static v8::Handle<v8::Value> GetNativeSuggestions(const v8::Arguments& args); On 2012/08/09 22:13:51, sreeram wrote: > Don't you need to also make changes to > chrome/renderer/resources/extensions/searchbox_api.js? Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.cc:203: static v8::Handle<v8::Value> NavigateContentWindow(const v8::Arguments& args); On 2012/08/09 22:13:51, sreeram wrote: > Get rid of this also. Not needed in the near future. Done. http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... File chrome/renderer/searchbox_extension.h (right): http://codereview.chromium.org/10809063/diff/33002/chrome/renderer/searchbox_... chrome/renderer/searchbox_extension.h:36: static void DispatchNativeSuggestions(WebKit::WebFrame* frame); On 2012/08/09 22:13:51, sreeram wrote: > DispatchNativeSuggestions -> DispatchAutocompleteResults Done.
http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... File chrome/renderer/resources/extensions/searchbox_api.js (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... chrome/renderer/resources/extensions/searchbox_api.js:18: native function GetAutocompleteResults(); Don't we need all the shadow DOM stuff here? http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... chrome/renderer/resources/extensions/searchbox_api.js:25: native function SetNonNativeDropdownHeight(); This is a good opportunity to rename some of these. Remember to rename the corresponding accessors in searchbox.cc, IPCs and maybe even InstantLoader/Controller if necessary. GetValue -> GetFullText / GetText / GetQuery ("searchbox.value" makes sense in context, but I think it's good to avoid generic words like value/data/info and use more descriptive names whenever possible). SetNonNativeDropdownHeight -> SetPreviewHeight I like the Inline names, but I'd re-order the words slightly so that they read better: SetInlineSuggestionText SetInlineAutocompleteResult The Replace names don't read well ("Replace" seems more like a verb than an adjective/noun). Instead, let's make it mirror the GetValue accessor: SetFullText / SetText / SetQuery SetAutocompleteResult http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox.h (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox.h:44: const { The const seems to fit on the previous line, no? http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLRequest.h" We don't need this header. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:313: SearchBox::Get(render_view)->autocomplete_results(); suggestions -> results, throughout this method. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:321: v8::String::New(suggestions[i].contents.c_str())); suggestions[i].provider/contents are string16s. .c_str() will not do what we want (given the embedded NULLs). Instead, do: const string16& provider = results[i].provider; suggestion->Set(v8::String::New("provider"), v8::String::New(reinterpret_cast<const uint16_t*>(provider.data()), provider.length())); http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:383: suggestion_object->Get(v8::String::New("value"))); Why replace all the "type foo = bar;" statements with "type foo(bar)"? They are exactly the same in terms of functionality, though the former seems slightly more legible due to avoiding unnecessary parentheses. If you agree, I'd suggest following the same style in GetAutocompleteResults() above.
Please take another look. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... File chrome/renderer/resources/extensions/searchbox_api.js (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... chrome/renderer/resources/extensions/searchbox_api.js:18: native function GetAutocompleteResults(); On 2012/08/13 16:37:08, sreeram wrote: > Don't we need all the shadow DOM stuff here? Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/resources/... chrome/renderer/resources/extensions/searchbox_api.js:25: native function SetNonNativeDropdownHeight(); On 2012/08/13 16:37:08, sreeram wrote: > This is a good opportunity to rename some of these. Remember to rename the > corresponding accessors in searchbox.cc, IPCs and maybe even > InstantLoader/Controller if necessary. > > GetValue -> GetFullText / GetText / GetQuery > ("searchbox.value" makes sense in context, but I think it's good to avoid > generic words like value/data/info and use more descriptive names whenever > possible). > > SetNonNativeDropdownHeight -> SetPreviewHeight > > I like the Inline names, but I'd re-order the words slightly so that they read > better: > SetInlineSuggestionText > SetInlineAutocompleteResult > > The Replace names don't read well ("Replace" seems more like a verb than an > adjective/noun). Instead, let's make it mirror the GetValue accessor: > SetFullText / SetText / SetQuery > SetAutocompleteResult Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox.h (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox.h:44: const { On 2012/08/13 16:37:08, sreeram wrote: > The const seems to fit on the previous line, no? Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLRequest.h" On 2012/08/13 16:37:08, sreeram wrote: > We don't need this header. Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:313: SearchBox::Get(render_view)->autocomplete_results(); On 2012/08/13 16:37:08, sreeram wrote: > suggestions -> results, throughout this method. Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:321: v8::String::New(suggestions[i].contents.c_str())); On 2012/08/13 16:37:08, sreeram wrote: > suggestions[i].provider/contents are string16s. .c_str() will not do what we > want (given the embedded NULLs). Instead, do: > > const string16& provider = results[i].provider; > suggestion->Set(v8::String::New("provider"), > v8::String::New(reinterpret_cast<const uint16_t*>(provider.data()), > provider.length())); Done. http://codereview.chromium.org/10809063/diff/21018/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:383: suggestion_object->Get(v8::String::New("value"))); On 2012/08/13 16:37:08, sreeram wrote: > Why replace all the "type foo = bar;" statements with "type foo(bar)"? They are > exactly the same in terms of functionality, though the former seems slightly > more legible due to avoiding unnecessary parentheses. > > If you agree, I'd suggest following the same style in GetAutocompleteResults() > above. Done.
Please take a look.
http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:46: s->Write(reinterpret_cast<uint16_t*>(WriteInto(&result, len + 1)), 0, len); How about this instead? string16 V8ValueToUTF16(v8::Handle<v8::Value> v) { v8::String::Value s = v; return string16(reinterpret_cast<const char16*>(*s), s.length()); } And given the number of times we have to do the other way around, might as well define this too: v8::Handle<v8::String> UTF16ToV8String(const string16& s) { return v8::String::New(reinterpret_cast<const uint16_t*>(s.data()), s.size()); } http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:261: SearchBox::Get(render_view)->query().length()); This can be simplified to: return UTF16ToV8String(SearchBox::Get(render_view)->query()); http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:340: contents.length())); These can be simplified to: result->Set(v8::String::New("provider"), UTF16ToV8String(results[i].provider); result->Set(v8::String::New("contents"), UTF16ToV8String(results[i].contents); http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:370: v8::Local<v8::Object> suggestion_json = args[0]->ToObject(); I'm not sure why we refer to v8::Local explicitly, as opposed to just saying v8::Handle. Could you change all these v8::Local to v8::Handle (throughout this method)? Unless there's an overriding reason to say that a specific object is a local (instead of a persistent one), we should just use the base class (v8::Handle). http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:404: string16 text = V8StringToUTF16(suggestion_object_value->ToString()); This can be simplified to: string16 text = V8ValueToUTF16(suggestion_object_value); (and you can use a similar pattern everywhere else below.)
http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... File chrome/renderer/searchbox/searchbox_extension.cc (right): http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:46: s->Write(reinterpret_cast<uint16_t*>(WriteInto(&result, len + 1)), 0, len); On 2012/08/13 22:40:56, sreeram wrote: > How about this instead? > string16 V8ValueToUTF16(v8::Handle<v8::Value> v) { > v8::String::Value s = v; > return string16(reinterpret_cast<const char16*>(*s), s.length()); > } > > And given the number of times we have to do the other way around, might as well > define this too: > v8::Handle<v8::String> UTF16ToV8String(const string16& s) { > return v8::String::New(reinterpret_cast<const uint16_t*>(s.data()), s.size()); > } Done with minor modifications. http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:261: SearchBox::Get(render_view)->query().length()); On 2012/08/13 22:40:56, sreeram wrote: > This can be simplified to: > return UTF16ToV8String(SearchBox::Get(render_view)->query()); Done. http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:340: contents.length())); On 2012/08/13 22:40:56, sreeram wrote: > These can be simplified to: > result->Set(v8::String::New("provider"), > UTF16ToV8String(results[i].provider); > result->Set(v8::String::New("contents"), > UTF16ToV8String(results[i].contents); Done. http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:370: v8::Local<v8::Object> suggestion_json = args[0]->ToObject(); On 2012/08/13 22:40:56, sreeram wrote: > I'm not sure why we refer to v8::Local explicitly, as opposed to just saying > v8::Handle. Could you change all these v8::Local to v8::Handle (throughout this > method)? Unless there's an overriding reason to say that a specific object is a > local (instead of a persistent one), we should just use the base class > (v8::Handle). Done. http://codereview.chromium.org/10809063/diff/27009/chrome/renderer/searchbox/... chrome/renderer/searchbox/searchbox_extension.cc:404: string16 text = V8StringToUTF16(suggestion_object_value->ToString()); On 2012/08/13 22:40:56, sreeram wrote: > This can be simplified to: > string16 text = V8ValueToUTF16(suggestion_object_value); > (and you can use a similar pattern everywhere else below.) Done.
lgtm
Hi Scott, Could you look at the chrome/common/... changes? Thanks Shishir
http://codereview.chromium.org/10809063/diff/16005/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/16005/chrome/common/instant_type... chrome/common/instant_types.h:44: void Clear(); newline between 43/44 and add a description. That said, is there really a reason for Clear? Since this class supports copy semantics you just do: suggestion = InstantSuggestion();
PTAL. http://codereview.chromium.org/10809063/diff/16005/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10809063/diff/16005/chrome/common/instant_type... chrome/common/instant_types.h:44: void Clear(); On 2012/08/14 17:10:10, sky wrote: > newline between 43/44 and add a description. That said, is there really a reason > for Clear? Since this class supports copy semantics you just do: > suggestion = InstantSuggestion(); Removed the clear method.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10809063/25018
Change committed as 151642 |
