Bug 214896

Created attachment 405416 [details] Patch

Comment on attachment 405416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review > Source/WebKit/UIProcess/ProcessThrottler.cpp:138 > + m_assertion->setClient(*this); Good catch but I think this should be done unconditionally.

Comment on attachment 405416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review >> Source/WebKit/UIProcess/ProcessThrottler.cpp:138 >> + m_assertion->setClient(*this); > > Good catch but I think this should be done unconditionally. Also we probably want to drop the m_assertion->setClient(*this) call in ProcessThrottler::didConnectToProcess()

Comment on attachment 405416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review > Source/WebKit/UIProcess/ProcessThrottler.cpp:129 > + bool hasClient = !!m_assertion->client(); m_assertion can be null so this will crash.

Created attachment 405445 [details] Patch

<rdar://problem/65138628>

Comment on attachment 405445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405445&action=review > Source/WebKit/UIProcess/ProcessAssertion.h:69 > + ProcessAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr); It is not really less error-prone if the parameter is optional.. Also, why can it be null? > Source/WebKit/UIProcess/ProcessAssertion.h:100 > + ProcessAndUIAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr); Ditto. > Source/WebKit/UIProcess/ProcessThrottler.h:54 > +class ProcessThrottler : public CanMakeWeakPtr<ProcessThrottler>, public ProcessAssertion::Client { Why is this needed?

Comment on attachment 405445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405445&action=review >> Source/WebKit/UIProcess/ProcessAssertion.h:69 >> + ProcessAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr); > > It is not really less error-prone if the parameter is optional.. Also, why can it be null? Ok, I see that ProcessAssertion is used by other things than ProcessThrottler (like DownloadMap) and those don't set themselves as client. So using a raw pointer seems OK. That said, I don't think the parameter should be optional. If it is optional, then it is not any less error-prone than setting the client later using a setter.

> Ok, I see that ProcessAssertion is used by other things than > ProcessThrottler (like DownloadMap) and those don't set themselves as > client. So using a raw pointer seems OK. That said, I don't think the > parameter should be optional. If it is optional, then it is not any less > error-prone than setting the client later using a setter. Plan is to make it mandatory and pass a ref instead of a pointer. But this should be a follow-up because of other assertions (download and media ones at least). Probably we should be also using a WeakPtr instead of a pointer.

Keeping it optional just reduces the size of the patch basically.

(In reply to youenn fablet from comment #9) > > Ok, I see that ProcessAssertion is used by other things than > > ProcessThrottler (like DownloadMap) and those don't set themselves as > > client. So using a raw pointer seems OK. That said, I don't think the > > parameter should be optional. If it is optional, then it is not any less > > error-prone than setting the client later using a setter. > > Plan is to make it mandatory and pass a ref instead of a pointer. > But this should be a follow-up because of other assertions (download and > media ones at least). > Probably we should be also using a WeakPtr instead of a pointer. I think parameter should be mandatory. Clients can explicitly pass null for now.

(In reply to youenn fablet from comment #10) > Keeping it optional just reduces the size of the patch basically. But makes it error prone so I am against it.

(In reply to youenn fablet from comment #10) > Keeping it optional just reduces the size of the patch basically. There are not that many call sites and it is just an extra parameter. If you want to keep the patch small, then we don't have to do the refactoring at all and we can keep setClient().

I uploaded a refactoring proposal at Bug 214976 because I don't like the Client interface. If you land this fix first, I will rebase my patch.

(In reply to Chris Dumez from comment #14) > I uploaded a refactoring proposal at Bug 214976 because I don't like the > Client interface. If you land this fix first, I will rebase my patch. My personal preference would be you landing the 2 line change to fix the bug here (moving setClient from didConnectToProcess to setAssertionType) and then doing the refactoring in Bug 214976.

Fixed by https://trac.webkit.org/r265162