Use mimetype hint for JSON in sync

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
23 days ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---

Firefox Tracking Flags

(fennec2.0b5+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Bug 632177 shows that using the system's mimetypes can be very expensive for Fennec. This gives mimetype hints for JSON in sync.
(Assignee)

Comment 1

8 years ago
Created attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync
(Assignee)

Comment 2

8 years ago
I haven't set myself up to make sure this fixes the problem on Android. Matt or Taras, mind testing?
(Assignee)

Updated

8 years ago
Attachment #510634 - Flags: review?(edilee)

Comment 3

8 years ago
Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

Seems reasonable. Passing review to sync team.

Will need to get a reviewer for netutil bits. Maybe sdwilsh or bz? (I'm assuming setting the content type will prevent future content type lookups?)
Attachment #510634 - Flags: review?(edilee) → review?(philipp)

Comment 4

8 years ago
Any idea how content type lookup works? The OS will separately synchronously read the file to infer a type?
Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

>+     * @param aHint
>+     *        The mimetype of the resource we are opening (optional)

Drive-by review: I would prefer a more specific identifier like "aContentType" or "aTypeHint."
I have verified that this eliminates the calls from Weave to GetMIMEInfoFromOS in Fennec.
Comment on attachment 510634 [details] [diff] [review]
Use mimetype hint for JSON in sync

Looks good to me, but please create a separate patch for the Sync bit since it needs to land in https://hg.mozilla.org/services/fx-sync/ (the canonical repository, merged periodically). Clearing the review flag for now.

Btw, if there's a way to test the NetUtil bit (I imagine there is), there should be a test exercising the new argument.
Attachment #510634 - Flags: review?(philipp)

Comment 9

8 years ago
For the test, I was thinking it might be tricky to test that GetMIMEInfoFromOS doesn't get called. But I think a satisfactory test would be to provide an override. E.g., text/plain for a application/json file. (But I'm not a network peer! :p)

Updated

8 years ago
Blocks: 632177
I'm not sure we actually want to make this change to NetUtil (and this isn't in a component that people who actually care about this stuff would see it).  It'd be pretty trivial for the Sync code to just say:
let channel = NetUtil.newChannel(whateverItIsPassingInNow);
channel.setContentType("application/json");
NetUtil.asyncFetch(channel, ...

Comment 11

8 years ago
Oh hey indeed. It's passing in a nsIFile but NetUtil does expose newChannel, so what sdwilsh suggested should work without any NetUtils changes.

asyncFetch calling newChannel:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#177
(In reply to comment #10)
> let channel = NetUtil.newChannel(whateverItIsPassingInNow);
> channel.setContentType("application/json");
> NetUtil.asyncFetch(channel, ...

Good point. Moving this bug to Services :: Sync Backend.
Component: Web Services → Firefox Sync: Backend
Product: Core → Mozilla Services
QA Contact: web-services → sync-backend
Version: Trunk → unspecified
(Assignee)

Updated

8 years ago
Attachment #510634 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Created attachment 510707 [details] [diff] [review]
Use mimetype hint for JSON in sync
(Assignee)

Updated

8 years ago
Attachment #510707 - Flags: review?(philipp)
Comment on attachment 510707 [details] [diff] [review]
Use mimetype hint for JSON in sync

Please land this in https://hg.mozilla.org/services/fx-sync/
Attachment #510707 - Flags: review?(philipp) → review+
(Assignee)

Updated

8 years ago
Attachment #510707 - Attachment is obsolete: true
(Assignee)

Comment 15

8 years ago
Created attachment 510761 [details] [diff] [review]
Use mimetype hint for JSON in sync
(Assignee)

Updated

8 years ago
Attachment #510761 - Flags: review+
This should probably block Fennec if it provides the suggested performance improvement.
tracking-fennec: --- → ?
Yes, it does. We have seen a Ts improvement from similar patches that already landed.
tracking-fennec: ? → 2.0b5+
Landed in places: http://hg.mozilla.org/projects/places/rev/8214fee0a355
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in places]
(Assignee)

Comment 20

8 years ago
Hm, there were some issues in try I wanted to look at, but it seems that it has landed fine in places. Shall we commit this to m-c as well?
(In reply to comment #20)
> Hm, there were some issues in try I wanted to look at,

http://tbpl.mozilla.org/?tree=MozillaTry&rev=878c925f96ea looks good to me.

> Shall we commit this to m-c as well?

I will merge places to m-c later today once the tree reopens.
Assignee: nobody → ben
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/8214fee0a355
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in places]

Updated

8 years ago
Whiteboard: [qa-]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.