Convert search.xml to use api from bug 545714

RESOLVED FIXED

Status

()

Firefox
Search
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 434288 [details] [diff] [review]
patch
Attachment #434288 - Flags: review?(rflint)
Can we add a test for search bar dropping, too?
(Assignee)

Updated

8 years ago
Attachment #434288 - Flags: review?(rflint)
(Assignee)

Comment 2

8 years ago
Created attachment 434860 [details] [diff] [review]
version 2

That previous patch was completely wrong. Here is the real version.
Attachment #434288 - Attachment is obsolete: true
Attachment #434860 - Flags: review?(gavin.sharp)
Comment on attachment 434860 [details] [diff] [review]
version 2

This patch applied on top of attachment 434292 [details] [diff] [review] seems to break drag-to-search (the text is dropped but no search is triggered). search.xml's drop handler just isn't called.

The test should probably cover that... I guess that's a bit of a pain since you'd need a search engine that doesn't hit the network, but you can do that relatively easily the same way e.g. browser_415700.js does. You should probably also do the test in a new tab to avoid interfering with other tests.
Attachment #434860 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 4

8 years ago
Looks like there is also a dependency on the patch in bug 539476 as well. Otherwise the default editor listener will respond instead. This is likely why the original code used a capturing listener. The patch in bug 539476 changes the editor listeners to respond during the system group phase so that they act more like default handlers.
Depends on: 539476
Comment on attachment 434860 [details] [diff] [review]
version 2

OK, r=me with the test changes (doing the test in a new tab at the very least, since as it is now the test causes a load of Google and doesn't clean it up).
Attachment #434860 - Flags: review- → review+
(Assignee)

Comment 6

8 years ago
Created attachment 434949 [details] [diff] [review]
version 3

Adjust test to check if the search starts a load. Uses an existing test which already creates some new tabs.
Attachment #434860 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d9c5998f3e41
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
i suppose we can dupe Bug 545122 to this one(?)...

Updated

8 years ago
Duplicate of this bug: 545122
You need to log in before you can comment on or make changes to this bug.