Add the feature "Add as search engine..." in a search text field context menu for chromium

Link to chromium review:
http://codereview.chromium.org/335023/show

Created attachment 71278 [details]
Proposed patch

[Peter Kasting]

Comment on attachment 71278 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71278&action=review

> WebKit/chromium/ChangeLog:64
> +>>>>>>> .r69954

You have a merge conflict marker here.

[Peter Kasting]

Also, why did you mark yourself r-?  Is this patch not yet what you want?

[Scott Violet]

I think you should split HasSuitableTextElement into at least two functions:

. FindSuitableTextElement: used if no element provided to the constructor.
. Another function that builds the encodedString.

Created attachment 71712 [details]
Proposed Patch V2 - Split of HasSuitableTextElement in two parts

Created attachment 71714 [details]
Proposed Patch V2 - Split of HasSuitableTextElement in two parts

[WebKit Review Bot]

Attachment 71714 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebSearchableFormData.cpp:233:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Scott Violet]

Comment on attachment 71714 [details]
Proposed Patch V2 - Split of HasSuitableTextElement in two parts

View in context: https://bugs.webkit.org/attachment.cgi?id=71714&action=review

> WebKit/chromium/src/WebSearchableFormData.cpp:147
> +WebCore::HTMLInputElement* HasSuitableTextElement(const HTMLFormElement* form, TextEncoding* encoding)

Rename to FindSuitableTextElement.

> WebKit/chromium/src/WebSearchableFormData.cpp:187
> +bool BuildSearchString(const HTMLFormElement* form, Vector<char>* encodedString, TextEncoding* encoding, const WebCore::HTMLInputElement* textElement)

This seems to always return true, so convert to void.

> WebKit/chromium/src/WebSearchableFormData.cpp:196
> +        if (!formElement->appendFormData(dataList, false))

You also do this in BuildSearchString. It should probably only be here.

> WebKit/chromium/src/WebSearchableFormData.cpp:235
> +            return;

Spacing looks off here.

Comment on attachment 71714 [details]
Proposed Patch V2 - Split of HasSuitableTextElement in two parts

View in context: https://bugs.webkit.org/attachment.cgi?id=71714&action=review

>> WebKit/chromium/src/WebSearchableFormData.cpp:196
>> +        if (!formElement->appendFormData(dataList, false))
> 
> You also do this in BuildSearchString. It should probably only be here.

Agree.  This is what I did at first but there is something strange with the "if (input->isTextField() && !items.isEmpty()"  the input->isTextField() returns true for a list.  We need to find a better way to determine that this is a list other than generating it.  I'll try to investigate tonight to find the exact explanation, still not clear for me.

[Adam Barth]

Comment on attachment 71714 [details]
Proposed Patch V2 - Split of HasSuitableTextElement in two parts

Please address the review comments above.

Created attachment 72172 [details]
Proposed Patch V3

[WebKit Review Bot]

Attachment 72172 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebSearchableFormData.cpp:159:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 72190 [details]
Proposed Patch V3 - removed \r char

Scott, 

What do you think about the formElement->willValidate() condition instead of generating the list?  Seems to be working fine. Do you see something that can break this?

Philippe

[WebKit Review Bot]

Attachment 72190 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 72190 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4851063

[Scott Violet]

Comment on attachment 72190 [details]
Proposed Patch V3 - removed \r char

View in context: https://bugs.webkit.org/attachment.cgi?id=72190&action=review

> WebKit/chromium/src/WebSearchableFormData.cpp:228
> +        && !selectedInputElement)

You need another set of parens here.

> WebKit/chromium/src/WebSearchableFormData.cpp:250
> +    }

Verify that if selectedInputElement is supplied that it's in the form.

[Adam Barth]

Comment on attachment 72190 [details]
Proposed Patch V3 - removed \r char

Does not compile.

Created attachment 72498 [details]
Proposed Patch V5

[WebKit Review Bot]

Attachment 72498 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebSearchableFormData.cpp:211:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 72499 [details]
Proposed Patch V5

[Scott Violet]

Comment on attachment 72499 [details]
Proposed Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=72499&action=review

> WebKit/chromium/src/WebSearchableFormData.cpp:181
> +bool BuildSearchString(const HTMLFormElement* form, Vector<char>* encodedString, TextEncoding* encoding, const WebCore::HTMLInputElement* textElement)

Document method and return value.

> WebKit/chromium/src/WebSearchableFormData.cpp:211
> +                FormDataBuilder::encodeStringAsFormData(*encodedString, j->data());

Add parens around this.

> WebKit/chromium/src/WebSearchableFormData.cpp:270
> +    if (!validSearchString)

Should this be before 266? Perhaps it should just be:

if (!BuildSear...())
  return;

Created attachment 72654 [details]
Proposed Patch V6

[WebKit Review Bot]

Attachment 72654 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebSearchableFormData.cpp:184:  Line contains invalid UTF-8 (or Unicode replacement character).  [readability/utf8] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 72499 [details]
Proposed Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=72499&action=review

