646396 - Update QtNetworkManager to new API useage

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

Attachment: Fix Network Manager to use updated Qt API

Current Impl. is just broken. Update to new API useage to give network manager any kind of functionality back.

Comment 1

[Jason Duell]

Comment on attachment 522956 [details] [diff] [review]
Fix Network Manager to use updated Qt API

Drive-by comment:

>-  if (nsQtNetworkManager::OpenConnectionSync())
>+  if (nsQtNetworkManager::instance()->openConnection(QString::fromUtf16(hostName))) {

So you've got changes like this all over the code:  can we just initialize gQtNetworkManagerover in some known safe place (i.e an ::Init function) and use it directly, instead of this ugly syntax?

Comment 2

[Jeremias Bosch (:jbos)]

mhm well its a singleton. Which place would you call save?

Comment 3

[Oleg Romashin (:romaxa)]

I guess NetworkLinkService should be safe enough.

Comment 4

[Oleg Romashin (:romaxa)]

Comment on attachment 522956 [details] [diff] [review]
Fix Network Manager to use updated Qt API

could you attach patch with globalVar instead of method?

Comment 5

[Jeremias Bosch (:jbos)]

Attachment: make it global

I disagree that globalization is a good thing here... but well, i have no strong feeling for one or the other. So find patch attached.

Comment 6

[Oleg Romashin (:romaxa)]

Idea of comment #1 was not to just replace ::instance() call with globalPointer and add null checks everywhere instead, it was about initializing networkManager in safe place NetworkLinkService::Init  (comment3) and make sure that pointer available everywhere where it is needed...

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: Remove singleton

After talk with romaxa i removed all singleton.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 524363 [details] [diff] [review]
Remove singleton


>  * ***** END LICENSE BLOCK ***** */
>-

don't remove this line
>+#include "nsQtNetworkManager.h"
> #include "nsQtNetworkLinkService.h"
> #include "nsCOMPtr.h"
> #include "nsIObserverService.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
>-#include "nsQtNetworkManager.h"
> #include "mozilla/Services.h"

>+  //create singleton and make it global
      ^ add space here and in other comments

>+    if (networkSession != 0) {
if (networkSession) {
I think this should also work

the rest seem looks good...
Also fix problem with mailformed patch apply.

r+ with comments fixed

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Fixed nits

Took review + from prev version.

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 524441 [details] [diff] [review]
Fixed nits

still comments not fixed.

Comment 11

[Jeremias Bosch (:jbos)]

Attachment: Fixed more nits

Fixed more nits.

I overlooked the - and the "and the other comments to".

Comment 12

[(no longer active)]

This patch doesn't apply to trunk any more.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 524450 [details] [diff] [review]
Fixed more nits

patch again mailformed...

Comment 14

[Jeremias Bosch (:jbos)]

Attachment: Applies now...

I have no idea what is going on with that patch... Here its once more the same one. It does apply

Comment 15

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/4f957dd3f9f7

Comment 16

[Benjamin Stover (:stechz)]

Did this get backeed out and then re-pushed to cedar? My merge from cedar->m-c committed this:
http://hg.mozilla.org/mozilla-central/rev/5e7eef0ccdc6

Comment 17

[(no longer active)]

(In reply to comment #16)
> Did this get backeed out and then re-pushed to cedar? My merge from cedar->m-c
> committed this:
> http://hg.mozilla.org/mozilla-central/rev/5e7eef0ccdc6

No, Romaxa also landed it on mozilla-central before cedar was merged in.  Good thing is that hg merge is smart enough to handle that.