Ditto.
Created attachment 28439 [details] style.patch Fix coding style.
Created attachment 28440 [details] hashtablefull.patch Use g_hash_table_new_full so we can save the manual unref on the values when removing them from the table.
Created attachment 28441 [details] dispose.patch dispose can run more than once, so make sure we don't try to access the items hash table after it's potentially destroyed.
Comment on attachment 28439 [details] style.patch r=me
(In reply to comment #4) > (From update of attachment 28439 [details] [review]) > r=me > Landed in r41558, thanks.
Comment on attachment 28440 [details] hashtablefull.patch r=me. It's a bit counter-intuitive that the table derefs the elements, but does not ref them automatically. Perhaps a comment would help.
(In reply to comment #6) > (From update of attachment 28440 [details] [review]) > r=me. It's a bit counter-intuitive that the table derefs the elements, but does > not ref them automatically. Perhaps a comment would help. > Mmm, well, the table can't magically know what function to use to do the opposite operation of the destroy function you pass, and besides you might now want to do it in all cases...
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 28440 [details] [review] [review]) > > r=me. It's a bit counter-intuitive that the table derefs the elements, but does > > not ref them automatically. Perhaps a comment would help. > > > > Mmm, well, the table can't magically know what function to use to do the > opposite operation of the destroy function you pass, and besides you might now > want to do it in all cases... > s/now/not/ ...
Created attachment 28466 [details] fixstyle.patch return foo? foo : NULL == return foo
Created attachment 28467 [details] deref.patch Call deref() on our internal HistoryItem on dispose, as we always acquire it with a releaseRef() call to a PassRefPtr, which passes ownership.
Comment on attachment 28441 [details] dispose.patch > static void webkit_web_history_item_dispose(GObject* object) > { > WebKitWebHistoryItem* webHistoryItem = WEBKIT_WEB_HISTORY_ITEM(object); > - > - GHashTable* table = webkit_history_items(); > - > - g_hash_table_remove(table, core(webHistoryItem)); > - > - /* destroy table if empty */ > - if (!g_hash_table_size(table)) > - g_hash_table_destroy(table); > + static gboolean isDisposed = false; this looks wrong. For every HistoryItem we will only dispose/clear the first time one instance of HistoryItem should get destroyed? What am I missing?
(In reply to comment #11) > (From update of attachment 28441 [details] [review]) > > > static void webkit_web_history_item_dispose(GObject* object) > > { > > WebKitWebHistoryItem* webHistoryItem = WEBKIT_WEB_HISTORY_ITEM(object); > > - > > - GHashTable* table = webkit_history_items(); > > - > > - g_hash_table_remove(table, core(webHistoryItem)); > > - > > - /* destroy table if empty */ > > - if (!g_hash_table_size(table)) > > - g_hash_table_destroy(table); > > + static gboolean isDisposed = false; > > this looks wrong. For every HistoryItem we will only dispose/clear the first > time one instance of HistoryItem should get destroyed? What am I missing? > Mmm, the point is to only remove ourselves from the global hash table, and destroy it if it's empty, once, as dispose can run several times for the same GObject.
Uhh, ok, nevermind. We need a per instance flag in the private data :)
Created attachment 28468 [details] disposeonce.patch Something like this.
*** Bug 19898 has been marked as a duplicate of this bug. ***
Created attachment 28473 [details] add WebKitWebHistoryItem unit test adding unit test to this bug
Comment on attachment 28473 [details] add WebKitWebHistoryItem unit test Looks good. the only complain/question is to multiple programs vs. one program with multiple test suites
Comment on attachment 28467 [details] deref.patch okay, sounds good..
(In reply to comment #17) > (From update of attachment 28473 [details] [review]) > Looks good. the only complain/question is to multiple programs vs. one program > with multiple test suites > zecke, I think we should just split it. There's a bug about that at https://bugs.webkit.org/show_bug.cgi?id=24039 Thanks
Comment on attachment 28473 [details] add WebKitWebHistoryItem unit test > # Build unit test > -noinst_PROGRAMS += Programs/UnitTests > +noinst_PROGRAMS += Programs/UnitTests Programs/WebHistoryItemUnitTests okay, this needs rediffing as of bug #24039. > +static void > +test_webkit_web_history_item_alternate_title(WebHistoryItemFixture* fixture, > + gconstpointer data) inconsistency with the new lines...
(In reply to comment #17) > (From update of attachment 28473 [details] [review]) > Looks good. the only complain/question is to multiple programs vs. one program > with multiple test suites > Landed in r41866. Closing as all other patches have been committed.