Last Comment Bug 570266 - AddKeywordForSearchField resolves form action URI incorrectly
: AddKeywordForSearchField resolves form action URI incorrectly
Status: RESOLVED FIXED
[good first bug][mentor=gavin]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 25
Assigned To: Steven MacLeod [:smacleod]
:
:
Mentors:
Depends on: 891500 566128
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-04 18:18 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-07-09 12:27 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.15 KB, patch)
2013-06-26 09:38 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review
Patch (5.86 KB, patch)
2013-06-27 07:12 PDT, Steven MacLeod [:smacleod]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch (5.95 KB, patch)
2013-06-27 08:32 PDT, Steven MacLeod [:smacleod]
no flags Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-04 18:18:57 PDT
It uses the document URI rather than the form's baseURI when calling newURI on the action attribute's value.

(Also it can use the property rather than the attribute, since XPCNativeWrappers mean we don't need to worry about bug 264607.)
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-18 13:17:32 PDT
It can just use form.mozActionURI once the relevant patch in bug 566128 lands.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-01-05 03:13:47 PST
(In reply to comment #1)
> It can just use form.mozActionURI once the relevant patch in bug 566128 lands.

FTR: form.mozActionUri was introduced and then removed through bug 607145.
Comment 3 Liz Henry (:lizzard) (needinfo? me) 2013-06-03 14:20:51 PDT
Hey Gavin, this has been marked as a good first bug for some time now but no one's taken it up. Maybe it would do better if it were a mentored bug. What do you think?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-03 14:28:03 PDT
Sure.
Comment 5 Steven MacLeod [:smacleod] 2013-06-26 09:38:02 PDT
Created attachment 767822 [details] [diff] [review]
Patch
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-26 09:56:13 PDT
This could be a good opportunity to gain some test-writing experience!

This is testable with a browser-chrome test, I think (https://developer.mozilla.org/en-US/docs/Browser_chrome_tests).

An example of a test that loads a page and tests how chrome interacts with it is http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug581947.js. You could create a new test next to that (maybe name it browser_addKeywordSearch.js), have it load a page with a form whose baseURI is non-default, then call  AddKeywordForSearchField directly and check that it creates a keyword with the right URI.

Ping me in person if you want to chat about this more!
Comment 7 Steven MacLeod [:smacleod] 2013-06-27 07:12:34 PDT
Created attachment 768297 [details] [diff] [review]
Patch

New patch including tests.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-27 07:54:22 PDT
Comment on attachment 768297 [details] [diff] [review]
Patch

Looks good! A couple of nits:

>diff --git a/browser/base/content/test/browser_addKeywordSearch.js b/browser/base/content/test/browser_addKeywordSearch.js

This file needs a license header (see https://www.mozilla.org/MPL/license-policy.html). Just use the "public domain" one at https://www.mozilla.org/MPL/headers/.

>+function test() {

>+  gBrowser.selectedBrowser.addEventListener("load", function() {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

arguments.callee is deprecated, give the anonymous event handler function a name instead, and just refer to that.

>+      base.appendChild(form);

IIRC the <base> tag is supposed to be standalone in the <head>, and not have any children? It's a bit odd to be inserting the form into it, but I suppose it doesn't matter much.

>+      is(data.spec, expected, "Bookmark spec for search field named " + fieldName + " and baseURI " + baseURI + " incrorect");

typo "incrorect"

>+    for each (let data in testData) {

for...of is the new hotness (preferred over less-standard for each)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

>+  gBrowser.selectedTab = gBrowser.addTab();

>+  gBrowser.selectedBrowser.addEventListener("load", function() {
>+  }, true);
>+  content.location = "http://example.org/browser/browser/base/content/test/dummy_page.html";

This pattern can lead to some tricky intermittent failures, because the addTab() call triggers a load of about:blank in that tab, which can race with the load of example.org that you trigger later. Your load listener could then run against about:blank instead of the page you're expecting. That may not matter in this particular test (since I don't think you rely on the page contents at all?), but probably worth avoiding lest someone cargo cult it. I think you can fix it by passing the URL you want to load to addTab(), so that it avoids loading about:blank and just loads the right URL directly.

r=me with those comments addressed
Comment 9 Steven MacLeod [:smacleod] 2013-06-27 08:32:22 PDT
Created attachment 768348 [details] [diff] [review]
Patch

Patch updated based on Gavin's review.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-07-01 06:32:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc0f2db4add
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:48:49 PDT
https://hg.mozilla.org/mozilla-central/rev/3fc0f2db4add

Note You need to log in before you can comment on or make changes to this bug.