Fennec should hint to necko about any items that are clickable in the awesome screen

RESOLVED WONTFIX

Status

()

Firefox for Android
General
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: blassey, Assigned: Mina Almasry)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment)

Necko exposes an API where it can start warming itself up (up to the point of having an https connection already open and ready) in anticipation of the user needing to go somewhere. We could use this for awesome screen items as they come into view. That should decrease the time between when the user clicks on a url and when they can see the page
(Reporter)

Updated

6 years ago
tracking-fennec: ? → +
I assume we would use nsISpeculativeConnect as the way to send the hint. I don't think sending 6, 12, or whatever number of URLs through nsISpeculativeConnect.speculativeConnect(...) is appropriate. We certainly don't want to waste memory or bandwidth resources.

Comment 2

6 years ago
CC'ing Patrick McManus, who worked on connection hints.
Assignee: nobody → malmasry
(Assignee)

Comment 3

5 years ago
Created attachment 809312 [details] [diff] [review]
neck-hint.patch

So this hints to necko about the top 3 none search items: items from the history and bookmarks.

Let me know if that's what you want, and if there is anything else to do.

Would you like a test with this? Where do I start with writing one?
Attachment #809312 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 809312 [details] [diff] [review]
neck-hint.patch

Review of attachment 809312 [details] [diff] [review]:
-----------------------------------------------------------------

Bouncing feedback request to margaret - I think somebody who's worked on BrowserSearch is probably a better person to do this review. At a high level it looks like it's doing the right thing, though.
Attachment #809312 - Flags: feedback?(bugmail.mozilla) → feedback?(margaret.leibovic)

Comment 5

5 years ago
Comment on attachment 809312 [details] [diff] [review]
neck-hint.patch

Review of attachment 809312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a sensible approach. I mainly just have some style/code clarity comments.

::: mobile/android/base/home/BrowserSearch.java
@@ +804,5 @@
>              }
>          }
>  
> +        private void sendNeckoHint(Cursor c) {
> +          if (c == null || !c.moveToFirst()) {

Style nit: Please use 4-space indentation in Java files.

@@ +814,5 @@
> +            final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
> +            if (url.startsWith("about:")) {
> +              continue;
> +            }
> +            urls += url + " ";

I would prefer to see us create a JSONArray, and then use toString() to convert it to a string to pass in the message to JS.

@@ +816,5 @@
> +              continue;
> +            }
> +            urls += url + " ";
> +            i++;
> +          } while(i < 3 && c.moveToNext());

I think it would be clearer if you just created a for loop, and then just call c.moveToNext() at the top of it (and bail if that fails).

::: mobile/android/chrome/content/browser.js
@@ +1481,5 @@
>          this.notifyPrefObservers(aData);
>          break;
>  
> +      case "Necko:Hint":
> +        var urls = aData.split(" ");

Use let instead of var, and you should add braces around the body of the case statement whenever you declare local variables, so that they don't creep out into the rest of the switch statement.

Also, if you pass a stringified JSONArray, you would replace this split call with JSON.parse(aData).

@@ +1482,5 @@
>          break;
>  
> +      case "Necko:Hint":
> +        var urls = aData.split(" ");
> +        for (var i = 0; i < urls.length; i++) {

I would prefer to see you use forEach here (e.g. urls.forEach(function(url) { ... }).
Attachment #809312 - Flags: feedback?(margaret.leibovic) → feedback+
(Assignee)

Comment 6

5 years ago
also, the patch for this bug is applied on top of the one for this: https://bugzilla.mozilla.org/show_bug.cgi?id=813380
(Assignee)

Comment 7

5 years ago
oops. Nope, never mind. The patch for the other bug is on top of this one.
(Assignee)

Comment 8

5 years ago
So the concerns from Bug 813379 carry over to this one: the concern is a performance hit sending JSON from java to JS, and sending the speculative connections. What do we want to do here?

I believe the most reasonable option is to wait for the user to finish typing the search term, then send one JSON message from Java to JS that contains a list of clickable links in the awesome screen that Necko should send hints too.

Is this reasonable? How many links should we send a speculative connection to? I suggest sending one to the auto completed URL in the awesomebar, plus maybe the top clickable link in the awesome screen.

Or should we not send any more hints at all and mark this WONTFIX/leave it for now?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(blassey.bugs)
(Assignee)

Comment 9

5 years ago
I should add. Measuring the cost of sending a JSON message is difficult. I can look further into it if it's really needed. Measuring the time to send a speculative connection or page load should be easy to do though.
(In reply to Mina Almasry [:mina] from comment #8)
> So the concerns from Bug 813379 carry over to this one: the concern is a
> performance hit sending JSON from java to JS, and sending the speculative
> connections. What do we want to do here?
I wouldn't send json messages, I'd instead send events.

Note: json messages are events, but take longer to process, just add a new event type in GeckoEvent

> I believe the most reasonable option is to wait for the user to finish
> typing the search term, then send one JSON message from Java to JS that
> contains a list of clickable links in the awesome screen that Necko should
> send hints too.
figuring out when the user is done typing is hard. I'd suggest sending the hints early and often. If that volume turns out to be a problem then we can look at ways to address it. Don't try to prematurely optimize.

> Is this reasonable? How many links should we send a speculative connection
> to? I suggest sending one to the auto completed URL in the awesomebar, plus
> maybe the top clickable link in the awesome screen.
I'd suggest sending one for everything that is visible on the on the screen. Note that they can be batched up in the same event.
 
> Or should we not send any more hints at all and mark this WONTFIX/leave it
> for now?
Don't understand that reasoning.
Flags: needinfo?(blassey.bugs)
(Assignee)

Updated

5 years ago
Flags: needinfo?(mark.finkle)
I want to WONTFIX this bug. Speculatively connecting to lots of clickable things on the Awesomescreen sounds crazy. We should be more focused than that. We don't even know what kind of improvements we'd get from doing this shotgun approach.

> Don't try to prematurely optimize.

This bug *is* premature optimization

Bug 813379 is a good start and very focused. Let's see if any benefits come from that. Although, I don't know if we have any way to even find that out.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
mark, you should talk with nick hurley about his seer work
(In reply to Patrick McManus [:mcmanus] from comment #12)
> mark, you should talk with nick hurley about his seer work

Bug 881804 I assume
You need to log in before you can comment on or make changes to this bug.