>> WebKit/chromium/src/WebSearchableFormData.cpp:270
>> +    if (!validSearchString)
> 
> Should this be before 266? Perhaps it should just be:
> 
> if (!BuildSear...())
>   return;

The firstSubmitButton is set true at 261 and is set back to false at 267.  It will be left true if BuildSearchString returns false.

[Scott Violet]

The changes to WebSearchableFormData LGTM, you'll need to get someone else to review the changes to ContextMenuClientImpl.cpp.

Comment on attachment 72499 [details]
Proposed Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=72499&action=review

>> WebKit/chromium/src/WebSearchableFormData.cpp:270
>> +    if (!validSearchString)
> 
> Should this be before 266? Perhaps it should just be:
> 
> if (!BuildSear...())
>   return;

The firstSubmitButton is set true at 261 and is set back to false at 267.  It will be left true if BuildSearchString returns false.

Comment on attachment 72499 [details]
Proposed Patch V5

View in context: https://bugs.webkit.org/attachment.cgi?id=72499&action=review

>> WebKit/chromium/src/WebSearchableFormData.cpp:270
>> +    if (!validSearchString)
> 
> Should this be before 266? Perhaps it should just be:
> 
> if (!BuildSear...())
>   return;

The firstSubmitButton is set true at 261 and is set back to false at 267.  It will be left true if BuildSearchString returns false.

