Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335023:
Add the feature "Add as seach engine..." when right clicking on ... (Closed)
|
Created:
11 years, 2 months ago by Philippe Beauchamp Modified:
7 years, 5 months ago CC:
chromium-reviews, brettw+cc_chromium.org, darin (slow to review), ben+cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd the feature "Add as seach engine..." when right clicking on a form text field.
Note: POST forms are not supported with this patch
BUG=6872
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91253
Patch Set 1 #Patch Set 2 : '' #
Total comments: 12
Patch Set 3 : '' #
Total comments: 26
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #
Total comments: 12
Patch Set 6 : '' #
Total comments: 3
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 8
Patch Set 10 : '' #Patch Set 11 : '' #
Messages
Total messages: 43 (0 generated)
Draft of the "Add as search engine" feature. Please tell me if the approach is ok. Should we look forward to include the support for POST forms in this patch?
You beat me to this, I was refactoring the searchable form data code last week precisely so I could write something like this. There are a few issues with this patch: * Style violations, e.g. "foo(0,0,0)" instead of "foo(0, 0, 0)". Check the linter output too. * I think we should only add the context menu option when it's going to be usable, instead of adding it for all editable areas. * Your technique for determining when a field is suitable for this might be over-conservative, in that the WebSearchableFormData constructor will rule out some types of forms that are probably OK for a manual search. For example, we probably don't need to restrict to HTTP (HTTPS would be OK here), and maybe we can deal with forms with multiple entry fields or where some options aren't in the default state. Scott should chime in here.
I agree with Peter, all those restrictions aren't applicable when users is explicitly creating a search engine. -Scott On Mon, Oct 26, 2009 at 10:51 AM, <pkasting@chromium.org> wrote: > You beat me to this, I was refactoring the searchable form data code last > week > precisely so I could write something like this. > > There are a few issues with this patch: > * Style violations, e.g. "foo(0,0,0)" instead of "foo(0, 0, 0)". =A0Check= the > linter output too. > * I think we should only add the context menu option when it's going to b= e > usable, instead of adding it for all editable areas. > * Your technique for determining when a field is suitable for this might = be > over-conservative, in that the WebSearchableFormData constructor will rul= e > out > some types of forms that are probably OK for a manual search. =A0For exam= ple, > we > probably don't need to restrict to HTTP (HTTPS would be OK here), and may= be > we > can deal with forms with multiple entry fields or where some options aren= 't > in > the default state. =A0Scott should chime in here. > > http://codereview.chromium.org/335023 >
Ok, thanks. I will work on an updated patch for next weekend. I'm not working full time on this so feel free to tell me if you prefer that I work on another issue.
Updated patch. (Sorry, still some style errors remaining) Is the approach for WebSearchableFormData.cpp looks ok? I'm not sure if we should merge the original implementation with the explicit search.
http://codereview.chromium.org/335023/diff/6001/7006 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/6001/7006#newcode760 Line 760: TemplateURL *tpl = new TemplateURL(); Nit: * on type, not variable. http://codereview.chromium.org/335023/diff/6001/7006#newcode762 Line 762: tpl->set_keyword(L""); Why not set a default keyword here, like we would if we auto-added the engine? http://codereview.chromium.org/335023/diff/6001/7006#newcode764 Line 764: EditSearchEngineDialog::Show( Nit: Just wrap like this: EditSearchEngineDialog::Show(platform_util::GetTopLevel( source_tab_contents_->GetContentNativeView()), tpl, NULL, profile_); http://codereview.chromium.org/335023/diff/6001/7004 File webkit/api/public/WebContextMenuData.h (right): http://codereview.chromium.org/335023/diff/6001/7004#newcode67 Line 67: // The absolute keyword URL including the %s search tag Nit: Add more comments here, about what this is used for, whether it can be empty, and what that means. http://codereview.chromium.org/335023/diff/6001/7004#newcode110 Line 110: CanAddKeyword = 0x80, Instead of adding this flag, we should just key off whether the keyword URL is empty. http://codereview.chromium.org/335023/diff/6001/7003 File webkit/api/src/WebContextMenuClientImpl.cpp (right): http://codereview.chromium.org/335023/diff/6001/7003#newcode50 Line 50: #include "webkit/api/public/WebForm.h" Don't put paths on these, include them pathless in the list below. http://codereview.chromium.org/335023/diff/6001/7003#newcode52 Line 52: #include "webkit/glue/glue_util.h" I don't think webkit API code can depend on glue_util.h. http://codereview.chromium.org/335023/diff/6001/7001 File webkit/api/src/WebSearchableFormData.cpp (right): http://codereview.chromium.org/335023/diff/6001/7001#newcode143 Line 143: bool HasSuitableTextElement(const HTMLFormElement* form, Vector<char>* encodedString, String* encodingName, WebKit::WebString* searchFieldName) Why have this take a string instead of an HTMLInputElement*? Then all the code below falls out naturally since we'll just need to autodetect the elemnt if it's not already set. http://codereview.chromium.org/335023/diff/6001/7001#newcode154 Line 154: bool HasSuitableTextElement=false; Nit: First character should be lowercase. Spaces around operator. http://codereview.chromium.org/335023/diff/6001/7001#newcode170 Line 170: if ((!IsInDefaultState(formElement) || formElement->hasTagName(HTMLNames::textareaTag))) Is it safe to move these below the continue above? Can that change the way the algorithm behaves when autodetecting? http://codereview.chromium.org/335023/diff/6001/7001#newcode258 Line 258: Nit: Why did you add a newline here? http://codereview.chromium.org/335023/diff/6001/7005 File webkit/glue/context_menu.h (right): http://codereview.chromium.org/335023/diff/6001/7005#newcode51 Line 51: // This is the keyword URL including the %s search tag Nit: More comments, use complete sentences (period at end)
Updated Patch On 2009/11/09 18:48:45, Peter Kasting wrote: > http://codereview.chromium.org/335023/diff/6001/7006 > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > http://codereview.chromium.org/335023/diff/6001/7006#newcode760 > Line 760: TemplateURL *tpl = new TemplateURL(); > Nit: * on type, not variable. > > http://codereview.chromium.org/335023/diff/6001/7006#newcode762 > Line 762: tpl->set_keyword(L""); > Why not set a default keyword here, like we would if we auto-added the engine? > > http://codereview.chromium.org/335023/diff/6001/7006#newcode764 > Line 764: EditSearchEngineDialog::Show( > Nit: Just wrap like this: > EditSearchEngineDialog::Show(platform_util::GetTopLevel( > source_tab_contents_->GetContentNativeView()), tpl, NULL, profile_); > > http://codereview.chromium.org/335023/diff/6001/7004 > File webkit/api/public/WebContextMenuData.h (right): > > http://codereview.chromium.org/335023/diff/6001/7004#newcode67 > Line 67: // The absolute keyword URL including the %s search tag > Nit: Add more comments here, about what this is used for, whether it can be > empty, and what that means. > > http://codereview.chromium.org/335023/diff/6001/7004#newcode110 > Line 110: CanAddKeyword = 0x80, > Instead of adding this flag, we should just key off whether the keyword URL is > empty. > > http://codereview.chromium.org/335023/diff/6001/7003 > File webkit/api/src/WebContextMenuClientImpl.cpp (right): > > http://codereview.chromium.org/335023/diff/6001/7003#newcode50 > Line 50: #include "webkit/api/public/WebForm.h" > Don't put paths on these, include them pathless in the list below. > > http://codereview.chromium.org/335023/diff/6001/7003#newcode52 > Line 52: #include "webkit/glue/glue_util.h" > I don't think webkit API code can depend on glue_util.h. > > http://codereview.chromium.org/335023/diff/6001/7001 > File webkit/api/src/WebSearchableFormData.cpp (right): > > http://codereview.chromium.org/335023/diff/6001/7001#newcode143 > Line 143: bool HasSuitableTextElement(const HTMLFormElement* form, Vector<char>* > encodedString, String* encodingName, WebKit::WebString* searchFieldName) > Why have this take a string instead of an HTMLInputElement*? Then all the code > below falls out naturally since we'll just need to autodetect the elemnt if it's > not already set. > > http://codereview.chromium.org/335023/diff/6001/7001#newcode154 > Line 154: bool HasSuitableTextElement=false; > Nit: First character should be lowercase. Spaces around operator. > > http://codereview.chromium.org/335023/diff/6001/7001#newcode170 > Line 170: if ((!IsInDefaultState(formElement) || > formElement->hasTagName(HTMLNames::textareaTag))) > Is it safe to move these below the continue above? Can that change the way the > algorithm behaves when autodetecting? > > http://codereview.chromium.org/335023/diff/6001/7001#newcode258 > Line 258: > Nit: Why did you add a newline here? > > http://codereview.chromium.org/335023/diff/6001/7005 > File webkit/glue/context_menu.h (right): > > http://codereview.chromium.org/335023/diff/6001/7005#newcode51 > Line 51: // This is the keyword URL including the %s search tag > Nit: More comments, use complete sentences (period at end)
http://codereview.chromium.org/335023/diff/9001/10002 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/9001/10002#newcode473 chrome/browser/tab_contents/render_view_context_menu.cc:473: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: Your cases above fall through into this. You should move ADDSEARCHENGINE above them and leave them as falling into the default case. http://codereview.chromium.org/335023/diff/9001/10002#newcode756 chrome/browser/tab_contents/render_view_context_menu.cc:756: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: { Maybe the code in TemplateURLFetcher::RequestDelegate::OnURLFetchComplete() should be refactored so that we can call it from here too, instead of the block below. http://codereview.chromium.org/335023/diff/9001/10006 File third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h (right): http://codereview.chromium.org/335023/diff/9001/10006#newcode68 third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h:68: // "Add as search engine..." option is clicked (Left empty if not used). Nit: Left -> left http://codereview.chromium.org/335023/diff/9001/10004 File third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp (right): http://codereview.chromium.org/335023/diff/9001/10004#newcode214 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:214: WebKit::WebFormElement wf = PassRefPtr<WebCore::HTMLFormElement>(form); Constructing PassRefPtr<>s on the stack is never what you want. See http://webkit.org/coding/RefPtr.html . http://codereview.chromium.org/335023/diff/9001/10004#newcode215 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:215: WebSearchableFormData* ws = new WebSearchableFormData(wf, selectedElement); This leaks. Why not just construct it on the stack? http://codereview.chromium.org/335023/diff/9001/10004#newcode216 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:216: if (ws->url().isValid()) { Nit: WebKit style is no {} here http://codereview.chromium.org/335023/diff/9001/10005 File third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp (right): http://codereview.chromium.org/335023/diff/9001/10005#newcode143 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:143: // encoding_name. Nit: Add comments about |textElement|. http://codereview.chromium.org/335023/diff/9001/10005#newcode145 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:145: const WebCore::HTMLInputElement* textElement) Nit: Put all args on the same line http://codereview.chromium.org/335023/diff/9001/10005#newcode156 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:156: bool isSelectedInputElement = textElement; Nit: This name is kind of bad. How about reversing the sense and naming it |elementIsAutodetected|? http://codereview.chromium.org/335023/diff/9001/10005#newcode157 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:157: bool isElementFound = false; |isElementFound| does nothing for you, as far as I can tell. It seems like you just want to return whether |textElement| is non-NULL at the bottom of the function, like it used to, because if you run into an error before then you'll early-return. http://codereview.chromium.org/335023/diff/9001/10005#newcode192 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:192: // Allow multiple fields form when an input element is provided. Nit: "multiple fields form" -> "forms with multiple fields" http://codereview.chromium.org/335023/diff/9001/10005#newcode233 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:233: // Ignore post form and allow https only when an input element is provided Nit: Original comment's first half was better than "Ignore post form". Add a comma between the clauses for clarity. http://codereview.chromium.org/335023/diff/9001/10003 File webkit/glue/context_menu.h (right): http://codereview.chromium.org/335023/diff/9001/10003#newcode9 webkit/glue/context_menu.h:9: #include <string> Nit: Why add this?
http://codereview.chromium.org/335023/diff/9001/10002 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/9001/10002#newcode473 chrome/browser/tab_contents/render_view_context_menu.cc:473: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: On 2009/11/23 19:59:47, Peter Kasting wrote: > Your cases above fall through into this. You should move ADDSEARCHENGINE above > them and leave them as falling into the default case. Done. http://codereview.chromium.org/335023/diff/9001/10002#newcode756 chrome/browser/tab_contents/render_view_context_menu.cc:756: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: { Should we look forward to do a refactoring with TabContents::GenerateKeywordIfNecessary() instead? Maybe we can add an method in template_url_model that will take as input the search url and build a default search TemplateURL. On 2009/11/23 19:59:47, Peter Kasting wrote: > Maybe the code in TemplateURLFetcher::RequestDelegate::OnURLFetchComplete() > should be refactored so that we can call it from here too, instead of the block > below. http://codereview.chromium.org/335023/diff/9001/10006 File third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h (right): http://codereview.chromium.org/335023/diff/9001/10006#newcode68 third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h:68: // "Add as search engine..." option is clicked (Left empty if not used). On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: Left -> left Done. http://codereview.chromium.org/335023/diff/9001/10004 File third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp (right): http://codereview.chromium.org/335023/diff/9001/10004#newcode214 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:214: WebKit::WebFormElement wf = PassRefPtr<WebCore::HTMLFormElement>(form); On 2009/11/23 19:59:47, Peter Kasting wrote: > Constructing PassRefPtr<>s on the stack is never what you want. See > http://webkit.org/coding/RefPtr.html . Done. http://codereview.chromium.org/335023/diff/9001/10004#newcode215 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:215: WebSearchableFormData* ws = new WebSearchableFormData(wf, selectedElement); On 2009/11/23 19:59:47, Peter Kasting wrote: > This leaks. Why not just construct it on the stack? Done. http://codereview.chromium.org/335023/diff/9001/10004#newcode216 third_party/WebKit/WebKit/chromium/src/ContextMenuClientImpl.cpp:216: if (ws->url().isValid()) { On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: WebKit style is no {} here Done. http://codereview.chromium.org/335023/diff/9001/10005 File third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp (right): http://codereview.chromium.org/335023/diff/9001/10005#newcode143 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:143: // encoding_name. On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: Add comments about |textElement|. Done. http://codereview.chromium.org/335023/diff/9001/10005#newcode145 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:145: const WebCore::HTMLInputElement* textElement) On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: Put all args on the same line Done. http://codereview.chromium.org/335023/diff/9001/10005#newcode156 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:156: bool isSelectedInputElement = textElement; On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: This name is kind of bad. How about reversing the sense and naming it > |elementIsAutodetected|? Done. http://codereview.chromium.org/335023/diff/9001/10005#newcode157 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:157: bool isElementFound = false; The only reason why I added this check is because the textElement will always be true when we provide an explicit textElement. This is just in case that the provided textElement is not found in the form. On 2009/11/23 19:59:47, Peter Kasting wrote: > |isElementFound| does nothing for you, as far as I can tell. It seems like you > just want to return whether |textElement| is non-NULL at the bottom of the > function, like it used to, because if you run into an error before then you'll > early-return. http://codereview.chromium.org/335023/diff/9001/10005#newcode192 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:192: // Allow multiple fields form when an input element is provided. On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: "multiple fields form" -> "forms with multiple fields" Done. http://codereview.chromium.org/335023/diff/9001/10005#newcode233 third_party/WebKit/WebKit/chromium/src/WebSearchableFormData.cpp:233: // Ignore post form and allow https only when an input element is provided On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: Original comment's first half was better than "Ignore post form". Add a > comma between the clauses for clarity. Done. http://codereview.chromium.org/335023/diff/9001/10003 File webkit/glue/context_menu.h (right): http://codereview.chromium.org/335023/diff/9001/10003#newcode9 webkit/glue/context_menu.h:9: #include <string> To remove the lint warning. -> string defined in a sub .h file but I agree that string_util is there for that. I can remove it. On 2009/11/23 19:59:47, Peter Kasting wrote: > Nit: Why add this?
It doesn't look like you actually uploaded your newest patch?
I wanted to have your comments before sending a new patch regarding the refactoring of the templateURL creation. see below... http://codereview.chromium.org/335023/diff/9001/10002#newcode756 chrome/browser/tab_contents/render_view_context_menu.cc:756: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: { Should we look forward to do a refactoring with TabContents::GenerateKeywordIfNecessary() instead? Maybe we can add an method in template_url_model that will take as input the search url and build a default search TemplateURL. On 2009/11/23 19:59:47, Peter Kasting wrote: > Maybe the code in TemplateURLFetcher::RequestDelegate::OnURLFetchComplete() > should be refactored so that we can call it from here too, instead of the block > below.
TemplateURL stuff is probably better reviewed by Scott. I would go ahead and upload a patch that addresses as much stuff as you can.
Let me know when you do and I'll take a look at the TemplateURL side. -Scott On Mon, Nov 30, 2009 at 3:56 PM, <pkasting@chromium.org> wrote: > TemplateURL stuff is probably better reviewed by Scott. > > I would go ahead and upload a patch that addresses as much stuff as you can. > > http://codereview.chromium.org/335023 >
Updated Patch On 2009/11/30 23:59:18, sky wrote: > Let me know when you do and I'll take a look at the TemplateURL side. > > -Scott > > On Mon, Nov 30, 2009 at 3:56 PM, <mailto:pkasting@chromium.org> wrote: > > TemplateURL stuff is probably better reviewed by Scott. > > > > I would go ahead and upload a patch that addresses as much stuff as you can. > > > > http://codereview.chromium.org/335023 > > >
http://codereview.chromium.org/335023/diff/15002/15004 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/15002/15004#newcode756 chrome/browser/tab_contents/render_view_context_menu.cc:756: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: { Scott, here is the section about the TemplateURL that Peter was talking about. Maybe we can add a method in the TemplateURLModel that creates a default search TemplateUrl based on a given path and call the function from here? This can also be applied to TabContents::GenerateKeywordIfNecessary() and maybe this one also TemplateURLFetcher::RequestDelegate::OnURLFetchComplete() Philippe
http://codereview.chromium.org/335023/diff/15002/15004 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/15002/15004#newcode756 chrome/browser/tab_contents/render_view_context_menu.cc:756: case IDS_CONTENT_CONTEXT_ADDSEARCHENGINE: { On 2009/12/03 19:52:33, Philippe Beauchamp wrote: > Scott, here is the section about the TemplateURL that Peter was talking about. > Maybe we can add a method in the TemplateURLModel that creates a default search > TemplateUrl based on a given path and call the function from here? > > This can also be applied to > TabContents::GenerateKeywordIfNecessary() > and maybe this one also > TemplateURLFetcher::RequestDelegate::OnURLFetchComplete() Sure. You should also deal with the case where there is a TemplateURL matching keyword already.
Going through my old reviews, I see nothing ever happened on this. Not sure whether the onus was on Scott or Philippe to move here. Can either of you say what's needed to move forward?
On 2010/07/27 19:19:26, Peter Kasting wrote: > Going through my old reviews, I see nothing ever happened on this. Not sure > whether the onus was on Scott or Philippe to move here. Can either of you say > what's needed to move forward? As far as I know Philippe hasn't addressed my last comment. -Scott
Sorry for the delay. I started working on the issue but my priorities changed a little at that time. I'll have some time in the next weeks to have a look at it so that we can close this issue. I'll keep you posted. Philippe On Mon, Aug 2, 2010 at 1:01 PM, <sky@chromium.org> wrote: > On 2010/07/27 19:19:26, Peter Kasting wrote: > >> Going through my old reviews, I see nothing ever happened on this. Not >> sure >> whether the onus was on Scott or Philippe to move here. Can either of you >> say >> what's needed to move forward? >> > > As far as I know Philippe hasn't addressed my last comment. > > -Scott > > > http://codereview.chromium.org/335023/show >
Scott, I would like to have your input for the case when a keyword already exists. I'm currently opening the EditSearchEngineDialog using a new TemplateURL (exactly as if the user clicked on the ADD new search engine) with a filled TemplateURL (and the EditsearchEngineControllerDelegate NULL). If the keyword is already in use, the keyword field in EditSearchEngineDialog is marked as already existing (with an exclamation mark icon) and the Ok button is also disabled. This will force the user to enter a new keyword. This approach should work for most cases but might be annoying when the user wants to replace the current keyword. We can check before opening the EditSearchEngineDialog to see if there is already a matching keyword and load the matching one. It will not be possible in this case to simply add another keyword for the same search without deleting the previous one. Another solution is to allow to override an existing Keyword with the EditSearchEngineDialog (instead of disabling the Ok button). We will need to handle the case when a user edits a keyword and save it “as” another matching keyword. We will need to remove the first one and override the second one. When the user clicks the OK we might need a pop up a question asking to confirm the override. Do you have a better approach in mind? For the TemplateUrl refactoring, after looking at the function in detail, I don’t really see a gain to merge the GenerateKeywordIfNecessary with the current addsearchengine templateurl initialization. There are some small differences in both implementations. I think we can leave it like this, what do you think? Philippe
I spoke with Glen a fair amount before implementing the current behavior and we discussed the issues you raise here. We felt the current approach is simpler and less error prone, so I'm inclined not to mess with it. -Scott On Sun, Aug 22, 2010 at 9:19 PM, <philippe.beauchamp@gmail.com> wrote: > Scott, > > I would like to have your input for the case when a keyword already exists. > > I'm currently opening the EditSearchEngineDialog using a new TemplateURL > (exactly as if the user clicked on the ADD new search engine) with a filled > TemplateURL (and the EditsearchEngineControllerDelegate NULL). If the > keyword > is already in use, the keyword field in EditSearchEngineDialog is marked as > already existing (with an exclamation mark icon) and the Ok button is also > disabled. This will force the user to enter a new keyword. This approach > should > work for most cases but might be annoying when the user wants to replace the > current keyword. > > We can check before opening the EditSearchEngineDialog to see if there is > already a matching keyword and load the matching one. It will not be > possible > in this case to simply add another keyword for the same search without > deleting > the previous one. > > Another solution is to allow to override an existing Keyword with the > EditSearchEngineDialog (instead of disabling the Ok button). We will need > to > handle the case when a user edits a keyword and save it “as” another > matching > keyword. We will need to remove the first one and override the second one. > When > the user clicks the OK we might need a pop up a question asking to confirm > the > override. > > Do you have a better approach in mind? > > For the TemplateUrl refactoring, after looking at the function in detail, I > don’t really see a gain to merge the GenerateKeywordIfNecessary with the > current > addsearchengine templateurl initialization. There are some small differences > in > both implementations. I think we can leave it like this, what do you think? > > Philippe > > http://codereview.chromium.org/335023/show >
This means that the current implementation should be ok? We simply generates a keyword and open the EditSearchEngineDialog with a new filled TemplateURL. On 2010/08/23 16:15:34, sky wrote: > I spoke with Glen a fair amount before implementing the current > behavior and we discussed the issues you raise here. We felt the > current approach is simpler and less error prone, so I'm inclined not > to mess with it. > > -Scott
http://codereview.chromium.org/335023/diff/48001/49003 File chrome/browser/views/edit_search_engine_dialog.cc (right): http://codereview.chromium.org/335023/diff/48001/49003#newcode72 chrome/browser/views/edit_search_engine_dialog.cc:72: contents->keyword_tf_->RequestFocus(); I changed the default focus to keyword instead of title for the EditSearchEngineDialog. I think it's more useful when manually adding a keyword using right click on textbox.
Yes. Are you ready for another review? -Scott On Thu, Aug 26, 2010 at 10:32 PM, <philippe.beauchamp@gmail.com> wrote: > This means that the current implementation should be ok? We simply > generates a > keyword and open the EditSearchEngineDialog with a new filled TemplateURL. > > On 2010/08/23 16:15:34, sky wrote: >> >> I spoke with Glen a fair amount before implementing the current >> behavior and we discussed the issues you raise here. We felt the >> current approach is simpler and less error prone, so I'm inclined not >> to mess with it. > >> -Scott > > http://codereview.chromium.org/335023/show >
Yes, I uploaded an updated version in sync with head earlier this week. On 8/27/10, Scott Violet <sky@chromium.org> wrote: > Yes. Are you ready for another review? > > -Scott > > On Thu, Aug 26, 2010 at 10:32 PM, <philippe.beauchamp@gmail.com> wrote: >> This means that the current implementation should be ok? We simply >> generates a >> keyword and open the EditSearchEngineDialog with a new filled TemplateURL. >> >> On 2010/08/23 16:15:34, sky wrote: >>> >>> I spoke with Glen a fair amount before implementing the current >>> behavior and we discussed the issues you raise here. We felt the >>> current approach is simpler and less error prone, so I'm inclined not >>> to mess with it. >> >>> -Scott >> >> http://codereview.chromium.org/335023/show >> > -- Sent from my mobile device
http://codereview.chromium.org/335023/diff/48001/49002 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/48001/49002#newcode39 chrome/browser/tab_contents/render_view_context_menu.cc:39: #include "chrome/browser/views/edit_search_engine_dialog.h" browser/views is only appropriate for windows and linux/views. http://codereview.chromium.org/335023/diff/48001/49002#newcode1356 chrome/browser/tab_contents/render_view_context_menu.cc:1356: DCHECK(model); If NULL return, don't DCHECK. http://codereview.chromium.org/335023/diff/48001/49002#newcode1359 chrome/browser/tab_contents/render_view_context_menu.cc:1359: TemplateURL* template_url = new TemplateURL(); I think this leaks if the user cancels the add. http://codereview.chromium.org/335023/diff/48001/49003 File chrome/browser/views/edit_search_engine_dialog.cc (right): http://codereview.chromium.org/335023/diff/48001/49003#newcode72 chrome/browser/views/edit_search_engine_dialog.cc:72: contents->keyword_tf_->RequestFocus(); On 2010/08/27 05:34:20, Philippe Beauchamp wrote: > I changed the default focus to keyword instead of title for the > EditSearchEngineDialog. I think it's more useful when manually adding a keyword > using right click on textbox. Yes, but this isn't the common case.
http://codereview.chromium.org/335023/diff/48001/49002 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/48001/49002#newcode39 chrome/browser/tab_contents/render_view_context_menu.cc:39: #include "chrome/browser/views/edit_search_engine_dialog.h" I'm not sure how to handle and test the case with GTK, can you help me? Can I use this include instead? #include "chrome/browser/views/browser_dialogs.h" with: browser::EditSearchEngine(platform_util::GetTopLevel( source_tab_contents_->GetContentNativeView()), template_url, NULL, profile_); On 2010/08/30 16:31:46, sky wrote: > browser/views is only appropriate for windows and linux/views. http://codereview.chromium.org/335023/diff/48001/49002#newcode1356 chrome/browser/tab_contents/render_view_context_menu.cc:1356: DCHECK(model); On 2010/08/30 16:31:46, sky wrote: > If NULL return, don't DCHECK. Done. http://codereview.chromium.org/335023/diff/48001/49002#newcode1359 chrome/browser/tab_contents/render_view_context_menu.cc:1359: TemplateURL* template_url = new TemplateURL(); This code is called: void EditSearchEngineController::CleanUpCancelledAdd() { if (!edit_keyword_delegate_ && template_url_) { // When we have no Delegate, we know that the template_url_ hasn't yet been // added to the model, so we need to clean it up. delete template_url_; template_url_ = NULL; } } On 2010/08/30 16:31:46, sky wrote: > I think this leaks if the user cancels the add. http://codereview.chromium.org/335023/diff/48001/49003 File chrome/browser/views/edit_search_engine_dialog.cc (right): http://codereview.chromium.org/335023/diff/48001/49003#newcode72 chrome/browser/views/edit_search_engine_dialog.cc:72: contents->keyword_tf_->RequestFocus(); I think that changing the focus to keyword is also good for the case when you edit a search keyword. Maybe I'm not seeing the common case. I can revert. On 2010/08/30 16:31:46, sky wrote: > On 2010/08/27 05:34:20, Philippe Beauchamp wrote: > > I changed the default focus to keyword instead of title for the > > EditSearchEngineDialog. I think it's more useful when manually adding a > keyword > > using right click on textbox. > > Yes, but this isn't the common case.
http://codereview.chromium.org/335023/diff/48001/49002 File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/48001/49002#newcode39 chrome/browser/tab_contents/render_view_context_menu.cc:39: #include "chrome/browser/views/edit_search_engine_dialog.h" On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > I'm not sure how to handle and test the case with GTK, can you help me? > > Can I use this include instead? > #include "chrome/browser/views/browser_dialogs.h" > with: > browser::EditSearchEngine(platform_util::GetTopLevel( > source_tab_contents_->GetContentNativeView()), template_url, NULL, > profile_); The views directory is windows and chromeos specific. You'll need to route this call through the tab_contents_delegate. > On 2010/08/30 16:31:46, sky wrote: > > browser/views is only appropriate for windows and linux/views. > > http://codereview.chromium.org/335023/diff/48001/49002#newcode1359 chrome/browser/tab_contents/render_view_context_menu.cc:1359: TemplateURL* template_url = new TemplateURL(); On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > This code is called: > void EditSearchEngineController::CleanUpCancelledAdd() { > if (!edit_keyword_delegate_ && template_url_) { > // When we have no Delegate, we know that the template_url_ hasn't yet been > // added to the model, so we need to clean it up. > delete template_url_; > template_url_ = NULL; > } > } > > On 2010/08/30 16:31:46, sky wrote: > > I think this leaks if the user cancels the add. > > Great. Please add a comment that EditSearchEngineDialog takes ownership of template_url. http://codereview.chromium.org/335023/diff/48001/49003 File chrome/browser/views/edit_search_engine_dialog.cc (right): http://codereview.chromium.org/335023/diff/48001/49003#newcode72 chrome/browser/views/edit_search_engine_dialog.cc:72: contents->keyword_tf_->RequestFocus(); Yes, please revert. Thanks. On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > I think that changing the focus to keyword is also good for the case when you > edit a search keyword. Maybe I'm not seeing the common case. I can revert. > > On 2010/08/30 16:31:46, sky wrote: > > On 2010/08/27 05:34:20, Philippe Beauchamp wrote: > > > I changed the default focus to keyword instead of title for the > > > EditSearchEngineDialog. I think it's more useful when manually adding a > > keyword > > > using right click on textbox. > > > > Yes, but this isn't the common case. > >
Updated patch with changes. On 2010/09/08 15:57:51, sky wrote: > http://codereview.chromium.org/335023/diff/48001/49002 > File chrome/browser/tab_contents/render_view_context_menu.cc (right): > > http://codereview.chromium.org/335023/diff/48001/49002#newcode39 > chrome/browser/tab_contents/render_view_context_menu.cc:39: #include > "chrome/browser/views/edit_search_engine_dialog.h" > On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > > I'm not sure how to handle and test the case with GTK, can you help me? > > > > Can I use this include instead? > > #include "chrome/browser/views/browser_dialogs.h" > > with: > > browser::EditSearchEngine(platform_util::GetTopLevel( > > source_tab_contents_->GetContentNativeView()), template_url, NULL, > > profile_); > > The views directory is windows and chromeos specific. You'll need to route this > call through the tab_contents_delegate. > > > On 2010/08/30 16:31:46, sky wrote: > > > browser/views is only appropriate for windows and linux/views. > > > > > > http://codereview.chromium.org/335023/diff/48001/49002#newcode1359 > chrome/browser/tab_contents/render_view_context_menu.cc:1359: TemplateURL* > template_url = new TemplateURL(); > On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > > This code is called: > > void EditSearchEngineController::CleanUpCancelledAdd() { > > if (!edit_keyword_delegate_ && template_url_) { > > // When we have no Delegate, we know that the template_url_ hasn't yet > been > > // added to the model, so we need to clean it up. > > delete template_url_; > > template_url_ = NULL; > > } > > } > > > > On 2010/08/30 16:31:46, sky wrote: > > > I think this leaks if the user cancels the add. > > > > > > Great. Please add a comment that EditSearchEngineDialog takes ownership of > template_url. > > http://codereview.chromium.org/335023/diff/48001/49003 > File chrome/browser/views/edit_search_engine_dialog.cc (right): > > http://codereview.chromium.org/335023/diff/48001/49003#newcode72 > chrome/browser/views/edit_search_engine_dialog.cc:72: > contents->keyword_tf_->RequestFocus(); > Yes, please revert. > > Thanks. > On 2010/09/07 02:25:22, Philippe Beauchamp wrote: > > I think that changing the focus to keyword is also good for the case when you > > edit a search keyword. Maybe I'm not seeing the common case. I can revert. > > > > On 2010/08/30 16:31:46, sky wrote: > > > On 2010/08/27 05:34:20, Philippe Beauchamp wrote: > > > > I changed the default focus to keyword instead of title for the > > > > EditSearchEngineDialog. I think it's more useful when manually adding a > > > keyword > > > > using right click on textbox. > > > > > > Yes, but this isn't the common case. > > > >
http://codereview.chromium.org/335023/diff/64001/65003 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/335023/diff/64001/65003#newcode942 chrome/browser/tab_contents/tab_contents.cc:942: return; This leaks http://codereview.chromium.org/335023/diff/64001/65004 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/335023/diff/64001/65004#newcode375 chrome/browser/tab_contents/tab_contents.h:375: // a search engine. Mention this takes ownership. http://codereview.chromium.org/335023/diff/64001/65006 File third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h (right): http://codereview.chromium.org/335023/diff/64001/65006#newcode74 third_party/WebKit/WebKit/chromium/public/WebContextMenuData.h:74: WebURL keywordURL; Webkit changes have to happen on the WebKit tree.
Scott, are you the right person to give the changes on https://bugs.webkit.org/show_bug.cgi?id=47980 a once-over? If so, and you think they're OK, I can get someone with WK r+ powers to take it from there.
I added a comment to the review. -Scott On Wed, Oct 20, 2010 at 9:31 AM, <pkasting@chromium.org> wrote: > Scott, are you the right person to give the changes on > https://bugs.webkit.org/show_bug.cgi?id=47980 a once-over? If so, and you > think > they're OK, I can get someone with WK r+ powers to take it from there. > > http://codereview.chromium.org/335023/show >
I pinged on the WK side because I don't want to see this issue die.
Hello Peter, I uploaded a new patch. Still a small style error, I'll change it tonight. Can you review the rest? Main change is in render_view_context_menu.cc Philippe
LGTM http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1638: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile( Nit: Breaking after '=' would be a little more readable http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1640: Nit: Unnecessary blank line http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1645: TemplateURL* template_url = new TemplateURL(); Nit: Use a scoped_ptr<> here and then use "release()" in the conditional body below http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1656: Nit: Unnecessary blank line
Updated patch http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... File chrome/browser/tab_contents/render_view_context_menu.cc (right): http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1638: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile( On 2011/06/29 18:08:23, Peter Kasting wrote: > Nit: Breaking after '=' would be a little more readable Done. http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1640: On 2011/06/29 18:08:23, Peter Kasting wrote: > Nit: Unnecessary blank line Done. http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1645: TemplateURL* template_url = new TemplateURL(); On 2011/06/29 18:08:23, Peter Kasting wrote: > Nit: Use a scoped_ptr<> here and then use "release()" in the conditional body > below Done. http://codereview.chromium.org/335023/diff/111001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_context_menu.cc:1656: On 2011/06/29 18:08:23, Peter Kasting wrote: > Nit: Unnecessary blank line Done.
Presubmit check for 335023-114001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/common/view_messages.h Presubmit checks took 1.4s to calculate.
jam, can you OWNER-LGTM this?
view_messages LGTM
Try job failure for 335023-114001 (retry) on linux for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...
Philippe, I'm leaving this in your hands to fix try failures and retry checking the commit box.
ok, looks like the IDS_CONTENT_CONTEXT_ADDSEARCHENGINE string on linux and mac. On 2011/06/30 21:46:16, Peter Kasting wrote: > Philippe, I'm leaving this in your hands to fix try failures and retry checking > the commit box.
Change committed as 91253 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
