Last Comment Bug 600244 - Some cleanups in search and sidebar code found by SeaMonkey's OpenSearch work
: Some cleanups in search and sidebar code found by SeaMonkey's OpenSearch work
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
Depends on: 410613
Blocks: 453980 612965 643172
  Show dependency treegraph
 
Reported: 2010-09-28 10:18 PDT by Robert Kaiser (not working on stability any more)
Modified: 2013-03-18 03:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: port back fixes (11.88 KB, patch)
2010-09-30 10:03 PDT, Robert Kaiser (not working on stability any more)
gavin.sharp: review-
Details | Diff | Review
v1.1: update for review comments (11.35 KB, patch)
2010-11-17 11:53 PST, Robert Kaiser (not working on stability any more)
gavin.sharp: review-
Details | Diff | Review
v1.2: unbitrotted, typo fixed (9.43 KB, patch)
2011-05-21 06:59 PDT, Robert Kaiser (not working on stability any more)
gavin.sharp: review+
Details | Diff | Review
v1.2 in checkin-ready format, with hg patch header including user name and comment (6.58 KB, patch)
2011-05-23 13:18 PDT, Robert Kaiser (not working on stability any more)
kairo: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-09-28 10:18:34 PDT
In bug 410613, I'm working on bringing OpenSearch to SeaMonkey, and reviews as well as review-inspired work found some cleanups in search and sidebar code that are worth porting back to Firefox to improve all our lives. :)
Comment 1 Robert Kaiser (not working on stability any more) 2010-09-30 10:03:23 PDT
Created attachment 479809 [details] [diff] [review]
v1: port back fixes

And here's the patch to port back those fixes, I hope they look good for your side as well.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-30 11:24:07 PDT
Comment on attachment 479809 [details] [diff] [review]
v1: port back fixes

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

>-      openUILinkIn(Services.search.defaultEngine.searchForm, "current");
>+      loadURI(Services.search.defaultEngine.searchForm);

Don't want this change - openUILinkIn handles the "current tab is an app tab" case differently (uses a new tab).

>-      gBrowser.loadOneTab(submission.uri.spec, {
>-                          postData: submission.postData,
>-                          relatedToCurrent: true});
>+      gBrowser.loadOneTab(submission.uri.spec,
>+                          { postData: submission.postData,
>+                            relatedToCurrent: true,
>+                            inBackground: false });

What's the justification for the addition of inBackground? Even though it might make sense, I'd rather not include the behavior change in a "cleanup" patch.
Comment 3 Robert Kaiser (not working on stability any more) 2010-09-30 11:50:45 PDT
(In reply to comment #2)
> >-      openUILinkIn(Services.search.defaultEngine.searchForm, "current");
> >+      loadURI(Services.search.defaultEngine.searchForm);
> 
> Don't want this change - openUILinkIn handles the "current tab is an app tab"
> case differently (uses a new tab).

Ah, OK, interesting to know! Without that info, it sounds strange to go through openUILink() there. ;-)

> What's the justification for the addition of inBackground? Even though it might
> make sense, I'd rather not include the behavior change in a "cleanup" patch.

Ah, right, I almost forgot that was in there when naming this bug as it is. I surely can split this off to yet another bug. It made not much sense to me or Neil that searches one opens intentionally via direct user interaction would appear in background tabs.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-01 13:35:17 PDT
That mirrors our behavior for "open link in new tab", fwiw.
Comment 5 Robert Kaiser (not working on stability any more) 2010-10-04 13:10:52 PDT
(In reply to comment #4)
> That mirrors our behavior for "open link in new tab", fwiw.

True, though opening tabs from the UI is different from opening them from a web page in that you are not reading content when issuing the page load from the UI, like a search or opening a "UI tab" like Add-ons Manager (where we actually always oping in the foreground even in Firefox, I think).

You're right though in that it probably something to be thinking about more closely, I'll split this off into a separate bug - do we need a word from UX on this?
Comment 6 Robert Kaiser (not working on stability any more) 2010-11-17 11:53:21 PST
Created attachment 491270 [details] [diff] [review]
v1.1: update for review comments

Here's a new patch updated for the review comments, I've split off the background/foreground matter into bug 612965 instead.
Comment 7 Dão Gottwald [:dao] 2011-04-07 13:38:16 PDT
Comment on attachment 491270 [details] [diff] [review]
v1.1: update for review comments

>@@ -3248,19 +3248,19 @@ const BrowserSearch = {
>     // getSubmission can return null if the engine doesn't have a URL
>     // with a text/html response type.  This is unlikely (since
>     // SearchService._addEngineToStore() should fail for such an engine),
>     // but let's be on the safe side.
>     if (!submission)
>       return;
> 
>     if (useNewTab) {
>-      gBrowser.loadOneTab(submission.uri.spec, {
>-                          postData: submission.postData,
>-                          relatedToCurrent: true});
>+      gBrowser.loadOneTab(submission.uri.spec,
>+                          { postData: submission.postData,
>+                            relatedToCurrent: true });
>     } else
>       loadURI(submission.uri.spec, null, submission.postData, false);

This code doesn't exist anymore, see bug 641090.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-06 13:08:37 PDT
Comment on attachment 491270 [details] [diff] [review]
v1.1: update for review comments

>diff --git a/browser/components/sidebar/src/nsSidebar.js b/browser/components/sidebar/src/nsSidebar.js

>-  this.searchService.addEngine(aDescriptionURL, typeXML, iconURL, true);
>+  Servcies.search.addEngine(aDescriptionURL, typeXML, iconURL, true);

typo: Servcies

There's a lot of bitrot here, but all of the changes look fine. I'll quickly review an updated patch (ideally one that's been tested :), sorry for taking forever.
Comment 9 Robert Kaiser (not working on stability any more) 2011-05-21 06:59:36 PDT
Created attachment 534224 [details] [diff] [review]
v1.2: unbitrotted, typo fixed

Here's an unbitrotted version that I tested for the things I knew how to test (sidebars working, installation of search plugins working).
Comment 10 Robert Kaiser (not working on stability any more) 2011-05-23 13:11:36 PDT
No time to watch the tree right now, so setting checkin-needed.
Comment 11 Robert Kaiser (not working on stability any more) 2011-05-23 13:18:30 PDT
Created attachment 534537 [details] [diff] [review]
v1.2 in checkin-ready format, with hg patch header including user name and comment

As for the checkin-needed, this has the user name and comment in a "hg export" format.
Comment 12 Dão Gottwald [:dao] 2011-05-25 00:35:36 PDT
http://hg.mozilla.org/mozilla-central/rev/063f67e146ec

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