Bug 77926 - [Blackberry] Clean up Networkjob and Networkmanger: remove unused variables in release build and change some public functions into be private ones
Summary: [Blackberry] Clean up Networkjob and Networkmanger: remove unused variables i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-02-06 19:22 PST by Chris.Guan
Modified: 2012-02-07 05:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.40 KB, patch)
2012-02-06 20:32 PST, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2012-02-06 21:35 PST, Chris.Guan
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2012-02-07 00:17 PST, Chris.Guan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris.Guan 2012-02-06 19:22:12 PST
1. remove unused functions and member variable.
2. move some public functions to private.
Comment 1 Leo Yang 2012-02-06 20:00:03 PST
Fixing up the title.
Comment 2 Chris.Guan 2012-02-06 20:32:55 PST
Created attachment 125759 [details]
Patch
Comment 3 WebKit Review Bot 2012-02-06 20:35:18 PST
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.
Comment 4 Chris.Guan 2012-02-06 21:35:21 PST
Created attachment 125762 [details]
Patch
Comment 5 Antonio Gomes 2012-02-06 21:37:50 PST
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
Comment 6 Leo Yang 2012-02-06 21:38:37 PST
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.
Comment 7 Chris.Guan 2012-02-06 21:55:00 PST
(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:)
Comment 8 Chris.Guan 2012-02-06 21:56:30 PST
(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:)
Comment 9 Chris.Guan 2012-02-06 22:08:58 PST
(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"?
Comment 10 Leo Yang 2012-02-06 22:10:42 PST
(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.
Comment 11 Chris.Guan 2012-02-06 22:18:15 PST
(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?
Comment 12 Leo Yang 2012-02-06 22:27:41 PST
(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.
Comment 13 Chris.Guan 2012-02-06 23:09:26 PST
(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.
Comment 14 Chris.Guan 2012-02-07 00:17:55 PST
Created attachment 125777 [details]
Patch
Comment 15 Rob Buis 2012-02-07 04:57:53 PST
Comment on attachment 125777 [details]
Patch

LGTM
Comment 16 WebKit Review Bot 2012-02-07 05:41:45 PST
Comment on attachment 125777 [details]
Patch

Clearing flags on attachment: 125777

Committed r106931: <http://trac.webkit.org/changeset/106931>
Comment 17 WebKit Review Bot 2012-02-07 05:41:50 PST
All reviewed patches have been landed.  Closing bug.