Closed Bug 646396 Opened 9 years ago Closed 9 years ago

Update QtNetworkManager to new API useage

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jbos, Assigned: jbos)

References

Details

Attachments

(1 file, 5 obsolete files)

Current Impl. is just broken. Update to new API useage to give network manager any kind of functionality back.
Attachment #522956 - Flags: review?(romaxa)
Blocks: 646412
Blocks: 646417
Blocks: 609844
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?
mhm well its a singleton. Which place would you call save?
I guess NetworkLinkService should be safe enough.
Comment on attachment 522956 [details] [diff] [review]
Fix Network Manager to use updated Qt API

could you attach patch with globalVar instead of method?
Attachment #522956 - Flags: review?(romaxa)
Attached patch make it global (obsolete) — Splinter Review
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.
Attachment #522956 - Attachment is obsolete: true
Attachment #524183 - Flags: review?(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...
Attached patch Remove singleton (obsolete) — Splinter Review
After talk with romaxa i removed all singleton.
Attachment #524183 - Attachment is obsolete: true
Attachment #524183 - Flags: review?(romaxa)
Attachment #524363 - Flags: review?(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
Attachment #524363 - Flags: review?(romaxa) → review+
Attached patch Fixed nits (obsolete) — Splinter Review
Took review + from prev version.
Attachment #524441 - Flags: review+
Comment on attachment 524441 [details] [diff] [review]
Fixed nits

still comments not fixed.
Attachment #524441 - Flags: review+
Attached patch Fixed more nits (obsolete) — Splinter Review
Fixed more nits.

I overlooked the - and the "and the other comments to".
Attachment #524441 - Attachment is obsolete: true
Attachment #524450 - Flags: review?(romaxa)
Attachment #524450 - Flags: review?(romaxa) → review+
Attachment #524363 - Attachment is obsolete: true
This patch doesn't apply to trunk any more.
Keywords: checkin-needed
Comment on attachment 524450 [details] [diff] [review]
Fixed more nits

patch again mailformed...
Attached patch Applies now...Splinter Review
I have no idea what is going on with that patch... Here its once more the same one. It does apply
Attachment #524450 - Attachment is obsolete: true
Attachment #524600 - Flags: review+
Assignee: nobody → jeremias.bosch
http://hg.mozilla.org/mozilla-central/rev/4f957dd3f9f7
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
(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.
You need to log in before you can comment on or make changes to this bug.