Bug 77926 – [Blackberry] Clean up Networkjob and Networkmanger: remove unused variables in release build and change some public functions into be private ones

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris.Guan]

1. remove unused functions and member variable.
2. move some public functions to private.

[Leo Yang]

Fixing up the title.

[Chris.Guan]

Created attachment 125759 [details]
Patch

[WebKit Review Bot]

Attachment 125759 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris.Guan]

Created attachment 125762 [details]
Patch

[Antonio Gomes]

Comment on attachment 125762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125762&action=review

please, lets find a better name than clientIsOk, since we are around :)

> Source/WebCore/ChangeLog:9
> +        2. Remove an ASSERT in NetworkManger, becuase never hit it.

because* it is never hit

[Leo Yang]

Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.

[Chris.Guan]

(In reply to comment #6)
> Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.

No, it dose not make any help, because the code log makes sure this assert is never hit, because the delete timer is firing after handleNotifyClose() in which
definitely m_isRunning was set to false. I added this comments in my change log in the patch:)

[Chris.Guan]

(In reply to comment #5)
> (From update of attachment 125762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125762&action=review
> 
> please, lets find a better name than clientIsOk, since we are around :)
> 
> > Source/WebCore/ChangeLog:9
> > +        2. Remove an ASSERT in NetworkManger, becuase never hit it.
> 
> because* it is never hit

thanks, let me change it:)

[Chris.Guan]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 125762 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=125762&action=review
> > 
> > please, lets find a better name than clientIsOk, since we are around :)
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        2. Remove an ASSERT in NetworkManger, becuase never hit it.
> > 
> > because* it is never hit
> 
> thanks, let me change it:)

what about "isClientAvailable()" for "clientIsOk"?

[Leo Yang]

(In reply to comment #7)
> (In reply to comment #6)
> > Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.
> 
> No, it dose not make any help, because the code log makes sure this assert is never hit, because the delete timer is firing after handleNotifyClose() in which
> definitely m_isRunning was set to false. I added this comments in my change log in the patch:)


ASSERT is designed for never being hit. We ASSERT(!job->isRunning()) to make sure we don't delete a job on which handleNotifyClose() never be called. If you remove this ASSERT, we can't catch the code offending this assumption any more.

[Chris.Guan]

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.
> > 
> > No, it dose not make any help, because the code log makes sure this assert is never hit, because the delete timer is firing after handleNotifyClose() in which
> > definitely m_isRunning was set to false. I added this comments in my change log in the patch:)
> 
> 
> ASSERT is designed for never being hit. We ASSERT(!job->isRunning()) to make sure we don't delete a job on which handleNotifyClose() never be called. If you remove this ASSERT, we can't catch the code offending this assumption any more.

yes, you are right, but deleteJob() is a private function of NetworkManger, and only the friend class of networkjob can use it, actually it is deleteTimer. refactor?

[Leo Yang]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.
> > > 
> > > No, it dose not make any help, because the code log makes sure this assert is never hit, because the delete timer is firing after handleNotifyClose() in which
> > > definitely m_isRunning was set to false. I added this comments in my change log in the patch:)
> > 
> > 
> > ASSERT is designed for never being hit. We ASSERT(!job->isRunning()) to make sure we don't delete a job on which handleNotifyClose() never be called. If you remove this ASSERT, we can't catch the code offending this assumption any more.
> 
> yes, you are right, but deleteJob() is a private function of NetworkManger, and only the friend class of networkjob can use it, actually it is deleteTimer. refactor?
Why refactor? As long as you keep NetworkJob::isRunning() as public and keep NetowrkManager::deleteJob() AS IS, I don't think a refactor is required. My suggestion is, just as I said in the previous comment, guarding NetworkJob::isRuning() and NetworkJob::m_isRunning by #ifndef NDEBUG.

[Chris.Guan]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > Why remove m_isRunning and the ASSERT? What about guard it with #ifndef NDEBUG macro so that m_isRunning and NetworkJob::isRunning() are there only for Debug mode.
> > > > 
> > > > No, it dose not make any help, because the code log makes sure this assert is never hit, because the delete timer is firing after handleNotifyClose() in which
> > > > definitely m_isRunning was set to false. I added this comments in my change log in the patch:)
> > > 
> > > 
> > > ASSERT is designed for never being hit. We ASSERT(!job->isRunning()) to make sure we don't delete a job on which handleNotifyClose() never be called. If you remove this ASSERT, we can't catch the code offending this assumption any more.
> > 
> > yes, you are right, but deleteJob() is a private function of NetworkManger, and only the friend class of networkjob can use it, actually it is deleteTimer. refactor?
> Why refactor? As long as you keep NetworkJob::isRunning() as public and keep NetowrkManager::deleteJob() AS IS, I don't think a refactor is required. My suggestion is, just as I said in the previous comment, guarding NetworkJob::isRuning() and NetworkJob::m_isRunning by #ifndef NDEBUG.
yes, I understand it, let me fix your comments in next patch.

[Chris.Guan]

Created attachment 125777 [details]
Patch

[Rob Buis]

Comment on attachment 125777 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 125777 [details]
Patch

Clearing flags on attachment: 125777

Committed r106931: <http://trac.webkit.org/changeset/106931>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.