Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21323003:
The UI part of search-by-image search. (Closed)
|
Created:
7 years, 4 months ago by Johnny(Jianning) Ding Modified:
7 years, 4 months ago CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionThe UI part of search-by-image search.
When users click the 'search-by-image', the work flow is
(1) Chrome sends ChromeViewMsg_RequestThumbnailForContextNode to render
(2) Render gets the image, down-scale (if necessary) and send it back with ChromeViewHostMsg_RequestThumbnailForContextNode_ACK)
(3) Chrome generates image search URL for current search provider, sends the search request and opens a new tab to show the result.
BUG=89945
TEST=
it depends on https://codereview.chromium.org/21378002/ to pass compilation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215528
Patch Set 1 #
Total comments: 17
Patch Set 2 : #Patch Set 3 : add menu string resource #
Total comments: 6
Patch Set 4 : fix string resource and comment #
Total comments: 8
Patch Set 5 : addressed avi's comments #
Total comments: 7
Patch Set 6 : add android version string resource #
Total comments: 2
Patch Set 7 : remove android version string resource #Patch Set 8 : address jam's comment #
Messages
Total messages: 35 (0 generated)
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); I would suggest to rename this as "Search %s for this image" just to keep consistent with the existing "Search Google for 'highlighted text'" function in the context menu. Also, is there any i18n support for translating such messages?
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:193: {57, IDC_CONTENT_CONTEXT_SEARCHIMAGENEWTAB }, Match style; space after { https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); Um... Every one of our UI strings is localized and translated. How is this different?
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); note: just like IDS_CONTENT_CONTEXT_SEARCHWEBFOR in generated_resources.grd
https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi#newc... components/autofill.gypi:36: '../skia/skia.gyp:skia', Why is an autofill file in this CL? What does it have to do with this feature?
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); On 2013/07/31 20:20:53, Kibeom Kim wrote: > note: just like IDS_CONTENT_CONTEXT_SEARCHWEBFOR in generated_resources.grd Yes. Look for examples (in this very file) with l10n_util::GetStringFUTF16, where that retrieves a localized string and substitutes in a variable.
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:125: const int kImageSearchThumbnailMaxHeight = 600; Actually, Android Chrome need to access these values from the outside. The call stack of Android popup goes like: 1. content::WebContentsImpl::ShowContextMenu(...) content/browser/web_contents/web_contents_impl.cc 2. content::WebContentsImpl::ShowContextMenu(...) content/browser/web_contents/web_contents_view_android.cc 3. content::WebContentsViewAndroid::ShowContextMenu(...) chrome/browser/ui/android/tab_contents/chrome_web_contents_view_delegate_android.cc 4.TabAndroidImpl::ShowContextMenu(...) clank/native/framework/chrome/tab_android_impl.cc //private 5. // Android Shows context menu 6. TabAndroidImpl::SearchByImage(...) clank/native/framework/chrome/tab_android_impl.cc //private 7. call: web_contents()->GetRenderViewHost()->Send(new ChromeViewMsg_RequestThumbnailForContextNode(.....))
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); The hard-code "Search Image By %s" should be replaced by the translated message, but I am not familiar with this part. Please guide me how to change this part.
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:193: {57, IDC_CONTENT_CONTEXT_SEARCHIMAGENEWTAB }, On 2013/07/31 20:11:05, Avi wrote: > Match style; space after { Done. https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi#newc... components/autofill.gypi:36: '../skia/skia.gyp:skia', On 2013/07/31 20:24:48, Avi wrote: > Why is an autofill file in this CL? What does it have to do with this feature? it is caused by https://codereview.chromium.org/19856004/ which adds WebImage in WebNode. WebImage depends on Skia, WebNode depends on WebImage, and the autofill used WebNode API. So for now I just add skia dependency in here to pass compilation. James plans to remove the skia dependency from WebNode, once it is done, we can drop this change.
On 2013/07/31 21:26:36, Johnny(Jianning) Ding wrote: > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > chrome/browser/tab_contents/render_view_context_menu.cc:867: > base::StringPrintf("Search Image By %s", search_provider_name.c_str()); > The hard-code "Search Image By %s" should be replaced by the translated message, > but I am not familiar with this part. Please guide me how to change this part. one more question, when changing the grd file, should I file a request to ask string resource translation, or should I notice someone?
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:2067: void RenderViewContextMenu::GetImageThumbnailForSearch(int x, int y) { How x and y are used?
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:2067: void RenderViewContextMenu::GetImageThumbnailForSearch(int x, int y) { On 2013/07/31 21:49:49, Kibeom Kim wrote: > How x and y are used? they are gone.
+add Peter Peter, can you help review changes inside chrome/browser/ui/gtk/menu_gtk.cc? Jingbin On 2013/07/31 21:53:47, Johnny(Jianning) Ding wrote: > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > chrome/browser/tab_contents/render_view_context_menu.cc:2067: void > RenderViewContextMenu::GetImageThumbnailForSearch(int x, int y) { > On 2013/07/31 21:49:49, Kibeom Kim wrote: > > How x and y are used? > > they are gone.
On 2013/07/31 21:56:41, jingbinw wrote: > +add Peter > > Peter, can you help review changes inside chrome/browser/ui/gtk/menu_gtk.cc? c/b/ui/gtk LGTM
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); Can you rewording the message to be "Search %s for this image"? BTW: In terms of translated message, I think what you did is already correct given Avi's last comment. On 2013/07/31 21:26:36, Johnny(Jianning) Ding wrote: > The hard-code "Search Image By %s" should be replaced by the translated message, > but I am not familiar with this part. Please guide me how to change this part.
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); On 2013/07/31 23:31:29, jingbinw wrote: > Can you rewording the message to be "Search %s for this image"? > > BTW: In terms of translated message, I think what you did is already correct > given Avi's last comment. No, it's not correct. You should never see a .cc file with a string in it that gets displayed to a user. All strings should be handled by the l10n framework. Look at line 952 for an example of how to do this correctly.
https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... chrome/browser/tab_contents/render_view_context_menu.cc:867: base::StringPrintf("Search Image By %s", search_provider_name.c_str()); here: menu_model_.AddItem( IDC_CONTENT_CONTEXT_SEARCHIMAGENEWTAB, l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE, default_provider->short_name())); in generated_resources.grd, near the two IDS_CONTENT_CONTEXT_SEARCHWEBFOR s: <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search the Web for the image in the content area context menu"> &Search image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> </message> and <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search the Web for the image in the content area context menu"> &Search Image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> </message>
On 2013/08/01 00:14:59, Kibeom Kim wrote: > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > https://codereview.chromium.org/21323003/diff/1/chrome/browser/tab_contents/r... > chrome/browser/tab_contents/render_view_context_menu.cc:867: > base::StringPrintf("Search Image By %s", search_provider_name.c_str()); > here: > menu_model_.AddItem( > IDC_CONTENT_CONTEXT_SEARCHIMAGENEWTAB, > l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE, > default_provider->short_name())); > > in generated_resources.grd, near the two IDS_CONTENT_CONTEXT_SEARCHWEBFOR s: > > <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the > Search the Web for the image in the content area context menu"> > &Search image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> > </message> > > and > > <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the > Search the Web for the image in the content area context menu"> > &Search Image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> > </message> done
https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi#newc... components/autofill.gypi:36: '../skia/skia.gyp:skia', Yikes. This is a weird dependency. Can you add a comment? How will you be sure it will be removed when James makes that fix? https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... chrome/app/generated_resources.grd:697: + &Search image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> Why does this not have parallel phrasing to IDS_CONTENT_CONTEXT_SEARCHWEBFOR? IDS_CONTENT_CONTEXT_SEARCHWEBFOR is: Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for '<ph name="SEARCH_TERMS">$2<ex>flowers</ex></ph>' Shouldn't this be: Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for this image https://codereview.chromium.org/21323003/diff/35001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right): https://codereview.chromium.org/21323003/diff/35001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.h:46: void OnGetImageThumbnail(const SkBitmap& bitmap); Needs a comment.
https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... chrome/app/generated_resources.grd:696: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search the Web for the image in the content area context menu"> Grammar: "The name of the Search For Image command in the content area context menu" https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... chrome/app/generated_resources.grd:697: + &Search image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> On 2013/08/01 05:33:03, Avi wrote: > Why does this not have parallel phrasing to IDS_CONTENT_CONTEXT_SEARCHWEBFOR? > > IDS_CONTENT_CONTEXT_SEARCHWEBFOR is: Search <ph > name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for '<ph > name="SEARCH_TERMS">$2<ex>flowers</ex></ph>' > > Shouldn't this be: Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for > this image Yes, it should.
https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/35001/chrome/app/generated_reso... chrome/app/generated_resources.grd:697: + &Search image by <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> On 2013/08/01 05:35:31, Peter Kasting wrote: > On 2013/08/01 05:33:03, Avi wrote: > > Why does this not have parallel phrasing to IDS_CONTENT_CONTEXT_SEARCHWEBFOR? > > > > IDS_CONTENT_CONTEXT_SEARCHWEBFOR is: Search <ph > > name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for '<ph > > name="SEARCH_TERMS">$2<ex>flowers</ex></ph>' > > > > Shouldn't this be: Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for > > this image > > Yes, it should. Done. https://codereview.chromium.org/21323003/diff/35001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right): https://codereview.chromium.org/21323003/diff/35001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.h:46: void OnGetImageThumbnail(const SkBitmap& bitmap); On 2013/08/01 05:33:03, Avi wrote: > Needs a comment. Done.
Mostly nits now, though you still haven't answered my question as to how you plan on preventing the temporary gypi change from ossifying. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right): https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:96: OnGetImageThumbnail) We name message handlers "OnXXX" where the message is TypeOfMsg_XXX. If your message is ChromeViewHostMsg_RequestThumbnailForContextNode_ACK, call OnGetImageThumbnail OnRequestThumbnailForContextNodeACK or so. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:222: GetDefaultSearchProvider(); indent another 4. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:224: TemplateURLRef::SearchTermsArgs search_args(ASCIIToUTF16("")); TemplateURLRef::SearchTermsArgs search_args(base::string16()); https://codereview.chromium.org/21323003/diff/45001/chrome/browser/tab_conten... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/45001/chrome/browser/tab_conten... chrome/browser/tab_contents/render_view_context_menu.cc:858: GetDefaultSearchProvider(); indent another 4.
https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi File components/autofill.gypi (right): https://codereview.chromium.org/21323003/diff/1/components/autofill.gypi#newc... components/autofill.gypi:36: '../skia/skia.gyp:skia', On 2013/08/01 05:33:03, Avi wrote: > Yikes. This is a weird dependency. Can you add a comment? How will you be sure > it will be removed when James makes that fix? The fix was landed, will revert the change in this file. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right): https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:96: OnGetImageThumbnail) On 2013/08/01 18:46:09, Avi wrote: > We name message handlers "OnXXX" where the message is TypeOfMsg_XXX. > > If your message is ChromeViewHostMsg_RequestThumbnailForContextNode_ACK, call > OnGetImageThumbnail OnRequestThumbnailForContextNodeACK or so. Done. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:222: GetDefaultSearchProvider(); On 2013/08/01 18:46:09, Avi wrote: > indent another 4. Done. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.cc:224: TemplateURLRef::SearchTermsArgs search_args(ASCIIToUTF16("")); On 2013/08/01 18:46:09, Avi wrote: > TemplateURLRef::SearchTermsArgs search_args(base::string16()); Done. https://codereview.chromium.org/21323003/diff/45001/chrome/browser/tab_conten... File chrome/browser/tab_contents/render_view_context_menu.cc (right): https://codereview.chromium.org/21323003/diff/45001/chrome/browser/tab_conten... chrome/browser/tab_contents/render_view_context_menu.cc:858: GetDefaultSearchProvider(); On 2013/08/01 18:46:09, Avi wrote: > indent another 4. Done.
LGTM Woot!
https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:790: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> could you add formatter_data="android_java" after desc="..." so that it is accessible from Android? https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:1002: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> also here.
https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:790: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> please discard this, just discussed with tedchoc@ and decided it to have a separate downstream string.
https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:790: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> Actually, not downstream but here. <if expr="is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for this image </message> </if> <if expr="not is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> &Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for this image </message> </if> https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:1002: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> note that not "this image" but "This Image" here. <if expr="is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for This Image </message> </if> <if expr="not is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> &Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for This Image </message> </if>
https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:790: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu"> Forgot to include formatter_data="android_java". This is correct version: <if expr="is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu" formatter_data="android_java"> Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for this image </message> </if> <if expr="not is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="The name of the Search For Image command in the content area context menu" > &Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for this image </message> </if> https://codereview.chromium.org/21323003/diff/54001/chrome/app/generated_reso... chrome/app/generated_resources.grd:1002: + <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> This is correct version: <if expr="is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu" formatter_data="android_java"> Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for This Image </message> </if> <if expr="not is_android"> <message name="IDS_CONTENT_CONTEXT_SEARCHWEBFORIMAGE" desc="In Title Case: The name of the Search For Image command in the content area context menu"> &Search <ph name="SEARCH_ENGINE">$1<ex>Google</ex></ph> for This Image </message> </if>
lgtm generate_resource.grd string lgtm for patch set 5 as we're adding the Android string on downstream https://gerrit-int.chromium.org/#/c/42116/ .
cpu@ jam@ Could you take a look at this? currently missng lgtms for chrome/app/chrome_command_ids.h chrome/browser/renderer_host/chrome_render_view_host_observer.h chrome/browser/renderer_host/chrome_render_view_host_observer.h
lgtm https://codereview.chromium.org/21323003/diff/67001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right): https://codereview.chromium.org/21323003/diff/67001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.h:46: // Handles the image thumbnail for the context node, composes a image search nit: we don't document IPC message handlers in the header. a description would usually be put in the messages.h file above the IPC instead. also separate all the IPC handlers into their own block (i.e. with new line above and below)
https://codereview.chromium.org/21323003/diff/67001/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right): https://codereview.chromium.org/21323003/diff/67001/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_view_host_observer.h:46: // Handles the image thumbnail for the context node, composes a image search On 2013/08/02 15:40:16, jam wrote: > nit: we don't document IPC message handlers in the header. a description would > usually be put in the messages.h file above the IPC instead. also separate all > the IPC handlers into their own block (i.e. with new line above and below) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jnd@chromium.org/21323003/88001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jnd@chromium.org/21323003/88001
Message was sent while issue was closed.
Change committed as 215528 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
