Bug 77926

Fixing up the title.

Created attachment 125759 [details] Patch

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.

Created attachment 125762 [details] Patch

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

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.

(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:)

(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:)

(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"?

(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.

(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?

(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.

(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.

Created attachment 125777 [details] Patch

Comment on attachment 125777 [details] Patch LGTM

Comment on attachment 125777 [details] Patch Clearing flags on attachment: 125777 Committed r106931: <http://trac.webkit.org/changeset/106931>

All reviewed patches have been landed. Closing bug.