1726853 - Remove unnecessary split() in UrlbarInput.search()

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P3)

Description

[Drew Willcoxon :adw]

UrlbarInput.search calls value.split to tokenize the search string, but it only uses the first token. There's no need to split the whole string.

split takes a second limit arg, so at least we could use that, assuming a limit of 1 doesn't actually cause the implementation to split or iterate over the whole string. A more straightforward approach would be to use value.match, or value.search followed by substring.

Comment 1

[Marco Bonardo [:mak]]

This could be a good-first-bug, it includes some investigation of the split behavior and comparison with other js methods to achieve the same.

Comment 2

[thapliyaladiti215]

Hello. I'm new here and I would like to ask if this bug is still unassigned? Can I work on this bug?

Comment 3

[Marco Bonardo [:mak]]

You're free to work on bugs unless there's a patch under-review already, or someone explained the bug is not fixable. Bugs are assigned to people once there's a patch.

Comment 4

[thapliyaladiti215]

Thanks. Can you please guide me on how can I start working on this task?

Comment 5

[Marco Bonardo [:mak]]

the basics are explained at https://firefox-source-docs.mozilla.org/setup/contributing_code.html
The bug is well described in comment 0, I think one should investigate the proposed option, run some js benchmark on them (can use jsbench) and post a patch explaining why that solution was picked.

Comment 6

[Tanmay]

Hey, is this issue up for grabs?

Comment 7

[Harry Twyford]

It is if it isn't being worked on. thapliyaladiti215, are you working on this?

Comment 8

[Tanmay]

(In reply to Harry Twyford [:harry] from comment #7)

It is if it isn't being worked on. thapliyaladiti215, are you working on this?

Hey, Harry can I work on the issue?

Comment 9

[Harry Twyford]

Go for it!

Comment 10

[Tanmay]

(In reply to Harry Twyford [:harry] from comment #9)

Go for it!

Sure, Thank you!

Comment 11

[Sean Pedigo]

Maybe there is something I don't get but, I can't find any documentation that explains plainly what this does. I changed a few things and I couldn't see the changes reflected after rebuilding and running it again. I suspect this may be because I am approaching this from a position of ignorance.

I tried printing the tokens to the console, but nothing was being printed. So I created a fake variable to replace the tokens variable. I thought that this might make Firefox search for the fake variable rather than what was typed in the urlbar. Instead it didn't seem to make a difference. The query was sent to Google just as it was typed.

I thought, well maybe if I could debug this. However, I can't find any documentation on how you would run a debugger for this. Maybe it's because I rarely write JavaScript. So I figured what the heck, best way to find a bug with the function is to see what goes wrong if it's missing. So I just deleted the entire function to see what would happen. Rebuilt, and reran it. Again, it didn't seem to make a difference. It didn't even throw an error saying that it was missing.

You would think that with a name like search it would be self evident, at least, what it does. In the end it feels like I'm coming away from this without accomplishing much. I guess I'll push a commit where whole line isn't split. Doesn't seem like much of a performance improvement if it doesn't matter whether or not the code is present, though.

Not sure if this makes a difference, but for reference I am running this on Linux, and using an artifact build. I am wondering if perhaps my results would be different if I was running the full build and/or I was running this on Windows. Maybe I should try this without Firefox being installed locally already? I'm not sure how to approach this, any help would be appreciated it.

Comment 12

[Simon Farre]

As Sean Pedigo pointed out;
The UrlbarInput.search method does nothing. The UrlbarInput.search is only for when a new tab is open and for the bar that says "Search with google or enter address" (meaning, not the actual address bar at the top but what I believe was used to be called the awesome bar if I am not mistaken, right under the firefox logo about one third of the window down).

As soon as the user enters input into that bar, Firefox will focus on the address bar at the top instead. If we do simple debug console logging from UrlbarInput.search it will fire once and then never again (unless we focus that bar below specifically - i.e. not the one at the top).

Thus this bug does not have anything to do with split or trim or string editing for that matter either, it seems to be a larger bug or issue as it seems we are calling "dead code" or code that on a surface level seems to do nothing, but unnecessary work.q

Comment 13

[Simon Farre]

Attachment: Bug 1726853 - Removed splitting of string. r=mak!,adw

I've added the most minimal changes I could, but I also removed some code bloat,
by moving the assignment of this.inputField out of the if-control block's branches.
Also changed the deprecated use of initUIEvent at the bottom of method to adhere
to Mozilla's own advice of using new UIEvent instead.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Simon Farre from comment #12)

Thus this bug does not have anything to do with split or trim or string editing for that matter either, it seems to be a larger bug or issue as it seems we are calling "dead code" or code that on a surface level seems to do nothing, but unnecessary work.

There is a pref that controls the handoff from the new tab page to the urlbar, browser.newtabpage.activity-stream.improvesearch.handoffToAwesomebar, that defaults to true from Firefox 89, it's possible as a consequence some of the code is currently not invoked, but it could be if you flip that pref.
Afaik this bug was mostly a small code optimization.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16d7109ea43d
Removed splitting of string. r=mak

Comment 16

[Iulian Moraru]

Backed out for causing bc failures on browser_searchFunction.js

Push with failures

Failure log

Backout link

Comment 17

[Simon Farre]

(In reply to Iulian Moraru from comment #16)

Backed out for causing bc failures on browser_searchFunction.js

Push with failures

Failure log

Backout link

I've fixed the bug I introduced and resubmitted the patch, with succeeding tests. Just to be sure -
I just had to say hg commit --amend then type in/change commit message and then do moz-phab again, right?

Comment 18

[Marco Bonardo [:mak]]

(In reply to Simon Farre from comment #17)

I just had to say hg commit --amend then type in/change commit message and then do moz-phab again, right?

YEP!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d67f94aa56c5
Removed splitting of string. r=mak

Comment 20

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d67f94aa56c5