656781 - More efficient way to use g_app_info_launch_uris and gnome_vfs_mime_application_launch

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A proposed patch

GList is dynamically allocated in the current implementation but because it is not needed because we have only one URI for those functions.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 532046 [details] [diff] [review]
A proposed patch

The URI is not needed to duplicate in nsGIOMimeApp::Launch too.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 532046 [details] [diff] [review]
A proposed patch

>+  uris.data = PromiseFlatCString(aUri).get();

The result of PromiseFlatCString::get() is only valid as long as the lifetime of the PromiseFlatCString.
I expect you can

  PromiseFlatCString flatUri(aUri);
  uris.data = flatUri.get();

The rest looks good.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

> > The result of PromiseFlatCString::get() is only valid as long as the
> lifetime of the PromiseFlatCString.
> I expect you can
> 
>   PromiseFlatCString flatUri(aUri);
>   uris.data = flatUri.get();

I did not know about the lifetime. Thank you very much!

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 532104 [details] [diff] [review]
Revised patch

>+  uris.data = (gpointer)flatUri.get();

r+ with the (gpointer) cast removed.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #4)
> Comment on attachment 532104 [details] [diff] [review] [review]
> Revised patch
> 
> >+  uris.data = (gpointer)flatUri.get();
> 
> r+ with the (gpointer) cast removed.

Compiler outputs an error without the case on my ubuntu 11.04 machine.

toolkit/system/gnome/nsGIOService.cpp:140:27: error: invalid conversion from ‘const void*’ to ‘void*’

Comment 6

[Karl Tomlinson (:karlt)]

OK, thanks.

How about const_cast<gpointer> or const_cast<char*>?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use const_cast<char*>

const_cast<gpointer> needs static_cast<void*> so I chose the latter.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 532113 [details] [diff] [review]
Use const_cast<char*>

Thanks for sorting that out.
I like const_cast because it is clearer why a cast is necessary.
It's hard to tell from "(gpointer)" what transformation may be taking place.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #8)
> Comment on attachment 532113 [details] [diff] [review] [review]
> Use const_cast<char*>
> 
> Thanks for sorting that out.
> I like const_cast because it is clearer why a cast is necessary.
> It's hard to tell from "(gpointer)" what transformation may be taking place.

Ah, now I see your point.

Comment 10

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/a593b2b745ff