Closed Bug 834543 Opened 11 years ago Closed 11 years ago

Add asynchronous version of setCharsetForURI and getCharsetForURI

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mak, Assigned: raymondlee)

References

Details

Attachments

(4 files, 11 obsolete files)

4.84 KB, patch
mak
: review+
Details | Diff | Splinter Review
28.57 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
5.59 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
these APIs are synchronous and used in browser may not be an easy task.
Attached patch Part I (Set charset) (obsolete) — Splinter Review
I need some guidance here, since the code is compiling without errors, but it's crashing when using:
 
> PlacesUtils.asyncHistory.setCharsetForURI(URI, charset);

Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (nsStandardURL not thread-safe), at /netwerk/base/src/nsStandardURL.cpp:921
Attachment #716250 - Flags: feedback?(mak77)
Comment on attachment 716250 [details] [diff] [review]
Part I (Set charset)

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

Since these are basically just UI hints used by browser and some backup toolkit code, it's very unlikely we need to access them from cpp, thus I think it would be much simpler (for everyone) to just expose a couple helpers in PlacesUtils, that for now will internally use annotations (but expose the value through a callback (and maybe a promise so that the consumer can choose?).
If in future we should find the need to do that in cpp, probably we should do that directly when adding the visit... But for now I couldn't find any consumer in need of that.

So I'd actually remove the APIs from history and provide them as async PlacesUtils helpers.

Btw, just for information your bug here was probably the fact you create a nsIURI on main-thread, then assign it a member variable of a runnable on another thread, and I think it doesn't support thread-safe refcounting, thus you should have rather stored the spec, or used .swap to avoid changing refcounting
Attachment #716250 - Flags: feedback?(mak77) → feedback-
The first part implements the PlacesUtils async helpers and updates the unit tests.
Attachment #716250 - Attachment is obsolete: true
Attachment #718486 - Flags: review?(mak77)
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attachment #718588 - Flags: review?(mak77)
Comment on attachment 718486 [details] [diff] [review]
Part I - Async helpers and unit tests

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +2136,5 @@
> +   * Sets the character-set for a URI.
> +   * If aCharset is empty remove character-set annotation for aURI.
> +   *
> +   * @param aURI nsIURI
> +   * @param aCharset character-set value.

please document the return value

@@ +2148,5 @@
> +        Ci.nsIAnnotationService.EXPIRE_NEVER);
> +    } else {
> +      PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO);
> +    }
> +    deferred.resolve();

to be sure to catch bogus behaviors with an async setter all of this should be enqueued on main-thread, otherwise it would still be synchronous and would be hard in future to make it properly async.

@@ +2158,5 @@
> +   * Gets the last saved character-set for a URI.
> +   *
> +   * @param aURI nsIURI
> +    * @param aCallback the callback method.
> +   * @return the character-set value if found.

this is wrong

@@ +2175,5 @@
> +      if (aCallback) {
> +        aCallback();
> +      }
> +      deferred.resolve();
> +    }

ditto, all of this should be enqueued on main-thread so we can start fixing consumers to an async behavior

::: toolkit/components/places/tests/unit/test_317472.js
@@ +22,5 @@
>    yield promiseAddVisits(TEST_BOOKMARKED_URI);
>  
>    // create bookmarks on TEST_BOOKMARKED_URI
> +  var bm1 = PlacesUtils.bookmarks.insertBookmark(
> +              PlacesUtils.bookmarks.unfiledBookmarksFolder,

for roots always use the PlacesUtils getters (PlacesUtils.unfiledBookmarksFolderId in this case) not the .bookmarks. getters

::: toolkit/components/places/tests/unit/test_384370.js
@@ +188,5 @@
>                PlacesUtils.annotations.getItemAnnotation(testBookmark1.itemId, POST_DATA_ANNO));
>  
>    // last charset
>    var testURI = PlacesUtils._uri(testBookmark1.uri);
> +  PlacesUtils.getCharsetForURI(testURI, function(aCharset) {

this is wrong, testCanonicalBookmarks should wait for this to be complete since now it's async

::: toolkit/components/places/tests/unit/test_bookmarks_html.js
@@ +362,5 @@
>        case "charset":
> +        let testURI = NetUtil.newURI(aNode.uri);
> +        Task.spawn(function() {
> +          do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), aExpected.charset);
> +        });

also this is wrong, checkItem doesn't seem to wait for these checks... indeed I think also the getLivemark in the feedUrl case is totally wrong and should be fixed

::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js
@@ +130,5 @@
>    // last charset
>    var testURI = uri(testBookmark1.uri);
> +  Task.spawn(function() {
> +    do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), "ISO-8859-1");
> +  });