(In reply to comment #25)
> The changes to WebSearchableFormData LGTM, you'll need to get someone else to review the changes to ContextMenuClientImpl.cpp.

OK thanks, Peter, are you right person to have a look at it?

[Peter Kasting]

(In reply to comment #28)
> (In reply to comment #25)
> > The changes to WebSearchableFormData LGTM, you'll need to get someone else to review the changes to ContextMenuClientImpl.cpp.
> 
> OK thanks, Peter, are you right person to have a look at it?

I can't review this because I don't know how all the WebKit data structures here fit together.  For example, I can't tell you whether your method of obtaining the current form is the right one.  I'm not sure who knows this.  Maybe dglazkov knows who'd know.

I can tell you that you can combine some of your nested conditionals.

[Dimitri Glazkov (Google)]

Comment on attachment 72654 [details]
Proposed Patch V6

View in context: https://bugs.webkit.org/attachment.cgi?id=72654&action=review

> WebKit/chromium/public/WebSearchableFormData.h:48
> +    WEBKIT_API WebSearchableFormData(const WebFormElement&, const WebCore::HTMLInputElement* selectedInputElement = 0);

Please don't pass WebCore types into WebKit API. That's what WebInputElement/WebElement wrappers are for.

> WebKit/chromium/src/ContextMenuClientImpl.cpp:253
> +        WebCore::HTMLInputElement* selectedElement = static_cast<WebCore::HTMLInputElement*>(r.innerNonSharedNode());

What if it's not an HTMLInputElement?

> WebKit/chromium/src/WebSearchableFormData.cpp:147
> +WebCore::HTMLInputElement* FindSuitableTextElement(const HTMLFormElement* form)

Why do we have Capitalized methods in WebKit? That's not WebKit style http://webkit.org/coding/coding-style.html

> WebKit/chromium/src/WebSearchableFormData.cpp:187
> +bool BuildSearchString(const HTMLFormElement* form, Vector<char>* encodedString, TextEncoding* encoding, const WebCore::HTMLInputElement* textElement)

Ditto.

> WebKit/chromium/src/WebSearchableFormData.cpp:222
> +{
> +    bool isElementFound = false;   
> +
> +    // FIXME: Consider refactoring this code so that we don't call form->associatedElements() twice.
> +    for (Vector<HTMLFormControlElement*>::const_iterator i(form->associatedElements().begin()); i != form->associatedElements().end(); ++i) {
> +        HTMLFormControlElement* formElement = *i;
> +        if (formElement->disabled() || formElement->name().isNull())
> +            continue;
> +
> +        FormDataList dataList(*encoding);
> +        if (!formElement->appendFormData(dataList, false))
> +            continue;
> +
> +        const Vector<FormDataList::Item>& items = dataList.items();
> +
> +        for (Vector<FormDataList::Item>::const_iterator j(items.begin()); j != items.end(); ++j) {
> +            // Handle ISINDEX / <input name=isindex> specially, but only if it's
> +            // the first entry.
> +            if (!encodedString->isEmpty() || j->data() != "isindex") {
> +                if (!encodedString->isEmpty())
> +                    encodedString->append('&');
> +                FormDataBuilder::encodeStringAsFormData(*encodedString, j->data());
> +                encodedString->append('=');
> +            }
> +            ++j;
> +            if (formElement == textElement) {
> +                encodedString->append("{searchTerms}", 13);
> +                isElementFound = true;
> +            } else {
> +                FormDataBuilder::encodeStringAsFormData(*encodedString, j->data());
> +            }
> +        }
> +    }
> +    return isElementFound;
> +}

Can this somehow not be a mostly duplication of FormData logic?

> WebKit/chromium/src/WebSearchableFormData.cpp:284
> +    m_encoding = (String) encoding.name();

Whoa. Why are we casting things here like that?

[Peter Kasting]

Philippe, are you still around?  I'd hate to see this die.

I'm still around. I agree, we should move this forward. I’ll check this during the weekend. 
Philippe

(In reply to comment #31)
> Philippe, are you still around?  I'd hate to see this die.

Created attachment 90924 [details]
Proposed Patch V7

[WebKit Review Bot]

Attachment 90924 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/chromium/src/WebSearchableFormData.cpp:230:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebSearchableFormData.cpp:192:  Line contains invalid UTF-8 (or Unicode replacement character).  [readability/utf8] [5]
Source/WebKit/chromium/public/WebSearchableFormData.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/public/WebSearchableFormData.h:47:  Missing spaces around =  [whitespace/operators] [4]
Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:46:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 90932 [details]
Proposed Patch V7

[WebKit Review Bot]

Attachment 90932 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:46:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Hello Dimitri,

Can you have  look at the patch when you have some time? 

I changed the Webcore/Webkit api call and other items.  

I'm not sure about the conversion for the selectedInputElement into WebCore type in WebSearchableFormData, can you comment on this?

thanks,
Philippe  


(In reply to comment #35)
> Created an attachment (id=90932) [details]
> Proposed Patch V7

[Dimitri Glazkov (Google)]

Comment on attachment 90932 [details]
Proposed Patch V7

View in context: https://bugs.webkit.org/attachment.cgi?id=90932&action=review

Please address style elf's feedback.

> Source/WebKit/chromium/src/WebSearchableFormData.cpp:151
> +WebCore::HTMLInputElement* findSuitableSearchInputElement(const HTMLFormElement* form)

Don't need WebCore namespace qualifier here.

> Source/WebKit/chromium/src/WebSearchableFormData.cpp:165
> +        // Return nothing if an element is not in the default state
> +        // Return nothing if a text area is found.

This comment just repeats what the code does. Don't need it here.

Created attachment 93734 [details]
Proposed Patch V8

Updated Patch. 
-Removed comments
-Removed WebCore namespace qualifier 

Style error is false positive for Alphabetical Sorting problem
***
#include "HitTestResult.h"
#include "HTMLFormElement.h"
           ^
***
in \Tools\Scripts\webkitpy\style\checkers\cpp.py:2703
if previous_header_type == _OTHER_HEADER and previous_line.strip() > line.strip():

should be something like:
if previous_header_type == _OTHER_HEADER and previous_line.strip().lower() > line.strip().lower():

I'll report to check-webkit-style

[WebKit Review Bot]

Attachment 93734 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:46:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 94064 [details]
Proposed Patch V9

Solved the Alphabetical sorting problem, I didn't pay attention to the Style Guideline for this one.

[Dimitri Glazkov (Google)]

(In reply to comment #39)
> Created an attachment (id=93734) [details]
> Proposed Patch V8
> 
> Updated Patch. 
> -Removed comments
> -Removed WebCore namespace qualifier 
> 
> Style error is false positive for Alphabetical Sorting problem
> ***
> #include "HitTestResult.h"
> #include "HTMLFormElement.h"
>            ^
> ***
> in \Tools\Scripts\webkitpy\style\checkers\cpp.py:2703
> if previous_header_type == _OTHER_HEADER and previous_line.strip() > line.strip():
> 
> should be something like:
> if previous_header_type == _OTHER_HEADER and previous_line.strip().lower() > line.strip().lower():
> 
> I'll report to check-webkit-style

It's intentionally a case-sensitive sort. See http://www.webkit.org/coding/coding-style.html

[Dimitri Glazkov (Google)]

(In reply to comment #41)
> Created an attachment (id=94064) [details]
> Proposed Patch V9
> 
> Solved the Alphabetical sorting problem, I didn't pay attention to the Style Guideline for this one.

Ah, I see you got this :)

[Dimitri Glazkov (Google)]

Comment on attachment 94064 [details]
Proposed Patch V9

View in context: https://bugs.webkit.org/attachment.cgi?id=94064&action=review

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:277
> +        WebCore::HTMLFormElement* form = selectedFrame->selection()->currentForm();

Still don't need WebCore namespace here and elsewhere.

Created attachment 95844 [details]
Proposed Patch V10 - Removed WebCore namespace tags

[WebKit Commit Bot]

Comment on attachment 95844 [details]
Proposed Patch V10 - Removed WebCore namespace tags 

Rejecting attachment 95844 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8763145

[WebKit Commit Bot]

Comment on attachment 95844 [details]
Proposed Patch V10 - Removed WebCore namespace tags 

Clearing flags on attachment: 95844

Committed r88030: <http://trac.webkit.org/changeset/88030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.