Closed
Bug 860625
Opened 11 years ago
Closed 11 years ago
Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: raymondlee, Assigned: marcos)
References
Details
Attachments
(1 file, 6 obsolete files)
12.71 KB,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•11 years ago
|
Blocks: asyncHistory
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → marcos
Assignee | ||
Comment 1•11 years ago
|
||
Hi guys, Here's a patch for this bug. Let me know if there are any issues. Cheers, Marcos.
Attachment #739290 -
Flags: review?(raymond)
Attachment #739290 -
Flags: review?(mak77)
Reporter | ||
Updated•11 years ago
|
Attachment #739290 -
Flags: review?(raymond)
Reporter | ||
Comment 2•11 years ago
|
||
Comment on attachment 739290 [details] [diff] [review] Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm @@ -497,29 +497,31 @@ let BookmarkNode = { * @param aResolveShortcuts * Converts folder shortcuts into actual folders. * @param aExcludeItems * An array of item ids that should not be written to the backup. */ serializeAsJSONToOutputStream: function BN_serializeAsJSONToOutputStream( aNode, aStream, aIsUICommand, aResolveShortcuts, aExcludeItems) { - // Serialize to stream - let array = []; - if (this._appendConvertedNode(aNode, null, array, aIsUICommand, - aResolveShortcuts, aExcludeItems)) { - let json = JSON.stringify(array[0]); - aStream.write(json, json.length); - } else { - throw Cr.NS_ERROR_UNEXPECTED; - } + Task.spawn(function() { + // Serialize to stream + let array = []; + if (yield this._appendConvertedNode(aNode, null, array, aIsUICommand, + aResolveShortcuts, aExcludeItems)) { + let json = JSON.stringify(array[0]); + aStream.write(json, json.length); + } else { + throw Cr.NS_ERROR_UNEXPECTED; + } + }.bind(this)); You should |return Task.spawn| here, otherwise, the callers won't get the |Promise| object. + }.bind(this)); }, _appendConvertedNode: function BN__appendConvertedNode( - bNode, aIndex, aArray, aIsUICommand, aResolveShortcuts, aExcludeItems) { + bNode, aIndex, aArray, aIsUICommand, aResolveShortcuts, aExcludeItems) { Please restore the indentation. @@ -537,17 +539,17 @@ let BookmarkNode = { // This will throw if we try to serialize an invalid url and it does // not make sense saving a wrong or corrupt uri node. try { NetUtil.newURI(bNode.uri); } catch (ex) { return false; } - this._addURIProperties(bNode, node, aIsUICommand); + Task.spawn(function() { yield this._addURIProperties(bNode, node, aIsUICommand); }.bind(this)); If you do this here, the _appendConvertedNode() wouldn't wait for the above code to complete and return true or false. You should use return Task.spawn to write the code and use throw new Task.Result for the booleans. @@ -556,18 +558,20 @@ let BookmarkNode = { if ((parent && parent.itemId == PlacesUtils.tagsFolderId) || (grandParent && grandParent.itemId == PlacesUtils.tagsFolderId)) return false; this._addSeparatorProperties(bNode, node); } if (!node.feedURI && node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) { - return this._appendConvertedComplexNode(node, bNode, aArray, aIsUICommand, - aResolveShortcuts, aExcludeItems); + return Task.spawn(function() { + throw new Task.Result(yield this._appendConvertedComplexNode(node, bNode, aArray, aIsUICommand, + aResolveShortcuts, aExcludeItems)); + }.bind(this)); You can just use yield here if the _appendConvertedNode() method is wrapped by return Task.spawn @@ -685,19 +689,21 @@ let BookmarkNode = { let wasOpen = aSourceNode.containerOpen; if (!wasOpen) aSourceNode.containerOpen = true; let cc = aSourceNode.childCount; for (let i = 0; i < cc; ++i) { let childNode = aSourceNode.getChild(i); if (aExcludeItems && aExcludeItems.indexOf(childNode.itemId) != -1) continue; - this._appendConvertedNode(aSourceNode.getChild(i), i, children, - aIsUICommand, aResolveShortcuts, - aExcludeItems); + Task.spawn(function() { + yield this._appendConvertedNode(aSourceNode.getChild(i), i, children, + aIsUICommand, aResolveShortcuts, + aExcludeItems); + }.bind(this)); The _appendConvertedComplexNode would return a boolean before the above code complete, please fix that.
Attachment #739290 -
Flags: review?(mak77) → feedback-
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #739290 -
Attachment is obsolete: true
Attachment #741107 -
Flags: feedback?(raymond)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 741107 [details] [diff] [review] Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm > let BookmarkNode = { > /** > * Serializes the given node (and all its descendents) as JSON >@@ -497,81 +497,85 @@ let BookmarkNode = { > * @param aResolveShortcuts > * Converts folder shortcuts into actual folders. > * @param aExcludeItems > * An array of item ids that should not be written to the backup. > */ > serializeAsJSONToOutputStream: function BN_serializeAsJSONToOutputStream( > aNode, aStream, aIsUICommand, aResolveShortcuts, aExcludeItems) { > Please add @return Looks good to me.
Reporter | ||
Updated•11 years ago
|
Attachment #741107 -
Flags: review?(mak77)
Attachment #741107 -
Flags: feedback?(raymond)
Attachment #741107 -
Flags: feedback+
Reporter | ||
Comment 5•11 years ago
|
||
mak: could you review this patch please?
Comment 6•11 years ago
|
||
Comment on attachment 741107 [details] [diff] [review] Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm Review of attachment 741107 [details] [diff] [review]: ----------------------------------------------------------------- It looks fine to me, thanks
Attachment #741107 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 7•11 years ago
|
||
Marcos: please ensure that the patch can apply cleanly to the latest m-c source.
Assignee | ||
Comment 8•11 years ago
|
||
Hi guys, Here's an updated to patch that applies cleanly to the latest sources. Let me know if everything works. Thanks.
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Marcos Aruj from comment #8) > Created attachment 749629 [details] [diff] [review] > Updated patch to apply cleanly to the source > > Hi guys, > Here's an updated to patch that applies cleanly to the latest sources. > > Let me know if everything works. > > Thanks. I can't apply this patch for some reasons. The format doesn't look like the first one? Raymonds-MacBook-Pro:mozilla-central raymondlee$ hg import --no-commit bug-860625.patch applying bug-860625.patch unable to find 'components/places/BookmarkJSONUtils.jsm' for patching 4 out of 4 hunks FAILED -- saving rejects to file components/places/BookmarkJSONUtils.jsm.rej abort: patch failed to apply
Assignee | ||
Comment 10•11 years ago
|
||
Reporter | ||
Comment 11•11 years ago
|
||
(In reply to Marcos Aruj from comment #10) > Created attachment 749636 [details] [diff] [review] > Updated patch to apply cleanly to the source. Regular patch format. I have pushed to try. https://tbpl.mozilla.org/?tree=Try&rev=074fe733eeac Please go to the link to check the results. Once everything looks ok, just add "checkin-needed" in keywords field.
Assignee | ||
Updated•11 years ago
|
Attachment #749629 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #741107 -
Attachment is obsolete: true
Reporter | ||
Comment 12•11 years ago
|
||
ubuntu64 always has orange on it https://tbpl.mozilla.org/?tree=Try&rev=6a3e07b945a9 https://tbpl.mozilla.org/?tree=Try&rev=29dd565fc834
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1f058843cc
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Backed out for Linux64 xpcshell failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1084d464ba https://tbpl.mozilla.org/php/getParsedLog.php?id=23162814&tree=Mozilla-Inbound
Comment 15•11 years ago
|
||
(In reply to Raymond Lee [:raymondlee] from comment #12) > ubuntu64 always has orange on it > https://tbpl.mozilla.org/?tree=Try&rev=6a3e07b945a9 > https://tbpl.mozilla.org/?tree=Try&rev=29dd565fc834 Uh, what?
Assignee | ||
Comment 16•11 years ago
|
||
Thanks Ryan. I'll look into the problem to see what may be causing the problem.
Assignee | ||
Comment 17•11 years ago
|
||
Hi guys, I've tried building mozilla with this patch on my Ubuntu64 bit system and I get no errors. Not sure what may be causing the problems on the TRY server. However, I see this in a comment right above the test that fails (3rd line): //XXX not working on Linux unit boxes. Could be filestats caching issue. //do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod); do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize); Maybe the problem is caused by the same thing, since it is working correctly on other OS. Wyt?
Reporter | ||
Comment 18•11 years ago
|
||
(In reply to Marcos Aruj from comment #17) > Hi guys, > > I've tried building mozilla with this patch on my Ubuntu64 bit system and I > get no errors. Not sure what may be causing the problems on the TRY server. > However, I see this in a comment right above the test that fails (3rd line): > > //XXX not working on Linux unit boxes. Could be filestats caching issue. > //do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod); > do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize); > > Maybe the problem is caused by the same thing, since it is working correctly > on other OS. Wyt? I have commented that line out and submitted to try. Lets see what happens. https://tbpl.mozilla.org/?tree=Try&rev=915e98383d4a
Reporter | ||
Comment 19•11 years ago
|
||
(In reply to Raymond Lee [:raymondlee] from comment #18) > (In reply to Marcos Aruj from comment #17) > > Hi guys, > > > > I've tried building mozilla with this patch on my Ubuntu64 bit system and I > > get no errors. Not sure what may be causing the problems on the TRY server. > > However, I see this in a comment right above the test that fails (3rd line): > > > > //XXX not working on Linux unit boxes. Could be filestats caching issue. > > //do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod); > > do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize); > > > > Maybe the problem is caused by the same thing, since it is working correctly > > on other OS. Wyt? > > I have commented that line out and submitted to try. Lets see what happens. > https://tbpl.mozilla.org/?tree=Try&rev=915e98383d4a The tests passed with that line commented out. Mak: any suggestions what to do?
Flags: needinfo?(mak77)
Comment 20•11 years ago
|
||
just skip those tests on linux, if there was already a known problem with stats caching it is likely causing the issue, file size and flastmod are both cached afaik. You should be able to skip that part using this: let isLinux = "@mozilla.org/gnome-gconf-service;1" in Components.classes; if (!isLinux) ...
Flags: needinfo?(mak77)
Reporter | ||
Comment 21•11 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #20) > just skip those tests on linux, if there was already a known problem with > stats caching it is likely causing the issue, file size and flastmod are > both cached afaik. You should be able to skip that part using this: > > let isLinux = "@mozilla.org/gnome-gconf-service;1" in Components.classes; > if (!isLinux) ... I have submitted a patch with the above check to try. https://tbpl.mozilla.org/?tree=Try&rev=99fea66e6cf7 Marcos: please check the results when it's done
Flags: needinfo?
Reporter | ||
Comment 22•11 years ago
|
||
Please note that the commented out line doesn't work on Mac OSX as well, therefore, I have added another check to the patch. // do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod); Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7610519db5a5
Attachment #749636 -
Attachment is obsolete: true
Flags: needinfo?
Reporter | ||
Comment 23•11 years ago
|
||
fixed some typo
Attachment #753237 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 24•11 years ago
|
||
(In reply to Raymond Lee [:raymondlee] from comment #22) > Created attachment 753237 [details] [diff] [review] > v4 > > Pushed to try > https://tbpl.mozilla.org/?tree=Try&rev=7610519db5a5 Passed try
Reporter | ||
Comment 25•11 years ago
|
||
Just updated the commit description
Attachment #753239 -
Attachment is obsolete: true
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cf513cd008
Flags: in-testsuite+
Keywords: checkin-needed
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7cf513cd008
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•