Closed Bug 1019989 Opened 6 years ago Closed 6 years ago

ContentSearch should pass images as shared ArrayBuffers instead of data URI strings

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33

People

(Reporter: adw, Assigned: adw)

References

Details

(Whiteboard: p=3 s=33.1 [qa-])

Attachments

(1 file, 2 obsolete files)

ContentSearch.jsm passes images to clients using base 64-encoded data URI strings, which can be pretty large, especially for the large 130x52 provider logos.  Instead it should pass images as shared ArrayBuffers.

The TART regression in bug 1004429 was nearly halved by using ArrayBuffers instead of strings according to http://compare-talos.mattn.ca/, but I'm not sure I trust those numbers.  See bug 1004429 comment 47.
Flags: firefox-backlog+
Whiteboard: p=3 → p=3 [qa-]
Whiteboard: p=3 [qa-] → p=3 s=33.1 [qa-]
Attached patch patch (obsolete) — Splinter Review
This updates ContentSearch.jsm and the front-end consumer.  It also updates the two affected tests.  It makes browser_ContentSearch.js use add_task instead of the custom generator-based framework it was using.  When I wrote that test, I didn't know that add_task was available in browser tests.

Since this modifies parts of the code that touch on Gavin's drive-by comments in bug 1009313 comment 3, this also stops lazily importing common JSMs and doesn't call _initService on observe() in ContentSearch.jsm.

https://tbpl.mozilla.org/?tree=Try&rev=96f6395a2d07
Attachment #8439627 - Flags: review?(felipc)
Comment on attachment 8439627 [details] [diff] [review]
patch

The try run failed.  The asynchronicity added by turning the image URIs into ArrayBuffers means that the search state, like the current engine, can change while ContentSearch is in the middle of responding to an input ("input" meaning either a message from content or an observed search service notification).  So responses can be out of date.  Plus, responses are no longer guaranteed to happen in FIFO order.  So ContentSearch can be in the middle of responding to one input A, receive another input B, and actually respond to B before A's response is ready.

It'll have to queue up inputs and process them serially or something.  I'm not sure whether ContentSearch should never send out of date responses or consumers should be prepared to handle them.
Attachment #8439627 - Flags: review?(felipc)
Attached patch patch 2 (obsolete) — Splinter Review
This adds inbound events to a queue so they're processed and responded to in FIFO order in ContentSearch.jsm.

This also creates a new CurrentState outbound message.  Previously, responses to the inbound GetState message and observed changes to the current engine both resulted in an outbound State message, which made it hard for tests to determine whether a State event was in response to the page requesting its initial search state (via GetState) or a spurious message triggered by a previous test changing the current engine.  With the two cases separated, tests can now just wait for the State event to tell when the page has set up its search state, and they don't get tripped up by spurious changes to the current engine.

This also moves whenSearchInitDone into newtab's head.js so that individual tests don't have to call it and aren't bitten when they don't call it.

And it also makes the changes that I noted in the comment for the previous patch.

This turned out to be more work than I thought.  Sorry for the big patch.

Green try run for a messy version of this patch: https://tbpl.mozilla.org/?tree=Try&rev=24561f1b04a9
Attachment #8439627 - Attachment is obsolete: true
Attachment #8440245 - Flags: review?(felipc)
Comment on attachment 8440245 [details] [diff] [review]
patch 2

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

For the common pattern of:
>  func: function () {
>   return Task.spawn(function* () {
>  
>   }.bind(this));
>  },

There's this Task.async helper now that does the same thing, visually simplifying the code a little bit. You might use it if you want, depends if you like the syntactic sugar.

::: browser/modules/ContentSearch.jsm
@@ +93,5 @@
> +        return;
> +      }
> +      this._currentEvent = this._eventQueue.shift();
> +      try {
> +        yield this["_on" + this._currentEvent.type](this._currentEvent.data);

are you not caring to check if _currentEvent is non-null because the try block will catch that anyways?

The try+finally (without a catch in-between) will still end up throwing the exception, and will result in this task becoming a rejected promise. No one is using its result now but it could be easy to overlook in the future.

So better check for non-null and not use exception throwing for the common case of reaching the end of the queue. (I realize the try block is still necessary in case the called function throws)

@@ +231,2 @@
>      };
> +    xhr.send();

is there any chance of this failing, for example by passing a wrong ui to this function?  Might be good to have an xhr.onerror, even if it means resolving the promise with null instead of leaving the _currentEngineObj task hanging.
Attachment #8440245 - Flags: review?(felipc) → review+
Thanks, Felipe.

(In reply to :Felipe Gomes from comment #4)
> There's this Task.async helper now that does the same thing, visually
> simplifying the code a little bit. You might use it if you want, depends if
> you like the syntactic sugar.

Cool, thanks, updated.

> ::: browser/modules/ContentSearch.jsm
> @@ +93,5 @@
> > +        return;
> > +      }
> > +      this._currentEvent = this._eventQueue.shift();
> > +      try {
> > +        yield this["_on" + this._currentEvent.type](this._currentEvent.data);
> 
> are you not caring to check if _currentEvent is non-null because the try
> block will catch that anyways?

Before the this._eventQueue.shift() line, there's an if block that returns early if the queue is empty.  (That's the return at the start of the quoted hunk above.)  Other than that case, shifting the queue should not result in null because the queue doesn't have nulls in it.

> The try+finally (without a catch in-between) will still end up throwing the
> exception, and will result in this task becoming a rejected promise. No one
> is using its result now but it could be easy to overlook in the future.

That's true, but I'm not sure what to do about it now.  Any exceptions at that point are real errors, and I think they should be reported (via the rejected promise) so we can fix them, just like all other unanticipated errors are, at least those involving rejected promises.

> @@ +231,2 @@
> >      };
> > +    xhr.send();
> 
> is there any chance of this failing, for example by passing a wrong ui to
> this function?  Might be good to have an xhr.onerror, even if it means
> resolving the promise with null instead of leaving the _currentEngineObj
> task hanging.

Good point, I changed it to use onloadend instead of onload.  onloadend "gets invoked when the operation is completed for any reason": https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIXMLHttpRequestEventTarget
Attachment #8440245 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c5da3b5f1782
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Depends on: 1027303
You need to log in before you can comment on or make changes to this bug.