nothing is waiting for the task
Attachment #718486 - Flags: review?(mak77)
Comment on attachment 718590 [details] [diff] [review]
Part IV - Remove cpp methods

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

this will require SR (and clearly all of the other patches should have been pushed already).
I found about 6 add-ons using these helpers, we can probably contact them directly once we push this, or we may file a bug apart to do the removal and here just mark the deprecation.
Attachment #718590 - Flags: review?(mak77) → review+
Comment on attachment 718588 [details] [diff] [review]
Part II - Update use of setCharsetForURI

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

this looks ok, there is some involved risk with moving to async since adding a page, calling the setter, removing a page would likely throw, but it's likely the setter will just fail there.
Attachment #718588 - Flags: review?(mak77) → review+
Comment on attachment 718589 [details] [diff] [review]
Part III - Update use of getCharsetForURI

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

Unfortunately Task doesn't have any magic that ensures the code stops there, that means most of this patch is wrong (the only right piece is BookmarkHTMLUtils.jsm change).
we can't just replace a synchronous getter with an asynchronous one like that, we need separate patches for each consumer and figure out how to change each consumer to accept an async getter
Attachment #718589 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #7)
> Comment on attachment 718486 [details] [diff] [review]
> Part I - Async helpers and unit tests
> 
> 
> ::: toolkit/components/places/tests/unit/test_bookmarks_html.js
> @@ +362,5 @@
> >        case "charset":
> > +        let testURI = NetUtil.newURI(aNode.uri);
> > +        Task.spawn(function() {
> > +          do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), aExpected.charset);
> > +        });
> 
> also this is wrong, checkItem doesn't seem to wait for these checks...
> indeed I think also the getLivemark in the feedUrl case is totally wrong and
> should be fixed
> 

I am not sure how to tackle this because checkItem() is called inside loop which makes it hard to wait for these checks and then carry on.... Any suggestions?
Flags: needinfo?
See comment 11 please
Attachment #718486 - Attachment is obsolete: true
Flags: needinfo?
(In reply to Raymond Lee [:raymondlee] from comment #11)
> 
> I am not sure how to tackle this because checkItem() is called inside loop
> which makes it hard to wait for these checks and then carry on.... Any
> suggestions?

It will have to return a promise, and the same should then happen in its callers up to the add_task... I don't think there is an easier solution
Attachment #719390 - Attachment is obsolete: true
Attachment #719801 - Flags: review?(mak77)
Depends on: 846635
Depends on: 846636
Depends on: 846638
Depends on: 846644
Simplified the patch
Attachment #719801 - Attachment is obsolete: true
Attachment #719801 - Flags: review?(mak77)
Attachment #719843 - Flags: review?(mak77)
There are some complicated cases so created some dependency bugs.
Attachment #718589 - Attachment is obsolete: true
Attachment #719843 - Attachment is patch: true
Attachment #719845 - Attachment is patch: true
Attachment #719845 - Flags: review?(mak77)
Assignee: andres → raymond
Blocks: 846635, 846636, 846644
No longer depends on: 846635, 846636, 846644
Summary: make setCharsetForURI and getCharsetForURI asynchronous → Add asynchronous version of setCharsetForURI and getCharsetForURI
I'm a bit worried by how complicate the getShortcutOrURI patches are, and the fact they convert something that depends on "synchronous" user input to an asynchronous behavior.  Those reviews may take a bit more time to ensure safety.
So likely we should move the "remove cpp" part to a separate bug that depends on them being fixed.
Comment on attachment 719843 [details] [diff] [review]
Part I - Async helpers and unit tests v4

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +49,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "Promise", function() {
> +  Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> +  return Promise;
> +});

why not defineLazyModuleGetter?
If you just took inspiration from the other getters, those are not using defineLazyModuleGetter cause likely at that time it didn't exist (Well, I actually verified and it existed the week before I made that patch, so it's possible I was just not aware of that, the week after).  I'm happy if you also fix those :)

