Reproduction: 1. Go to https://whsieh.github.io/examples/focus-navigate on an iPhone or iPad. 2. Tap the top input field
rdar://76792407
Created attachment 427805 [details] For EWS
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review > Source/WebKit/ChangeLog:37 > + Make a slight adjustment here so that `waitForMessage` bails if one of the messages we're dispatching underneath > + `SyncMessageState::dispatchMessages` is the message we happen to be waiting for anyways. In this case, it's > + necessary in order for the preemptive `WebPageProxy::HandleAutocorrectionContext` message sent by the web > + process to unblock the UI process, which is waiting for a `WebPageProxy::HandleAutocorrectionContext` response. This part could use a careful Chris review, but does not seem outrageous (once Wenson explained to me that MessageName is now unique across receivers, which I missed).
(In reply to Tim Horton from comment #3) > Comment on attachment 427805 [details] > For EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427805&action=review > > > Source/WebKit/ChangeLog:37 > > + Make a slight adjustment here so that `waitForMessage` bails if one of the messages we're dispatching underneath > > + `SyncMessageState::dispatchMessages` is the message we happen to be waiting for anyways. In this case, it's > > + necessary in order for the preemptive `WebPageProxy::HandleAutocorrectionContext` message sent by the web > > + process to unblock the UI process, which is waiting for a `WebPageProxy::HandleAutocorrectionContext` response. > > This part could use a careful Chris review, but does not seem outrageous > (once Wenson explained to me that MessageName is now unique across > receivers, which I missed). I don't object to the IPC change, I think it makes sense. However, I find the way it is currently implemented to be a bit confusing. Could you please give me a bit to come up with a proposal before landing?
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review r=me as well, nice fix! > Source/WebKit/UIProcess/PageClient.h:339 > + virtual void runModalJavaScriptDialog(Function<void()>&& callback) { callback(); } `CompletionHandler`? > Source/WebKit/UIProcess/WebPageProxy.cpp:5709 > + pageClient().runModalJavaScriptDialog([this, protectedThis = makeRef(*this), reply = WTFMove(reply), frameInfo = WTFMove(frameInfo), frame = WTFMove(frame), message]() mutable { Rather than have `protectedThis`, should we just `makeWeakPtr(*this)` here? Or are we not doing that on the assumption that most of the time this will not be called in response to keyboard activity?
(In reply to Chris Dumez from comment #4) > (In reply to Tim Horton from comment #3) > > Comment on attachment 427805 [details] > > For EWS > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427805&action=review > > > > > Source/WebKit/ChangeLog:37 > > > + Make a slight adjustment here so that `waitForMessage` bails if one of the messages we're dispatching underneath > > > + `SyncMessageState::dispatchMessages` is the message we happen to be waiting for anyways. In this case, it's > > > + necessary in order for the preemptive `WebPageProxy::HandleAutocorrectionContext` message sent by the web > > > + process to unblock the UI process, which is waiting for a `WebPageProxy::HandleAutocorrectionContext` response. > > > > This part could use a careful Chris review, but does not seem outrageous > > (once Wenson explained to me that MessageName is now unique across > > receivers, which I missed). > > I don't object to the IPC change, I think it makes sense. However, I find > the way it is currently implemented to be a bit confusing. Could you please > give me a bit to come up with a proposal before landing? Followed-up on Slack with Tim and Wenson.
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review > Source/WebKit/Platform/IPC/Connection.cpp:74 > +enum class DispatchedWaitingForMessage : bool { No, Yes }; This doesn’t seem like straightforward language. There’s a message name that we are watching for. I would not say we are "waiting for" it inside the dispatchMessages function, since that function doesn’t stop dispatching messages when we see it. It’s the next level up that does that. Also, "message we are waiting for" and "waiting for message" do not seem to mean the same thing; an existing bit of confusing terminology. I might name this something like "saw expected message". We should follow the rule that first we think through how we would say this to another person and then try to name it after that. > Source/WebKit/Platform/IPC/Connection.cpp:97 > + DispatchedWaitingForMessage dispatchMessages(Optional<MessageName> waitingForMessageName = WTF::nullopt); Maybe this is the "expected message name"? > Source/WebKit/Platform/IPC/Connection.cpp:215 > + if (waitingForMessageName && messageToDispatch.message->messageName() == *waitingForMessageName) The way the Optional class works, and how it defines ==, I think you can just write this: if (messageToDispatch.message->messageName() == waitingForMessageName) And if we were using plain booleans instead of enumerations, we could write: dispatchedWaitingForMessage |= messageToDispatch.message->messageName() == waitingForMessageName; But maybe "expected" instead of "waiting for"? We can convert the boolean into the unambiguous enumeration at the return statement, and still get the important clarity at the call site. Here inside the function I think the boolean should be just fine. > Source/WebKit/Platform/IPC/Connection.cpp:584 > + m_waitingForMessage = nullptr; At this level, it is the message we are waiting for, so the name is not as obviously wrong, but "m_waitingForMessage" is still not a great name. > Source/WebKit/UIProcess/WebPageProxy.cpp:5716 > + pageClient().runModalJavaScriptDialog([this, protectedThis = makeRef(*this), reply = WTFMove(reply), frameInfo = WTFMove(frameInfo), frame = WTFMove(frame), message]() mutable { > + m_isRunningModalJavaScriptDialog = true; > + m_uiClient->runJavaScriptAlert(*this, message, frame.get(), WTFMove(frameInfo), [reply = WTFMove(reply), weakThis = makeWeakPtr(*this)]() mutable { > + if (auto strongThis = makeRefPtr(weakThis.get())) > + strongThis->m_isRunningModalJavaScriptDialog = false; > + reply(); > + }); > + }); I think we can refactor this so the code is shared among the three JavaScript dialog types, instead of the tricky idiom being written out three times. There should be one function call here, with just one lambda. The code to set and then clear m_isRunningModalJavaScriptDialog should be in that single helper instead of related thrice. While it’s OK to land it like this, it’s not hard to refactor it to be more like that. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2599 > + if (!m_focusedElement) > + return; > + > + if (!m_focusedElement->hasEditableStyle() && !is<HTMLTextFormControlElement>(*m_focusedElement)) > + return; > + > + send(Messages::WebPageProxy::HandleAutocorrectionContext(autocorrectionContext())); If ever a function needed a "why" comment, this is the one. We avoid a synchronous hang by proactively sending this, which is so non-obvious! But it works. And even better, we have a test that covers it.
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review >> Source/WebKit/Platform/IPC/Connection.cpp:74 >> +enum class DispatchedWaitingForMessage : bool { No, Yes }; > > This doesn’t seem like straightforward language. There’s a message name that we are watching for. I would not say we are "waiting for" it inside the dispatchMessages function, since that function doesn’t stop dispatching messages when we see it. It’s the next level up that does that. Also, "message we are waiting for" and "waiting for message" do not seem to mean the same thing; an existing bit of confusing terminology. > > I might name this something like "saw expected message". We should follow the rule that first we think through how we would say this to another person and then try to name it after that. I had a hard time understanding the naming of this enum. > Source/WebKit/Platform/IPC/Connection.cpp:197 > +DispatchedWaitingForMessage Connection::SyncMessageState::dispatchMessages(Optional<MessageName> waitingForMessageName) The naming is confusing here and I really wish Connection::SyncMessageState::dispatchMessages() did not have to know about the "waitFor" logic, especially in its declaration. My proposal is to pass a `Function<void(MessageName, uint64_t destinationID)>` that can be called willDispatchMessage or didDispatchMessage depending on when you call it. Then the design is a bit more generic and the naming is not difficult. >> Source/WebKit/Platform/IPC/Connection.cpp:215 >> + if (waitingForMessageName && messageToDispatch.message->messageName() == *waitingForMessageName) > > The way the Optional class works, and how it defines ==, I think you can just write this: > > if (messageToDispatch.message->messageName() == waitingForMessageName) > > And if we were using plain booleans instead of enumerations, we could write: > > dispatchedWaitingForMessage |= messageToDispatch.message->messageName() == waitingForMessageName; > > But maybe "expected" instead of "waiting for"? > > We can convert the boolean into the unambiguous enumeration at the return statement, and still get the important clarity at the call site. Here inside the function I think the boolean should be just fine. Note that this needs to check the destinationID in addition to the MessageName to make sure they match.
Comment on attachment 427805 [details] For EWS r- because of the missing destinationID check.
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review >>>> Source/WebKit/ChangeLog:37 >>>> + process to unblock the UI process, which is waiting for a `WebPageProxy::HandleAutocorrectionContext` response. >>> >>> This part could use a careful Chris review, but does not seem outrageous (once Wenson explained to me that MessageName is now unique across receivers, which I missed). >> >> I don't object to the IPC change, I think it makes sense. However, I find the way it is currently implemented to be a bit confusing. Could you please give me a bit to come up with a proposal before landing? > > Followed-up on Slack with Tim and Wenson. 👍🏻 I'm changing dispatchMessages to `void dispatchMessages(Function<void(MessageName, uint64_t)>&& willDispatchMessage)`, instead of passing back an enum and taking an optional "waitForMessage" name. >>> Source/WebKit/Platform/IPC/Connection.cpp:74 >>> +enum class DispatchedWaitingForMessage : bool { No, Yes }; >> >> This doesn’t seem like straightforward language. There’s a message name that we are watching for. I would not say we are "waiting for" it inside the dispatchMessages function, since that function doesn’t stop dispatching messages when we see it. It’s the next level up that does that. Also, "message we are waiting for" and "waiting for message" do not seem to mean the same thing; an existing bit of confusing terminology. >> >> I might name this something like "saw expected message". We should follow the rule that first we think through how we would say this to another person and then try to name it after that. > > I had a hard time understanding the naming of this enum. Ah, so I'm removed this enum altogether (and am using a slightly different approach) after some discussion with Chris and Tim. >> Source/WebKit/Platform/IPC/Connection.cpp:97 >> + DispatchedWaitingForMessage dispatchMessages(Optional<MessageName> waitingForMessageName = WTF::nullopt); > > Maybe this is the "expected message name"? (I've removed this argument as well) >>> Source/WebKit/Platform/IPC/Connection.cpp:215 >>> + if (waitingForMessageName && messageToDispatch.message->messageName() == *waitingForMessageName) >> >> The way the Optional class works, and how it defines ==, I think you can just write this: >> >> if (messageToDispatch.message->messageName() == waitingForMessageName) >> >> And if we were using plain booleans instead of enumerations, we could write: >> >> dispatchedWaitingForMessage |= messageToDispatch.message->messageName() == waitingForMessageName; >> >> But maybe "expected" instead of "waiting for"? >> >> We can convert the boolean into the unambiguous enumeration at the return statement, and still get the important clarity at the call site. Here inside the function I think the boolean should be just fine. > > Note that this needs to check the destinationID in addition to the MessageName to make sure they match. Ah, so this code has been removed in the new version as well; that said, I can employ the `|=` trick in my new version. >> Source/WebKit/Platform/IPC/Connection.cpp:584 >> + m_waitingForMessage = nullptr; > > At this level, it is the message we are waiting for, so the name is not as obviously wrong, but "m_waitingForMessage" is still not a great name. Yeah, I'm always a tiny bit confused by the names around here when I venture into this code. Perhaps `m_messageToWaitFor`? (It's a minor grammar error to end with a preposition, but for the sake of clarity it may be an improvement). I'll think I'll tackle this separately. >> Source/WebKit/UIProcess/PageClient.h:339 >> + virtual void runModalJavaScriptDialog(Function<void()>&& callback) { callback(); } > > `CompletionHandler`? 👍🏻 >> Source/WebKit/UIProcess/WebPageProxy.cpp:5709 >> + pageClient().runModalJavaScriptDialog([this, protectedThis = makeRef(*this), reply = WTFMove(reply), frameInfo = WTFMove(frameInfo), frame = WTFMove(frame), message]() mutable { > > Rather than have `protectedThis`, should we just `makeWeakPtr(*this)` here? Or are we not doing that on the assumption that most of the time this will not be called in response to keyboard activity? 👍🏻 >> Source/WebKit/UIProcess/WebPageProxy.cpp:5716 >> + }); > > I think we can refactor this so the code is shared among the three JavaScript dialog types, instead of the tricky idiom being written out three times. There should be one function call here, with just one lambda. The code to set and then clear m_isRunningModalJavaScriptDialog should be in that single helper instead of related thrice. While it’s OK to land it like this, it’s not hard to refactor it to be more like that. Hm…so I actually tried to factor this out into a separate helper method, but got stuck trying to make it work with templates for each of the reply types. But I suppose you're suggesting the helper function could just take a single lambda and just manage the `m_isRunningModalJavaScriptDialog` member? I'll see if I can factor it out that way in this patch before I land. >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2599 >> + send(Messages::WebPageProxy::HandleAutocorrectionContext(autocorrectionContext())); > > If ever a function needed a "why" comment, this is the one. We avoid a synchronous hang by proactively sending this, which is so non-obvious! But it works. And even better, we have a test that covers it. Good point! Added a comment here.
Comment on attachment 427805 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=427805&action=review >>> Source/WebKit/UIProcess/WebPageProxy.cpp:5716 >>> + }); >> >> I think we can refactor this so the code is shared among the three JavaScript dialog types, instead of the tricky idiom being written out three times. There should be one function call here, with just one lambda. The code to set and then clear m_isRunningModalJavaScriptDialog should be in that single helper instead of related thrice. While it’s OK to land it like this, it’s not hard to refactor it to be more like that. > > Hm…so I actually tried to factor this out into a separate helper method, but got stuck trying to make it work with templates for each of the reply types. But I suppose you're suggesting the helper function could just take a single lambda and just manage the `m_isRunningModalJavaScriptDialog` member? I'll see if I can factor it out that way in this patch before I land. Yes, that’s exactly my suggestion. There are other options too, but I think this most purely follows the "capture what’s different, then call the function that does everything else” pattern. It seems that what’s different can be a function for these three cases. And since this isn’t general purpose some of the arguments could be passed to the function; doesn’t have to be all done with captures.
Created attachment 427818 [details] Address feedback
Comment on attachment 427818 [details] Address feedback View in context: https://bugs.webkit.org/attachment.cgi?id=427818&action=review IPC changes look good to me with a nit. > Source/WebKit/Platform/IPC/Connection.cpp:573 > + bool foundMessageToWaitFor = false; I would name this wasMessageToWaitForAlreadyDispatched or something similar. "found" is a bit unclear in this context.
Created attachment 427821 [details] Rename a variable
(In reply to Chris Dumez from comment #13) > Comment on attachment 427818 [details] > Address feedback > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427818&action=review > > IPC changes look good to me with a nit. > > > Source/WebKit/Platform/IPC/Connection.cpp:573 > > + bool foundMessageToWaitFor = false; > > I would name this wasMessageToWaitForAlreadyDispatched or something similar. > "found" is a bit unclear in this context. Makes sense! Renamed to `wasMessageToWaitForAlreadyDispatched`
Comment on attachment 427821 [details] Rename a variable View in context: https://bugs.webkit.org/attachment.cgi?id=427821&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:5703 > + runDialogCallback(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = WTFMove(weakThis)]() mutable { We should pass *protectedThis in as one of the arguments here so the caller doesn’t have to capture weakThis. Also, we should pass frame.get() instead of passing frame as an rvalue reference since the client just needs to turn around and pass it. > Source/WebKit/UIProcess/WebPageProxy.cpp:5728 > + auto protectedThis = makeRefPtr(weakThis.get()); > + if (!protectedThis) > + return; This code isn’t needed any more, because runModalJavaScriptDialog already does this check. If it had passed us a reference to the WebPageProxy we could just use that. > Source/WebKit/UIProcess/WebPageProxy.cpp:5730 > + protectedThis->m_uiClient->runJavaScriptAlert(*protectedThis, message, frame.get(), WTFMove(frameInfo), [reply = WTFMove(reply), weakThis = WTFMove(weakThis), completion = WTFMove(completion)]() mutable { No need to capture weakThis here.
Created attachment 427836 [details] More minor adjustments
Comment on attachment 427821 [details] Rename a variable View in context: https://bugs.webkit.org/attachment.cgi?id=427821&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:5703 >> + runDialogCallback(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = WTFMove(weakThis)]() mutable { > > We should pass *protectedThis in as one of the arguments here so the caller doesn’t have to capture weakThis. Also, we should pass frame.get() instead of passing frame as an rvalue reference since the client just needs to turn around and pass it. 👍🏻 >> Source/WebKit/UIProcess/WebPageProxy.cpp:5728 >> + return; > > This code isn’t needed any more, because runModalJavaScriptDialog already does this check. If it had passed us a reference to the WebPageProxy we could just use that. Good point — changed to the latter. >> Source/WebKit/UIProcess/WebPageProxy.cpp:5730 >> + protectedThis->m_uiClient->runJavaScriptAlert(*protectedThis, message, frame.get(), WTFMove(frameInfo), [reply = WTFMove(reply), weakThis = WTFMove(weakThis), completion = WTFMove(completion)]() mutable { > > No need to capture weakThis here. Fixed!
Comment on attachment 427836 [details] More minor adjustments View in context: https://bugs.webkit.org/attachment.cgi?id=427836&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:5725 > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { Still unnecessarily capturing weakThis here. Should delete that. > Source/WebKit/UIProcess/WebPageProxy.cpp:5748 > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { Ditto. > Source/WebKit/UIProcess/WebPageProxy.cpp:5771 > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply), defaultValue](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { Ditto.
(In reply to Darin Adler from comment #19) > Comment on attachment 427836 [details] > More minor adjustments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427836&action=review > > > Source/WebKit/UIProcess/WebPageProxy.cpp:5725 > > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { > > Still unnecessarily capturing weakThis here. Should delete that. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:5748 > > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { > > Ditto. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:5771 > > + runModalJavaScriptDialog(WTFMove(frame), WTFMove(frameInfo), message, [weakThis = makeWeakPtr(*this), reply = WTFMove(reply), defaultValue](WebPageProxy& page, WebFrameProxy* frame, FrameInfoData&& frameInfo, const String& message, CompletionHandler<void()>&& completion) mutable { > > Ditto. Fixed! (I forgot that I could delete those as well after plumbing the `WebPageProxy` through completion handlers). Thank you for the reviews!
Created attachment 427890 [details] Patch for landing
Comment on attachment 427836 [details] More minor adjustments View in context: https://bugs.webkit.org/attachment.cgi?id=427836&action=review > Source/WebKit/Platform/IPC/Connection.cpp:574 > + bool wasMessageToWaitForAlreadyDispatched = false; > // Handle any messages that are blocked on a response from us. I’d reverse these two lines to make the code slightly more readable. I think the definition of the boolean fits in well with 3 lines below that set it while dispatching.
Created attachment 427892 [details] Patch for landing
Committed r277097 (237399@main): <https://commits.webkit.org/237399@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427892 [details].