Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363153002:
Migrate Linux Blimp client to use BlimpClientContext (Closed)
|
Created:
4 years, 3 months ago by steimel Modified:
4 years, 2 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext.
For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature).
blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread)
BUG=645202
Committed: https://crrev.com/d890645241cfa41fe40e22a5cdf5cd9d6cef4f01
Cr-Commit-Position: refs/heads/master@{#421851}
Patch Set 1 #Patch Set 2 : Move compositor creation into display manager. Pull out BlimpDisplayManagerDelegateMain class from … #
Total comments: 22
Patch Set 3 : Code review changes #
Total comments: 2
Patch Set 4 : Fix more nits #
Total comments: 37
Patch Set 5 : More code review changes #
Total comments: 15
Patch Set 6 : More nits. Refactored display manager's ctor. #Patch Set 7 : Fix dependency issue found by gn check #Messages
Total messages: 31 (13 generated)
dtrainor@chromium.org changed reviewers: + dtrainor@chromium.org
https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:21: const Assignment& assignment) {} Should this log some information about the connection attempt to the console? Same with OnAuthenticationError(). https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:37: contents_ = std::move(contents); Put this in the inline setters: contents_(std::move(contents)), https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:49: contents_->GetView()->SetSizeAndScale(platform_window_->GetBounds().size(), Actually, would it be cleaner to call OnBoundsChanged(platform_window_->GetBounds()) to set this on the right things? Makes it so we don't need to have the same code twice. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:53: context->SetDelegate(new blimp::client::BlimpClientContextDelegateLinux()); This will leak the delegate. It isn't owned by BlimpClientContext. Use a std::unique_ptr and pass .get(). Make sure the destruction order is correct (destroy BlimpClientContext before BlimpClientContextDelegate). https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:76: std::unique_ptr<base::Thread> CreateIOThread() { Put this up in the anonymous namespace block above or inline it if it's only called once. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:10: namespace { I'd put this inside the client{} namespace also (or outside both). Just easier to read. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: constexpr char kDummyAccountId[] = ""; Should we give this a value? Is "" anything special (not sure, just curious!)? https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:18: BlimpDefaultIdentityProvider::~BlimpDefaultIdentityProvider() {} = default. Same for constructor if C++ allows it :). https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.h (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.h:17: class BlimpDefaultIdentityProvider : public IdentityProvider { Add a comment describing what this class does (and doesn't) do wrt IdentityProvider (e.g. doesn't really log in, the token service will always be null, etc.) https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.h:22: // IdentityProvider: IdentityProvider implementation.
PTAL https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:21: const Assignment& assignment) {} On 2016/09/26 20:54:26, David Trainor wrote: > Should this log some information about the connection attempt to the console? > Same with OnAuthenticationError(). Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:37: contents_ = std::move(contents); On 2016/09/26 20:54:26, David Trainor wrote: > Put this in the inline setters: > > contents_(std::move(contents)), Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:49: contents_->GetView()->SetSizeAndScale(platform_window_->GetBounds().size(), On 2016/09/26 20:54:26, David Trainor wrote: > Actually, would it be cleaner to call > OnBoundsChanged(platform_window_->GetBounds()) to set this on the right things? > Makes it so we don't need to have the same code twice. Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:53: context->SetDelegate(new blimp::client::BlimpClientContextDelegateLinux()); On 2016/09/26 20:54:26, David Trainor wrote: > This will leak the delegate. It isn't owned by BlimpClientContext. Use a > std::unique_ptr and pass .get(). Make sure the destruction order is correct > (destroy BlimpClientContext before BlimpClientContextDelegate). Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:76: std::unique_ptr<base::Thread> CreateIOThread() { On 2016/09/26 20:54:26, David Trainor wrote: > Put this up in the anonymous namespace block above or inline it if it's only > called once. Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:10: namespace { On 2016/09/26 20:54:26, David Trainor wrote: > I'd put this inside the client{} namespace also (or outside both). Just easier > to read. Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: constexpr char kDummyAccountId[] = ""; On 2016/09/26 20:54:26, David Trainor wrote: > Should we give this a value? Is "" anything special (not sure, just curious!)? "" is nothing special. This isn't ever looked at as long as you're using command-line switches to connect (and as long as we're using a null token service, I believe you need to use command-line switches). I can throw a random string in there if you'd like https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:18: BlimpDefaultIdentityProvider::~BlimpDefaultIdentityProvider() {} On 2016/09/26 20:54:26, David Trainor wrote: > = default. Same for constructor if C++ allows it :). Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.h (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.h:17: class BlimpDefaultIdentityProvider : public IdentityProvider { On 2016/09/26 20:54:26, David Trainor wrote: > Add a comment describing what this class does (and doesn't) do wrt > IdentityProvider (e.g. doesn't really log in, the token service will always be > null, etc.) Done. https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.h:22: // IdentityProvider: On 2016/09/26 20:54:26, David Trainor wrote: > IdentityProvider implementation. Done.
last few nits https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: constexpr char kDummyAccountId[] = ""; On 2016/09/27 17:35:01, steimel wrote: > On 2016/09/26 20:54:26, David Trainor wrote: > > Should we give this a value? Is "" anything special (not sure, just > curious!)? > > "" is nothing special. This isn't ever looked at as long as you're using > command-line switches to connect (and as long as we're using a null token > service, I believe you need to use command-line switches). I can throw a random > string in there if you'd like I think this is fine as long as the IdentityProvider base class doesn't use "" for anything special/error checking. But you're right this class won't really do anything anyway. I trust your judgement on this :). https://codereview.chromium.org/2363153002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:45: blimp::client::CompositorDependencies* compositor_dependencies = Make this a std::unique_ptr<> and use MakeUnique. Then give ownership to the context instead of doing "new" here. Sorry I missed this earlier!
PTAL https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/20001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: constexpr char kDummyAccountId[] = ""; On 2016/09/27 17:40:37, David Trainor wrote: > On 2016/09/27 17:35:01, steimel wrote: > > On 2016/09/26 20:54:26, David Trainor wrote: > > > Should we give this a value? Is "" anything special (not sure, just > > curious!)? > > > > "" is nothing special. This isn't ever looked at as long as you're using > > command-line switches to connect (and as long as we're using a null token > > service, I believe you need to use command-line switches). I can throw a > random > > string in there if you'd like > > I think this is fine as long as the IdentityProvider base class doesn't use "" > for anything special/error checking. But you're right this class won't really > do anything anyway. I trust your judgement on this :). Looks like IdentitySource uses a check to see if an account id is empty, so I've added an actual string in there https://codereview.chromium.org/2363153002/diff/40001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/40001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:45: blimp::client::CompositorDependencies* compositor_dependencies = On 2016/09/27 17:40:37, David Trainor wrote: > Make this a std::unique_ptr<> and use MakeUnique. Then give ownership to the > context instead of doing "new" here. Sorry I missed this earlier! Discussed with you and added a comment describing why it's done this way
looks good! lgtm on the code, waiting on tests
Discussed with dtrainor offline and we decided to open a new issue regarding testing for the Linux client instead of addressing it all in this CL. Issue #: 650839
The CQ bit was checked by steimel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kmarshall@chromium.org changed reviewers: + kmarshall@chromium.org
Mind if I add myself to the review? I'll uncheck the CQ bit.
The CQ bit was unchecked by kmarshall@chromium.org
https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:24: VLOG(0) << "Assignment connection attempt result unknown"; "Connection" isn't a good term here and elsewhere. Assignments can fail for reasons other than connections. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:24: VLOG(0) << "Assignment connection attempt result unknown"; These should probably be LOG(INFO|ERROR) or DLOG(), not VLOG, so that users don't have to explicitly opt-in to seeing these (important) logging lines. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:26: case AssignmentRequestResult::ASSIGNMENT_REQUEST_RESULT_OK: Move success case to the top. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:29: default: Don't use "default" if you can avoid it - this lets the static enum coverage checks do their thing. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:44: VLOG(0) << "Error: Not signed in"; LOG(ERROR) https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.h (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.h:13: class BlimpContents; add newline https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:40: platform_window_->SetBounds(gfx::Rect(window_size)); This is a strange ctor side effect - any reason why the caller can't set the size? Also, this ctor looks like it's doing more heavy lifting than just initialization. Please look at this style rule: https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.h (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:35: class BlimpDisplayManager : public ui::PlatformWindowDelegate { Can we add a descriptive comment for this class? https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:37: BlimpDisplayManager(const gfx::Size& window_size, Comment on ctor parameters https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:43: // ui::PlatformWindowDelegate: "... implementation." https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager_delegate_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager_delegate_main.cc:5: #include "base/run_loop.h" Not used? Also: the .h counterpart of the .cc always goes at the top. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:27: const int kWindowWidth = 800; constexpr https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:45: // Creating this using "new" and passing to context using "WrapUnique" as Move this declaration directly above the one for |context| to minimize the distance between the place where it's allocated and the place where its lifetime is attached. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:51: std::unique_ptr<blimp::client::BlimpClientContextDelegate> context_delegate = Put this just before SetDelegate, in the same semantic block. General comment: try to cluster variable declarations as close to where they're being used (so long as it still makes sense). Use blank lines to separate discrete clusters. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:64: std::unique_ptr<blimp::client::BlimpContents> contents = Move this right before GetNavigationController. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:73: std::unique_ptr<ui::PlatformEventSource> event_source = Is this used? https://codereview.chromium.org/2363153002/diff/60001/blimp/client/support/se... File blimp/client/support/session/BUILD.gn (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/support/se... blimp/client/support/session/BUILD.gn:10: source_set("session") { Any reason why we can't put this under client/support ? Why do we need this dir; do we have other plans for it?
PTAL https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:24: VLOG(0) << "Assignment connection attempt result unknown"; On 2016/09/27 21:59:40, Kevin M wrote: > These should probably be LOG(INFO|ERROR) or DLOG(), not VLOG, so that users > don't have to explicitly opt-in to seeing these (important) logging lines. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:26: case AssignmentRequestResult::ASSIGNMENT_REQUEST_RESULT_OK: On 2016/09/27 21:59:40, Kevin M wrote: > Move success case to the top. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:29: default: On 2016/09/27 21:59:40, Kevin M wrote: > Don't use "default" if you can avoid it - this lets the static enum coverage > checks do their thing. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:44: VLOG(0) << "Error: Not signed in"; On 2016/09/27 21:59:41, Kevin M wrote: > LOG(ERROR) Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.h (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.h:13: class BlimpContents; On 2016/09/27 21:59:41, Kevin M wrote: > add newline Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:40: platform_window_->SetBounds(gfx::Rect(window_size)); On 2016/09/27 21:59:41, Kevin M wrote: > This is a strange ctor side effect - any reason why the caller can't set the > size? > > Also, this ctor looks like it's doing more heavy lifting than just > initialization. Please look at this style rule: > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors As part of the initialization of the display manager, it needs to create and initialize its X11 platform window. The three parts of this are creating the new window, setting its size to the size provided by the caller as the first argument, and making it visible. Perhaps we could move some/all of this initialization to a separate method for the caller to call (e.g. display_manager.Start/ShowWindow/SetupWindow/Whatever), but I'm not entirely sure where you'd like that split to occur https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.h (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:35: class BlimpDisplayManager : public ui::PlatformWindowDelegate { On 2016/09/27 21:59:41, Kevin M wrote: > Can we add a descriptive comment for this class? Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:37: BlimpDisplayManager(const gfx::Size& window_size, On 2016/09/27 21:59:41, Kevin M wrote: > Comment on ctor parameters Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:43: // ui::PlatformWindowDelegate: On 2016/09/27 21:59:41, Kevin M wrote: > "... implementation." Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager_delegate_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager_delegate_main.cc:5: #include "base/run_loop.h" On 2016/09/27 21:59:41, Kevin M wrote: > Not used? Also: the .h counterpart of the .cc always goes at the top. Moved below the .h counterpart. Used on line 12 https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:27: const int kWindowWidth = 800; On 2016/09/27 21:59:41, Kevin M wrote: > constexpr Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:45: // Creating this using "new" and passing to context using "WrapUnique" as On 2016/09/27 21:59:41, Kevin M wrote: > Move this declaration directly above the one for |context| to minimize the > distance between the place where it's allocated and the place where its lifetime > is attached. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:51: std::unique_ptr<blimp::client::BlimpClientContextDelegate> context_delegate = On 2016/09/27 21:59:41, Kevin M wrote: > Put this just before SetDelegate, in the same semantic block. > > General comment: try to cluster variable declarations as close to where they're > being used (so long as it still makes sense). > > Use blank lines to separate discrete clusters. Done. It's weird, but I could have sworn I had already done that. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:64: std::unique_ptr<blimp::client::BlimpContents> contents = On 2016/09/27 21:59:41, Kevin M wrote: > Move this right before GetNavigationController. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:73: std::unique_ptr<ui::PlatformEventSource> event_source = On 2016/09/27 21:59:41, Kevin M wrote: > Is this used? Good catch. I had meant to move this into the display manager, but it had slipped through. You cannot create a new instance of an X11 window unless there exists a PlatformEventSource. By creating one, it is set as static on the PlatformEventSource class and used (that's why it currently works even though it's within blimp_main). I've now moved this to be created and held entirely within the display manager, since that's the object that actually cares about it. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/support/se... File blimp/client/support/session/BUILD.gn (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/support/se... blimp/client/support/session/BUILD.gn:10: source_set("session") { On 2016/09/27 21:59:41, Kevin M wrote: > Any reason why we can't put this under client/support ? Why do we need this dir; > do we have other plans for it? For that, you'd have to ask dtrainor. He specified that I should create this particular directory for this file.
lgtm https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:30: LOG(WARNING) << "Assignment request error: Bad request"; Recommend adding this code to string table to the Assignment library. So you'd have const char* AssignmentRequestResultToString(result) { switch { case BLAHBLAH_UNKNOWN: return "UNKNOWN"; case BLAHBLAH_EXPIRED_ACCESS_TOKEN: return "EXPIRED_ACCESS_TOKEN"; ... } } Then you would just call that function here: LOG(WARNING) << "Assignment request error: " << AssignmentRequestResultToString(result); and remove the switch case from this function. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.h (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:40: BlimpDisplayManager( Document the parameters before the constructor declaration vs. doing them inline. // |window_size|: Size of the X11 window // |delegate|: The delegate to receive the // OnClosed call. // ... BlimpDisplayManager(...); https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:19: #include "blimp/client/support/compositor/blimp_embedder_compositor.h" Is this used? Can you sift through the other dependencies and see what you can eliminate? https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:21: #include "blimp/common/get_client_auth_token.h" Not used? https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:25: constexpr char kDefaultUrl[] = "https://www.google.com"; Nit: this can be const char* - it's advantageous to store char buffers only once in the binary. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: remove blank line
Also, please add more detail to the CL description, and try not to reference team-specific milestones (e.g. 0.6) in them. Instead, use plain text descriptions. You can look at Wez's CL history to get an idea of the level of specificity we're striving for. "git log --author=wez"
https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:40: platform_window_->SetBounds(gfx::Rect(window_size)); On 2016/09/27 21:59:41, Kevin M wrote: > This is a strange ctor side effect - any reason why the caller can't set the > size? > > Also, this ctor looks like it's doing more heavy lifting than just > initialization. Please look at this style rule: > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors Suggested split: Pull out a SetWindowSize() and SetBlimpContents() on BlimpDisplayManager. Just call those two after initializing the object. Check that calling platform_window_->SetBounds() properly triggers OnBoundsChanged() to the delegate. If so you don't have to call OnBoundsChanged in the constructor (or anywhere else explicitly). The only annoying thing is you'll have to DCHECK or null check use of contents_ everywhere else in the class. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:51: std::unique_ptr<blimp::client::BlimpClientContextDelegate> context_delegate = On 2016/09/27 23:23:19, steimel wrote: > On 2016/09/27 21:59:41, Kevin M wrote: > > Put this just before SetDelegate, in the same semantic block. > > > > General comment: try to cluster variable declarations as close to where > they're > > being used (so long as it still makes sense). > > > > Use blank lines to separate discrete clusters. > > Done. It's weird, but I could have sworn I had already done that. I would undo this. You don't want the delegate dying before the BlimpClientContext. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:30: LOG(WARNING) << "Assignment request error: Bad request"; On 2016/09/28 01:35:46, Kevin M wrote: > Recommend adding this code to string table to the Assignment library. So you'd > have > > const char* AssignmentRequestResultToString(result) { > switch { > case BLAHBLAH_UNKNOWN: > return "UNKNOWN"; > case BLAHBLAH_EXPIRED_ACCESS_TOKEN: > return "EXPIRED_ACCESS_TOKEN"; > ... > } > } > > Then you would just call that function here: > > LOG(WARNING) << "Assignment request error: " << > AssignmentRequestResultToString(result); > > and remove the switch case from this function. This is happening at a slightly higher level in xingliu@'s CL, where we need to clean up the strings to be presented to the user. It won't be this exact error code but it'll be a user-friendly version of it. We could use that instead if it makes more sense. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:50: BlimpDisplayManager::~BlimpDisplayManager() {} = default
Description was changed from ========== Migrate Linux Blimp client to 0.6 api BUG=645202 ========== to ========== Migrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext. For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature). blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread) BUG=645202 ==========
PTAL https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:40: platform_window_->SetBounds(gfx::Rect(window_size)); On 2016/09/28 16:34:01, David Trainor wrote: > On 2016/09/27 21:59:41, Kevin M wrote: > > This is a strange ctor side effect - any reason why the caller can't set the > > size? > > > > Also, this ctor looks like it's doing more heavy lifting than just > > initialization. Please look at this style rule: > > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors > > Suggested split: > > Pull out a SetWindowSize() and SetBlimpContents() on BlimpDisplayManager. Just > call those two after initializing the object. Check that calling > platform_window_->SetBounds() properly triggers OnBoundsChanged() to the > delegate. If so you don't have to call OnBoundsChanged in the constructor (or > anywhere else explicitly). > > The only annoying thing is you'll have to DCHECK or null check use of contents_ > everywhere else in the class. Done. https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/60001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:51: std::unique_ptr<blimp::client::BlimpClientContextDelegate> context_delegate = On 2016/09/28 16:34:01, David Trainor wrote: > On 2016/09/27 23:23:19, steimel wrote: > > On 2016/09/27 21:59:41, Kevin M wrote: > > > Put this just before SetDelegate, in the same semantic block. > > > > > > General comment: try to cluster variable declarations as close to where > > they're > > > being used (so long as it still makes sense). > > > > > > Use blank lines to separate discrete clusters. > > > > Done. It's weird, but I could have sworn I had already done that. > > I would undo this. You don't want the delegate dying before the > BlimpClientContext. Done and added comment to explain https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_client_context_delegate_linux.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_client_context_delegate_linux.cc:30: LOG(WARNING) << "Assignment request error: Bad request"; On 2016/09/28 16:34:01, David Trainor wrote: > On 2016/09/28 01:35:46, Kevin M wrote: > > Recommend adding this code to string table to the Assignment library. So you'd > > have > > > > const char* AssignmentRequestResultToString(result) { > > switch { > > case BLAHBLAH_UNKNOWN: > > return "UNKNOWN"; > > case BLAHBLAH_EXPIRED_ACCESS_TOKEN: > > return "EXPIRED_ACCESS_TOKEN"; > > ... > > } > > } > > > > Then you would just call that function here: > > > > LOG(WARNING) << "Assignment request error: " << > > AssignmentRequestResultToString(result); > > > > and remove the switch case from this function. > > This is happening at a slightly higher level in xingliu@'s CL, where we need to > clean up the strings to be presented to the user. It won't be this exact error > code but it'll be a user-friendly version of it. We could use that instead if > it makes more sense. Added TODO for xingliu@ to update to use new error strings. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.cc:50: BlimpDisplayManager::~BlimpDisplayManager() {} On 2016/09/28 16:34:01, David Trainor wrote: > = default Done. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_display_manager.h (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_display_manager.h:40: BlimpDisplayManager( On 2016/09/28 01:35:46, Kevin M wrote: > Document the parameters before the constructor declaration vs. doing them > inline. > > // |window_size|: Size of the X11 window > // |delegate|: The delegate to receive the > // OnClosed call. > // ... > BlimpDisplayManager(...); Done. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... File blimp/client/app/linux/blimp_main.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:19: #include "blimp/client/support/compositor/blimp_embedder_compositor.h" On 2016/09/28 01:35:46, Kevin M wrote: > Is this used? Can you sift through the other dependencies and see what you can > eliminate? Done. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:21: #include "blimp/common/get_client_auth_token.h" On 2016/09/28 01:35:47, Kevin M wrote: > Not used? Done. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/app/linux/... blimp/client/app/linux/blimp_main.cc:25: constexpr char kDefaultUrl[] = "https://www.google.com"; On 2016/09/28 01:35:46, Kevin M wrote: > Nit: this can be const char* - it's advantageous to store char buffers only once > in the binary. Done. https://codereview.chromium.org/2363153002/diff/80001/blimp/client/support/se... File blimp/client/support/session/blimp_default_identity_provider.cc (right): https://codereview.chromium.org/2363153002/diff/80001/blimp/client/support/se... blimp/client/support/session/blimp_default_identity_provider.cc:11: On 2016/09/28 01:35:47, Kevin M wrote: > remove blank line Done.
The CQ bit was checked by nyquist@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm thanks!
The CQ bit was checked by steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2363153002/#ps120001 (title: "Fix dependency issue found by gn check")
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.
Description was changed from ========== Migrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext. For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature). blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread) BUG=645202 ========== to ========== Migrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext. For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature). blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread) BUG=645202 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Migrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext. For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature). blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread) BUG=645202 ========== to ========== Migrating the Linux Blimp client away from using the deprecated BlimpClientSession and towards using BlimpClientContext. For this CL, we now create a BlimpClientContext and create a BlimpContents from that to send to the display manager, which replaces the individual features being sent before (e.g. TabControlFeature). blimp_main now handles some things that were being handled in the BlimpClientSession (e.g. creating a display manager and an IO thread) BUG=645202 Committed: https://crrev.com/d890645241cfa41fe40e22a5cdf5cd9d6cef4f01 Cr-Commit-Position: refs/heads/master@{#421851} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d890645241cfa41fe40e22a5cdf5cd9d6cef4f01 Cr-Commit-Position: refs/heads/master@{#421851} |
