Bug 24493 – [GTK] Misc patches for WebKitWebHistoryItem

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Xan Lopez]

Ditto.

[Xan Lopez]

Created attachment 28439 [details]
style.patch

Fix coding style.

[Xan Lopez]

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.

[Xan Lopez]

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.

[Alexey Proskuryakov]

Comment on attachment 28439 [details]
style.patch

r=me

[Xan Lopez]

(In reply to comment #4)
> (From update of attachment 28439 [details] [review])
> r=me
> 

Landed in r41558, thanks.

[Alexey Proskuryakov]

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.

[Xan Lopez]

(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...

[Xan Lopez]

(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/ ...

[Xan Lopez]

Created attachment 28466 [details]
fixstyle.patch

return foo? foo : NULL == return foo

[Xan Lopez]

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.

[Holger Freyther]

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?

[Xan Lopez]

(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.

[Xan Lopez]

Uhh, ok, nevermind. We need a per instance flag in the private data :)

[Xan Lopez]

Created attachment 28468 [details]
disposeonce.patch

Something like this.

[Jan Alonzo]

*** Bug 19898 has been marked as a duplicate of this bug. ***

[Jan Alonzo]

Created attachment 28473 [details]
add WebKitWebHistoryItem unit test

adding unit test to this bug

[Holger Freyther]

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

[Holger Freyther]

Comment on attachment 28467 [details]
deref.patch

okay, sounds good..

[Jan Alonzo]

(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

[Holger Freyther]

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...

[Jan Alonzo]

(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.