Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011673002:
[DevTools] Add notifyPopupOpeningObservers call before running debug loop (Closed)
|
Created:
4 years, 7 months ago by kozy Modified:
4 years, 6 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kinuko+watch, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Add notifyPopupOpeningObservers call before running debug loop
BUG=613464
R=dgozman@chromium.org
Committed: https://crrev.com/8d68a7807ca440e54819580e31f511ab28564c4d
Cr-Commit-Position: refs/heads/master@{#396481}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 26 (10 generated)
Dmitry, please take a look!
dgozman@chromium.org changed reviewers: + skyostil@chromium.org, tkent@chromium.org
[+tkent, skyostil] I was about to comment that we should do this in WebView::willEnterModalLoop, but then spotted that we suspend timers here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r.... Should we use SuspendTimerQueue instead of notifyPopupOpeningObservers? Looks like they have the same purpose. In fact, since popup dialogs now use sync IPCs (and thus pause the timers), maybe we can just kill popupOpeningObservers? @skyostil, what do you think?
The main purpose of PopupOpeningObserver is to release mouse event capturing. I think SuspendTimerQueue can't replace it. I think Patch Set 2 is acceptable. Probably we should rename notifyPopupOpeningObserver to releaseMouseCapturing later.
https://codereview.chromium.org/2011673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/2011673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/ChromeClient.h:284: virtual void willEnterDebugLoop() const {} Should not go through this interface, call it on ChromeClientImpl directly.
On 2016/05/26 00:06:03, tkent wrote: > The main purpose of PopupOpeningObserver is to release mouse event capturing. I > think SuspendTimerQueue can't replace it. > > I think Patch Set 2 is acceptable. Probably we should rename > notifyPopupOpeningObserver to releaseMouseCapturing later. Thanks for clarification!
Patchset #3 (id:40001) has been deleted
All done. Please take a look! https://codereview.chromium.org/2011673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/2011673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/ChromeClient.h:284: virtual void willEnterDebugLoop() const {} On 2016/05/26 00:08:56, dgozman wrote: > Should not go through this interface, call it on ChromeClientImpl directly. Done.
On 2016/05/26 00:06:03, tkent wrote: > The main purpose of PopupOpeningObserver is to release mouse event capturing. I > think SuspendTimerQueue can't replace it. Right, seems like we'll still need PopupOpeningObservers.
lgtm https://codereview.chromium.org/2011673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/2011673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:209: widget->chromeClient().notifyPopupOpeningObservers(); We don't need this.
All done! https://codereview.chromium.org/2011673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp (right): https://codereview.chromium.org/2011673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:209: widget->chromeClient().notifyPopupOpeningObservers(); On 2016/05/26 19:27:29, dgozman wrote: > We don't need this. Done.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2011673002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011673002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011673002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2011673002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011673002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011673002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011673002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011673002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Add notifyPopupOpeningObservers call before running debug loop BUG=613464 R=dgozman@chromium.org ========== to ========== [DevTools] Add notifyPopupOpeningObservers call before running debug loop BUG=613464 R=dgozman@chromium.org Committed: https://crrev.com/8d68a7807ca440e54819580e31f511ab28564c4d Cr-Commit-Position: refs/heads/master@{#396481} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8d68a7807ca440e54819580e31f511ab28564c4d Cr-Commit-Position: refs/heads/master@{#396481} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
