Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1582123002:
Augment message_loop with ability for custom UI message pumps (issue 576536) (Closed)
|
Created:
4 years, 11 months ago by jsbache Modified:
4 years, 10 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, sadrul, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis change modifies the message_loop so one can create UI message_loops with a custom message_pump.
This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-host-owned-message.
In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump.
BUG=576536
Committed: https://crrev.com/c7bfe00735a4930c4915ae04ac5ab3af52f000b5
Cr-Commit-Position: refs/heads/master@{#372265}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Improve header comment as requested in review #Patch Set 3 : Add name to Authors file #
Messages
Total messages: 58 (22 generated)
jsbache@gmail.com changed reviewers: + mark@chromium.org
Proposed change to the message pump allowing projects such as CEF to create a UI message loop with an externally provided message pump. See https://code.google.com/p/chromium/issues/detail?id=576536 and: https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-... https://bitbucket.org/chromiumembedded/cef/pull-requests/47/changes-related-t...
https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... base/message_loop/message_loop.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. Give this change an appropriate description. The description becomes the commit message, and nobody knows or cares what bug 123456 is when they’re scanning commit messages. https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... base/message_loop/message_loop.h:408: // Configure various members and bind this message loop to the current thread. Since this is now protected, you should say when and how it’s expected to be used, like how you adapted the constructor comment above.
Description was changed from ========== proposed fix for 576536 BUG=576536 ========== to ========== This change modifies the message_loop so one can create UI message_loops with a custom message_pump. This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-.... In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump. BUG=576536 ==========
On 2016/01/25 14:54:55, Mark Mentovai wrote: > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > File base/message_loop/message_loop.h (right): > > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > base/message_loop/message_loop.h:1: // Copyright 2013 The Chromium Authors. All > rights reserved. > Give this change an appropriate description. The description becomes the commit > message, and nobody knows or cares what bug 123456 is when they’re scanning > commit messages. > > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > base/message_loop/message_loop.h:408: // Configure various members and bind this > message loop to the current thread. > Since this is now protected, you should say when and how it’s expected to be > used, like how you adapted the constructor comment above. Issue description updated. I am running into the following issue when I try to check out the branch related to this issue, so at the moment I am having a hard time updating the header comment.. I am planning on adding the following comment when I get access to these files next: === // Common protected constructor. Other constructors delegate the initialization // to this constructor. // A subclass can invoke this constructor to create a message_loop of a // specific type with a custom loop. The implementation does not call // BindToCurrentThread. If this constructor is invoked directly by a subclass, // then the subclass must subsequently bind the message loop. ===
On 2016/01/25 19:39:18, jsbache wrote: > On 2016/01/25 14:54:55, Mark Mentovai wrote: > > > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > > File base/message_loop/message_loop.h (right): > > > > > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > > base/message_loop/message_loop.h:1: // Copyright 2013 The Chromium Authors. > All > > rights reserved. > > Give this change an appropriate description. The description becomes the > commit > > message, and nobody knows or cares what bug 123456 is when they’re scanning > > commit messages. > > > > > https://codereview.chromium.org/1582123002/diff/1/base/message_loop/message_l... > > base/message_loop/message_loop.h:408: // Configure various members and bind > this > > message loop to the current thread. > > Since this is now protected, you should say when and how it’s expected to be > > used, like how you adapted the constructor comment above. > > Issue description updated. > I am running into the following issue when I try to check out the branch related > to this issue: > https://code.google.com/p/chromium/issues/detail?id=579187 > At the moment I am therefore having a hard time updating the header > comment.. > I am planning on adding the following comment when I get access to these files > next: > === > // Common protected constructor. Other constructors delegate the initialization > // to this constructor. > // A subclass can invoke this constructor to create a message_loop of a > // specific type with a custom loop. The implementation does not call > // BindToCurrentThread. If this constructor is invoked directly by a subclass, > // then the subclass must subsequently bind the message loop. > ===
That sounds good. Poke this review once again when you’ve updated it.
On 2016/01/25 19:44:28, Mark Mentovai wrote: > That sounds good. Poke this review once again when you’ve updated it. New header uploaded
LGTM
The CQ bit was checked by jsbache@gmail.com
The CQ bit was checked by jsbache@gmail.com
The CQ bit was unchecked by jsbache@gmail.com
The CQ bit was checked by jsbache@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author jsbache@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by jsbache@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author jsbache@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by jsbache@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author jsbache@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2016/01/26 20:56:30, commit-bot: I haz the power wrote: > The author mailto:jsbache@gmail.com has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. The CLA was signed this morning, and the link gives me: "It looks like you've already signed this CLA" Maybe I need to wait for this information to peculate through the system? Any idea of how long the normal wait is?
The CQ bit was checked by mark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author jsbache@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was unchecked by mark@chromium.org
I just checked and don’t see you on the CLA signers list. Did you use a different e-mail address?
On 2016/01/26 22:25:00, Mark Mentovai wrote: > I just checked and don’t see you on the CLA signers list. Did you use a > different e-mail address? I should be listed under jsbache@gmail.com That is the email I used to re-sign. An (unlikely) alternative is jsbache@adobe.com
On 2016/01/26 22:44:49, jsbache wrote: > On 2016/01/26 22:25:00, Mark Mentovai wrote: > > I just checked and don’t see you on the CLA signers list. Did you use a > > different e-mail address? > > I should be listed under mailto:jsbache@gmail.com > That is the email I used try to re-sign (and where I get the message saying that I have already signed). > An (unlikely) alternative is mailto:jsbache@adobe.com
OK, I filed a ticket to get to the bottom of this.
The CQ bit was checked by jsbache@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
The author jsbache@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The response I got asked you to try signing the CLA again while signed in as jsbache@gmail.com at https://cla.developers.google.com/clas. If you try that and it doesn’t work, please let me know the exact error that you receive.
On 2016/01/27 21:53:10, Mark Mentovai wrote: > The response I got asked you to try signing the CLA again while signed in as > mailto:jsbache@gmail.com at https://cla.developers.google.com/clas. If you try that and > it doesn’t work, please let me know the exact error that you receive. Same problem: :"It looks like you've already signed this CLA."
Thanks. Still digging.
They want you to try signing the CLA at https://cla.developers.google.com/clas from an incognito window and verifying that jsbache@gmail.com shows up in the top right. The CLA-signer site doesn’t support multi-login, and this (I agree, kinda obvious-seeming) step is supposed to weed out the possibility that you signed it from an unexpected account.
On 2016/01/27 22:39:16, Mark Mentovai wrote: > They want you to try signing the CLA at https://cla.developers.google.com/clas > from an incognito window and verifying that mailto:jsbache@gmail.com shows up in the > top right. The CLA-signer site doesn’t support multi-login, and this (I agree, > kinda obvious-seeming) step is supposed to weed out the possibility that you > signed it from an unexpected account. Just did that: - Incognito Window - Got fresh login page - Logged in as jsbache@gmail.com & verified top right corner: "Logged in as jsbache@gmail.com" Same error message
Thanks.
On 2016/01/27 22:43:16, jsbache wrote: > On 2016/01/27 22:39:16, Mark Mentovai wrote: > > They want you to try signing the CLA at https://cla.developers.google.com/clas > > from an incognito window and verifying that mailto:jsbache@gmail.com shows up > in the > > top right. The CLA-signer site doesn’t support multi-login, and this (I agree, > > kinda obvious-seeming) step is supposed to weed out the possibility that you > > signed it from an unexpected account. > > Just did that: > - Incognito Window > - Got fresh login page > - Logged in as mailto:jsbache@gmail.com & verified top right corner: "Logged in as > mailto:jsbache@gmail.com" > Same error message Feel free to have support email me directly at jsbache@gmail.com or jsbache@adobe.com if that makes things easier
The CQ bit was checked by jsbache@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/01/28 18:47:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Although I got a CLA confirmation email and I got further in the "commit" process this time, it still looks as if the Linux pre-submit fails with the CLA check: ** Presubmit Warnings ** jsbache@gmail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA.
You can add yourself to the AUTHORS file in this change.
The CQ bit was checked by jsbache@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/1582123002/#ps40001 (title: "Add name to Authors file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582123002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582123002/40001
Message was sent while issue was closed.
Description was changed from ========== This change modifies the message_loop so one can create UI message_loops with a custom message_pump. This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-.... In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump. BUG=576536 ========== to ========== This change modifies the message_loop so one can create UI message_loops with a custom message_pump. This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-.... In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump. BUG=576536 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== This change modifies the message_loop so one can create UI message_loops with a custom message_pump. This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-.... In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump. BUG=576536 ========== to ========== This change modifies the message_loop so one can create UI message_loops with a custom message_pump. This change is needed to support the embedded case where Chromium runs inside a foreign host application. One example of this is the CEF project. See https://bitbucket.org/chromiumembedded/cef/issues/1805/improve-support-for-a-.... In the embedded case the host runs the OS message loop and the UI message_loop needs to delegate back to the host without pumping messages inside the standard UI message_pump. BUG=576536 Committed: https://crrev.com/c7bfe00735a4930c4915ae04ac5ab3af52f000b5 Cr-Commit-Position: refs/heads/master@{#372265} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c7bfe00735a4930c4915ae04ac5ab3af52f000b5 Cr-Commit-Position: refs/heads/master@{#372265} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
