Some cleanups in search and sidebar code found by SeaMonkey's OpenSearch work

RESOLVED FIXED in Firefox 7

Status

()

Firefox
Search
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

Trunk
Firefox 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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. :)

Updated

7 years ago
No longer depends on: 410613

Updated

7 years ago
Depends on: 410613
(Assignee)

Comment 1

7 years ago
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.
Attachment #479809 - Flags: review?(gavin.sharp)
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.
Attachment #479809 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 3

7 years ago
(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.
That mirrors our behavior for "open link in new tab", fwiw.
(Assignee)

Comment 5

7 years ago
(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?
(Assignee)

Updated

7 years ago
Blocks: 612965
(Assignee)

Comment 6

7 years ago
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.
Attachment #479809 - Attachment is obsolete: true
Attachment #491270 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Blocks: 643172
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 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.
Attachment #491270 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 9

6 years ago
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).
Attachment #491270 - Attachment is obsolete: true
Attachment #534224 - Flags: review?(gavin.sharp)
Attachment #534224 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 10

6 years ago
No time to watch the tree right now, so setting checkin-needed.
Keywords: checkin-needed
(Assignee)

Comment 11

6 years ago
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.
Attachment #534224 - Attachment is obsolete: true
Attachment #534537 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/063f67e146ec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Updated

4 years ago
Blocks: 453980
You need to log in before you can comment on or make changes to this bug.