Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488843002:
Switch MessagePumpWin to use base::win::MessageWindow (Closed)
DescriptionSwitch MessagePumpWin to use base::win::MessageWindow
This removes MessagePumpWin's own message window implementation in favor
of MessageWindow's implementation.
This was previously attempted as
https://chromium.googlesource.com/chromium/src/+/bddd3f69e0533c7d51f1bf041434053e2a56913e
but it seems to work fine now.
BUG=660930
Committed: https://crrev.com/9470e5bdcc0dd613dd5cc1d1ffac3d8a1eac8876
Cr-Commit-Position: refs/heads/master@{#430838}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fix DCHECK #
Total comments: 2
Patch Set 3 : NULL -> 0 #
Messages
Total messages: 30 (16 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
robliao@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: Please review this CL. Thanks!
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, I think this can't be in a DCHECK =) https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:318: BOOL ret = SetTimer(message_window_.hwnd(), NULL, delay_msec, nullptr); Should we be consistent about using nullptr?
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/08 23:20:39, dcheng wrote: > I think this can't be in a DCHECK =) This migrates the previous behavior: message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0, instance, 0); DCHECK(message_hwnd_); Would you rather this be a CHECK? https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:318: BOOL ret = SetTimer(message_window_.hwnd(), NULL, delay_msec, nullptr); On 2016/11/08 23:20:39, dcheng wrote: > Should we be consistent about using nullptr? The second argument to SetTimer is a UINT_PTR, which can't be nullptr.
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/08 23:31:56, robliao wrote: > On 2016/11/08 23:20:39, dcheng wrote: > > I think this can't be in a DCHECK =) > > This migrates the previous behavior: > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, > HWND_MESSAGE, 0, instance, 0); > DCHECK(message_hwnd_); > > Would you rather this be a CHECK? DCHECKs are compiled out, with side-effects lost in Debug builds.
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/08 23:33:54, scottmg wrote: > On 2016/11/08 23:31:56, robliao wrote: > > On 2016/11/08 23:20:39, dcheng wrote: > > > I think this can't be in a DCHECK =) > > > > This migrates the previous behavior: > > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, > > HWND_MESSAGE, 0, instance, 0); > > DCHECK(message_hwnd_); > > > > Would you rather this be a CHECK? > > DCHECKs are compiled out, with side-effects lost in Debug builds. Doh! Indeed. Fixed. Strange that this passed the bots? O_o Looks like I was conflating this with the code that helped make sure that the variable used at least got referenced. I guess the optimizer does the final cleanup.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/08 23:47:38, robliao wrote: > On 2016/11/08 23:33:54, scottmg wrote: > > On 2016/11/08 23:31:56, robliao wrote: > > > On 2016/11/08 23:20:39, dcheng wrote: > > > > I think this can't be in a DCHECK =) > > > > > > This migrates the previous behavior: > > > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, > > > HWND_MESSAGE, 0, instance, 0); > > > DCHECK(message_hwnd_); > > > > > > Would you rather this be a CHECK? > > > > DCHECKs are compiled out, with side-effects lost in Debug builds. > > Doh! Indeed. Fixed. Strange that this passed the bots? O_o > > Looks like I was conflating this with the code that helped make sure that the > variable used at least got referenced. I guess the optimizer does the final > cleanup. Unfortunately the tests on trybots are only release-with-dchecks on, because running an actual Debug is too slow.
LGTM https://codereview.chromium.org/2488843002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_win.cc:319: BOOL ret = SetTimer(message_window_.hwnd(), NULL, delay_msec, nullptr); I would personally just pass 0 directly instead then. *shrug*
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/08 23:50:26, scottmg wrote: > On 2016/11/08 23:47:38, robliao wrote: > > On 2016/11/08 23:33:54, scottmg wrote: > > > On 2016/11/08 23:31:56, robliao wrote: > > > > On 2016/11/08 23:20:39, dcheng wrote: > > > > > I think this can't be in a DCHECK =) > > > > > > > > This migrates the previous behavior: > > > > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, > > > > HWND_MESSAGE, 0, instance, 0); > > > > DCHECK(message_hwnd_); > > > > > > > > Would you rather this be a CHECK? > > > > > > DCHECKs are compiled out, with side-effects lost in Debug builds. > > > > Doh! Indeed. Fixed. Strange that this passed the bots? O_o > > > > Looks like I was conflating this with the code that helped make sure that the > > variable used at least got referenced. I guess the optimizer does the final > > cleanup. > > Unfortunately the tests on trybots are only release-with-dchecks on, because > running an actual Debug is too slow. Duly noted! Thanks! For trivia, this line expands to the following in NDEBUG builds... The "0 ?" below is particularly important for the dead code eliminator to get rid of the call later on, but we appear to compile with the full expression in the meantime. !(0 ? !(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, Unretained(this)))) : false) ? (void) 0 : ::logging::LogMessageVoidify() & (::logging::LogMessage("../../base/message_loop/message_pump_win.cc", 96, ::logging::LOG_INFO ).stream()) << "Check failed: " "message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, Unretained(this)))" ". "; https://codereview.chromium.org/2488843002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_win.cc:319: BOOL ret = SetTimer(message_window_.hwnd(), NULL, delay_msec, nullptr); On 2016/11/08 23:51:38, dcheng wrote: > I would personally just pass 0 directly instead then. *shrug* sgtm. Done.
https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:95: DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, On 2016/11/09 00:54:01, robliao wrote: > On 2016/11/08 23:50:26, scottmg wrote: > > On 2016/11/08 23:47:38, robliao wrote: > > > On 2016/11/08 23:33:54, scottmg wrote: > > > > On 2016/11/08 23:31:56, robliao wrote: > > > > > On 2016/11/08 23:20:39, dcheng wrote: > > > > > > I think this can't be in a DCHECK =) > > > > > > > > > > This migrates the previous behavior: > > > > > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, > > > > > HWND_MESSAGE, 0, instance, 0); > > > > > DCHECK(message_hwnd_); > > > > > > > > > > Would you rather this be a CHECK? > > > > > > > > DCHECKs are compiled out, with side-effects lost in Debug builds. > > > > > > Doh! Indeed. Fixed. Strange that this passed the bots? O_o > > > > > > Looks like I was conflating this with the code that helped make sure that > the > > > variable used at least got referenced. I guess the optimizer does the final > > > cleanup. > > > > Unfortunately the tests on trybots are only release-with-dchecks on, because > > running an actual Debug is too slow. > > Duly noted! Thanks! > > For trivia, this line expands to the following in NDEBUG builds... > The "0 ?" below is particularly important for the dead code eliminator to get > rid of the call later on, but we appear to compile with the full expression in > the meantime. > > !(0 ? !(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, > Unretained(this)))) : false) ? (void) 0 : ::logging::LogMessageVoidify() & > (::logging::LogMessage("../../base/message_loop/message_pump_win.cc", 96, > ::logging::LOG_INFO ).stream()) << "Check failed: " > "message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, > Unretained(this)))" ". "; Yeah, that sucks. :( I have the feeling that it's something to do with the wacky << logging, so we can't safely compile it out at the preprocessor level. But it could just be that the macro expansions scare everyone so they don't get touched. FWIW, I did just notice a call to ~LogMessage that I didn't expect to see while investigating crbug.com/614753 where the only cause looks like it'd be a DCHECK. So maybe the compiler doesn't always get rid of everything? I'm not sure, but it might be worth investigating a bit more.
On 2016/11/09 01:00:01, scottmg wrote: > https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... > File base/message_loop/message_pump_win.cc (right): > > https://codereview.chromium.org/2488843002/diff/1/base/message_loop/message_p... > base/message_loop/message_pump_win.cc:95: > DCHECK(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, > On 2016/11/09 00:54:01, robliao wrote: > > On 2016/11/08 23:50:26, scottmg wrote: > > > On 2016/11/08 23:47:38, robliao wrote: > > > > On 2016/11/08 23:33:54, scottmg wrote: > > > > > On 2016/11/08 23:31:56, robliao wrote: > > > > > > On 2016/11/08 23:20:39, dcheng wrote: > > > > > > > I think this can't be in a DCHECK =) > > > > > > > > > > > > This migrates the previous behavior: > > > > > > message_hwnd_ = CreateWindowEx(0, MAKEINTATOM(atom_), 0, 0, 0, 0, 0, > 0, > > > > > > HWND_MESSAGE, 0, instance, 0); > > > > > > DCHECK(message_hwnd_); > > > > > > > > > > > > Would you rather this be a CHECK? > > > > > > > > > > DCHECKs are compiled out, with side-effects lost in Debug builds. > > > > > > > > Doh! Indeed. Fixed. Strange that this passed the bots? O_o > > > > > > > > Looks like I was conflating this with the code that helped make sure that > > the > > > > variable used at least got referenced. I guess the optimizer does the > final > > > > cleanup. > > > > > > Unfortunately the tests on trybots are only release-with-dchecks on, because > > > running an actual Debug is too slow. > > > > Duly noted! Thanks! > > > > For trivia, this line expands to the following in NDEBUG builds... > > The "0 ?" below is particularly important for the dead code eliminator to get > > rid of the call later on, but we appear to compile with the full expression in > > the meantime. > > > > !(0 ? !(message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, > > Unretained(this)))) : false) ? (void) 0 : ::logging::LogMessageVoidify() & > > (::logging::LogMessage("../../base/message_loop/message_pump_win.cc", 96, > > ::logging::LOG_INFO ).stream()) << "Check failed: " > > "message_window_.Create(Bind(&MessagePumpForUI::MessageCallback, > > Unretained(this)))" ". "; > > Yeah, that sucks. :( I have the feeling that it's something to do with the wacky > << logging, so we can't safely compile it out at the preprocessor level. But it > could just be that the macro expansions scare everyone so they don't get > touched. > > FWIW, I did just notice a call to ~LogMessage that I didn't expect to see while > investigating crbug.com/614753 where the only cause looks like it'd be a DCHECK. > So maybe the compiler doesn't always get rid of everything? I'm not sure, but it > might be worth investigating a bit more. As I recall, DCHECK tries to accomplish two goals when NDEBUG is true: - Compile to no code. - Not result in a bunch of unused variable warnings. In theory, both of these should be true, but I'm not sure if anyone's looked at it lately.
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2488843002/#ps40001 (title: "NULL -> 0")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Switch MessagePumpWin to use base::win::MessageWindow This removes MessagePumpWin's own message window implementation in favor of MessageWindow's implementation. This was previously attempted as https://chromium.googlesource.com/chromium/src/+/bddd3f69e0533c7d51f1bf041434... but it seems to work fine now. BUG=660930 ========== to ========== Switch MessagePumpWin to use base::win::MessageWindow This removes MessagePumpWin's own message window implementation in favor of MessageWindow's implementation. This was previously attempted as https://chromium.googlesource.com/chromium/src/+/bddd3f69e0533c7d51f1bf041434... but it seems to work fine now. BUG=660930 Committed: https://crrev.com/9470e5bdcc0dd613dd5cc1d1ffac3d8a1eac8876 Cr-Commit-Position: refs/heads/master@{#430838} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9470e5bdcc0dd613dd5cc1d1ffac3d8a1eac8876 Cr-Commit-Position: refs/heads/master@{#430838} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
