Closed
Bug 632417
Opened 14 years ago
Closed 14 years ago
Use mimetype hint for JSON in sync
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b5+ | --- |
People
(Reporter: stechz, Assigned: stechz)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
1.02 KB,
patch
|
stechz
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
I haven't set myself up to make sure this fixes the problem on Android. Matt or Taras, mind testing?
Assignee | ||
Updated•14 years ago
|
Attachment #510634 -
Flags: review?(edilee)
Comment 3•14 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•14 years ago
|
||
Any idea how content type lookup works? The OS will separately synchronously read the file to infer a type?
Assignee | ||
Comment 5•14 years ago
|
||
Comment 6•14 years ago
|
||
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."
Comment 7•14 years ago
|
||
I have verified that this eliminates the calls from Weave to GetMIMEInfoFromOS in Fennec.
Comment 8•14 years ago
|
||
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•14 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)
Comment 10•14 years ago
|
||
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•14 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
Comment 12•14 years ago
|
||
(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•14 years ago
|
Attachment #510634 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #510707 -
Flags: review?(philipp)
Comment 14•14 years ago
|
||
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•14 years ago
|
Attachment #510707 -
Attachment is obsolete: true
Assignee | ||
Comment 15•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #510761 -
Flags: review+
Comment 16•14 years ago
|
||
This should probably block Fennec if it provides the suggested performance improvement.
tracking-fennec: --- → ?
Comment 17•14 years ago
|
||
Yes, it does. We have seen a Ts improvement from similar patches that already landed.
tracking-fennec: ? → 2.0b5+
Comment 18•14 years ago
|
||
Landed in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/20b5aa56b69f
Whiteboard: [fixed in fx-sync]
Comment 19•14 years ago
|
||
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•14 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?
Comment 21•14 years ago
|
||
(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.
Updated•14 years ago
|
Assignee: nobody → ben
Comment 22•14 years ago
|
||
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/8214fee0a355
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in places]
Updated•14 years ago
|
Whiteboard: [qa-]
Updated•7 years ago
|
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.
Description
•