@@ +2133,5 @@
> +  },
> +
> +  /**
> +   * Sets the character-set for a URI.
> +   * If aCharset is empty remove character-set annotation for aURI.

s/annotation//
that's an implementation detail that should not matter here

@@ +2149,5 @@
> +        Ci.nsIAnnotationService.EXPIRE_NEVER);
> +    } else {
> +      PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO);
> +    }
> +    Services.tm.mainThread.dispatch(function() {

you should delay the whole thing  (apart let deferred and return deferred.promise clearly), if we set it synchronously and just resolve asynchronously a test may still pass expecting it to be set as soon as the call completes.
please also add a comment stating we are delaying this to catch issues with asynchronous behavior while waiting to implement asynchronous annotations in bug 699844.

@@ +2160,5 @@
> +  /**
> +   * Gets the last saved character-set for a URI.
> +   *
> +   * @param aURI nsIURI
> +   * @param aCallback the callback method.

should state it gets the character-set as argument, or null otherwise

@@ +2162,5 @@
> +   *
> +   * @param aURI nsIURI
> +   * @param aCallback the callback method.
> +   * @return {Promise}
> +   * @resolve a character-set

should also state which value the character-set has if it doesn't exist, I suppose null may be fine.

@@ +2179,5 @@
> +        deferred.resolve(charset);
> +      } catch (ex) {
> +        if (aCallback) {
> +          aCallback();
> +        }

just let charset = null; outside of the try/catch, and invoke aCallback(charset) and resolve(charset) after it?

@@ +2182,5 @@
> +          aCallback();
> +        }
> +        deferred.resolve();
> +      }
> +    }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);

may avoid bind by useing full PlacesUtils, also cause you do PlacesUtils.annotations and this.CHARSET_ANNO, so it's not coherent atm. (ditto for setCharsetForURI)

::: toolkit/components/places/tests/unit/test_384370.js
@@ +55,5 @@
>      populate();
>  
>      // 2. run the test-suite
> +    Task.spawn(validate).then(function() {
> +      promiseAsyncUpdates().then(function testJsonExport() {

if we could just yield validate(); and yield promiseAsyncUpdates(); here and below, the code could be a bit more readable

::: toolkit/components/places/tests/unit/test_bookmarks_html.js
@@ +272,5 @@
>      root.containerOpen = false;
>    }
>  }
>  
> +/*

this looks wrong debug commenting

::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js
@@ +186,5 @@
>        // and comparing it, we just check the data size.
>        do_check_eq(TEST_FAVICON_DATA_SIZE, aDataLen);
>        deferred.resolve();
>      });
> +  yield deferred.promise;

instead of this I think you may change the test to do
yield Task.spawn(database_check) instead of yield database_check(), and here you may not need anymore the deferred
Attachment #719843 - Flags: review?(mak77)
Attachment #719845 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #19)
> Comment on attachment 719843 [details] [diff] [review]
> Part I - Async helpers and unit tests v4
> 
> Review of attachment 719843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +49,5 @@
> > +
> > +XPCOMUtils.defineLazyGetter(this, "Promise", function() {
> > +  Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> > +  return Promise;
> > +});
> 
> why not defineLazyModuleGetter?
> If you just took inspiration from the other getters, those are not using
> defineLazyModuleGetter cause likely at that time it didn't exist (Well, I
> actually verified and it existed the week before I made that patch, so it's
> possible I was just not aware of that, the week after).  I'm happy if you
> also fix those :)

Updated

> 
> @@ +2133,5 @@
> > +  },
> > +
> > +  /**
> > +   * Sets the character-set for a URI.
> > +   * If aCharset is empty remove character-set annotation for aURI.
> 
> s/annotation//
> that's an implementation detail that should not matter here
> 

Removed that comment.

> @@ +2149,5 @@
> > +        Ci.nsIAnnotationService.EXPIRE_NEVER);
> > +    } else {
> > +      PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO);
> > +    }
> > +    Services.tm.mainThread.dispatch(function() {
> 
> you should delay the whole thing  (apart let deferred and return
> deferred.promise clearly), if we set it synchronously and just resolve
> asynchronously a test may still pass expecting it to be set as soon as the
> call completes.
> please also add a comment stating we are delaying this to catch issues with
> asynchronous behavior while waiting to implement asynchronous annotations in
> bug 699844.

delayed the whole thing and added comment

> 
> @@ +2160,5 @@
> > +  /**
> > +   * Gets the last saved character-set for a URI.
> > +   *
> > +   * @param aURI nsIURI
> > +   * @param aCallback the callback method.
> 
> should state it gets the character-set as argument, or null otherwise

Updated

> 
> @@ +2162,5 @@
> > +   *
> > +   * @param aURI nsIURI
> > +   * @param aCallback the callback method.
> > +   * @return {Promise}
> > +   * @resolve a character-set
> 
> should also state which value the character-set has if it doesn't exist, I
> suppose null may be fine.
>

Updated
 
> @@ +2179,5 @@
> > +        deferred.resolve(charset);
> > +      } catch (ex) {
> > +        if (aCallback) {
> > +          aCallback();
> > +        }
> 
> just let charset = null; outside of the try/catch, and invoke
> aCallback(charset) and resolve(charset) after it?

Done.

> 
> @@ +2182,5 @@
> > +          aCallback();
> > +        }
> > +        deferred.resolve();
> > +      }
> > +    }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> 
> may avoid bind by useing full PlacesUtils, also cause you do
> PlacesUtils.annotations and this.CHARSET_ANNO, so it's not coherent atm.
> (ditto for setCharsetForURI)

Use PlacesUtils.CHARSET_ANNO instead

> 
> ::: toolkit/components/places/tests/unit/test_384370.js
> @@ +55,5 @@
> >      populate();
> >  
> >      // 2. run the test-suite
> > +    Task.spawn(validate).then(function() {
> > +      promiseAsyncUpdates().then(function testJsonExport() {
> 
> if we could just yield validate(); and yield promiseAsyncUpdates(); here and
> below, the code could be a bit more readable

Done.

> 
> ::: toolkit/components/places/tests/unit/test_bookmarks_html.js
> @@ +272,5 @@
> >      root.containerOpen = false;
> >    }
> >  }
> >  
> > +/*
> 
> this looks wrong debug commenting
> 

The testImportedBookmarksToFolder() is actually being used.  Shall we remove that method or leave it as it was?


> ::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js
> @@ +186,5 @@
> >        // and comparing it, we just check the data size.
> >        do_check_eq(TEST_FAVICON_DATA_SIZE, aDataLen);
> >        deferred.resolve();
> >      });
> > +  yield deferred.promise;
> 
> instead of this I think you may change the test to do
> yield Task.spawn(database_check) instead of yield database_check(), and here
> you may not need anymore the deferred

Removed.
Attachment #719843 - Attachment is obsolete: true
Attachment #722086 - Flags: review?(mak77)
Comment on attachment 722086 [details] [diff] [review]
Part I - Async helpers and unit tests v5

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

it looks fine now

::: toolkit/components/places/PlacesUtils.jsm
@@ +45,5 @@
> +                                  "resource://gre/modules/Task.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this,
> +                                  "Promise",
> +                                  "resource://gre/modules/commonjs/sdk/core/promise.js");

nit: please keep the 2nd argumento on the first line, it's a bit more compact and the most common style around, afaik

@@ +2095,5 @@
> +  setCharsetForURI: function PU_setCharsetForURI(aURI, aCharset) {
> +    let deferred = Promise.defer();
> +
> +    // delaying to catch issues with asynchronous behavior while waiting
> +    // to implement asynchronous annotations in bug 699844

nit: ucfirst and end with period

@@ +2115,5 @@
> +  /**
> +   * Gets the last saved character-set for a URI.
> +   *
> +   * @param aURI nsIURI
> +   * @param aCallback the callback method that retuns a character-set or null.

please add [optional]

@@ +2128,5 @@
> +
> +      try {
> +        charset =
> +          PlacesUtils.annotations.getPageAnnotation(
> +            aURI, PlacesUtils.CHARSET_ANNO);

too much indentation
Attachment #722086 - Flags: review?(mak77) → review+
Updated based on comments in comment 20
Attachment #722086 - Attachment is obsolete: true
mak: do you think we should land the part I, II and III patches first and then fix other dependency bugs?
This patch couldn't apply clearly so updated it.
Attachment #718588 - Attachment is obsolete: true
Part I, II and III passed try
https://tbpl.mozilla.org/?tree=Try&rev=e6b11d76a7bb
Attachment #725322 - Attachment is obsolete: true
Attachment #727749 - Flags: checkin?
Attachment #726737 - Attachment is obsolete: true
Attachment #727750 - Flags: checkin?
Attachment #719845 - Attachment is obsolete: true
Attachment #727754 - Flags: checkin?
Whiteboard: [leave open]
Attachment #727749 - Flags: checkin? → checkin+
Attachment #727750 - Flags: checkin? → checkin+
Attachment #727754 - Flags: checkin? → checkin+
No longer blocks: 846644
please close this bug and move part IV elsewhere
Also, please file a new bug to remove the callback from getCharsetForURI, in the end we don't need both the callback and the promise, since with the promise you may do getCharsetForURI(...).then(callback)
(In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #31)
> please close this bug and move part IV elsewhere
> Also, please file a new bug to remove the callback from getCharsetForURI, in
> the end we don't need both the callback and the promise, since with the
> promise you may do getCharsetForURI(...).then(callback)

Filed bug 854925 and bug 854927
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Blocks: 855190
You need to log in before you can comment on or make changes to this bug.