Closed Bug 1455326 Opened 5 years ago Closed 5 years ago

When browser.search.openintab is true, reuse current tab if it is blank

Categories

(Firefox :: Search, enhancement, P3)

Product:
Firefox
Component:
Search
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: epang, Assigned: mkaply)

References

Details

Attachments

(1 file)

Bug 1455326 - Don't open search in new tab if current tab is blank.
59 bytes, text/x-review-board-request
adw
: review+
Details 
To remain consistent with the prefs browser.urlbar.openintab and browser.tabs.loadBookmarksInTabs we should also update browser.search.openintab to reuse current new tab (if it's the active tab).
How do you define blank?
In browser/base/content/browser.js, there is a function for that.

https://searchfox.org/mozilla-central/search?q=isTabEmpty&path=
See bug1458534 for external links.
Blocks: 1394304
Priority: -- → P3
Any news about this?
Comment on attachment 8990150 [details]
Bug 1455326 - Don't open search in new tab if current tab is blank.

https://reviewboard.mozilla.org/r/255152/#review262052

Thanks Mike!

::: browser/components/search/content/search.xml:286
(Diff revision 1)
>                where += "-background";
>            } else {
>              var newTabPref = Services.prefs.getBoolPref("browser.search.openintab");
>              if (((aEvent instanceof KeyboardEvent) && aEvent.altKey) ^ newTabPref)
>                where = "tab";
> +            if (where == "tab" && isTabEmpty(gBrowser.selectedTab)) {

This should get the job done, but you could write it a little more simply IMO:

  if (((aEvent instanceof KeyboardEvent && aEvent.altKey) ^ newTabPref) &&
      !isTabEmpty(gBrowser.selectedTab)) {
    where = "tab";
  }

Since `where` is "current" by default.  Writing it this way also emphasizes that this is connected to the newTabPref and not e.g. the mouse check right below this.

But up to you, r+ either way

::: browser/components/search/content/search.xml:1791
(Diff revision 1)
>              }
>            } else {
>              var newTabPref = Services.prefs.getBoolPref("browser.search.openintab");
>              if (((aEvent instanceof KeyboardEvent) && aEvent.altKey) ^ newTabPref)
>                where = "tab";
> +            if (where == "tab" && isTabEmpty(gBrowser.selectedTab)) {

Here too of course
Attachment #8990150 - Flags: review?(adw) → review+
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/aa2c71e82371
Don't open search in new tab if current tab is blank. r=adw
Thanks for this, Mike!
Just leaving this here, as it's the last piece missing from this "puzzle" with this and bug1417133 being fixed: bug1458534.
https://hg.mozilla.org/mozilla-central/rev/aa2c71e82371
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.