Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035543002:
[Blimp] Creates engine tab class to handle tab related operations. (Closed)
|
Created:
4 years, 6 months ago by haibinlu Modified:
4 years, 6 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+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, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactoring. This is the first step towards multiple tab support in Engine.
BUG=547231
Committed: https://crrev.com/bea8a2012f97b8491c8895885a89cacf86aeb6bd
Cr-Commit-Position: refs/heads/master@{#398975}
Patch Set 1 #
Total comments: 40
Patch Set 2 : addresses comments #
Total comments: 18
Patch Set 3 : #
Total comments: 27
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Messages
Total messages: 30 (7 generated)
Description was changed from ========== Creates engine tab class to handle tab related operations. This is the first step towards mutliple tab support in Engine. BUG=547231 ========== to ========== Refactoring. This is the first step towards multiple tab support in Engine. BUG=547231 ==========
haibinlu@chromium.org changed reviewers: + kmarshall@chromium.org
Can you add some more meat to the CL description? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:237: // Ensure that all WebContents are torn down first, since teardown will Revise this comment? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:321: bool BlimpEngineSession::CreateWebContents(const int target_tab_id) { Why is this called CreateWebContents, when it creates a tab? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:356: tab_->LoadUrl(url); Won't this segfault if tab_ is closed (and nulled) beforehand? Ditto for all functions that reference tab_ that originate from network data. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:579: void BlimpEngineSession::PlatformSetContents( I don't see any other callers of this besides CreateWebContents. Can we inline it? This seems to be chopping up the execution flow arbitrarily. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:586: if (!parent->Contains(content)) The WebContents and its view are brand new, is it even possible that the parent contains it? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:114: void LoadUrl(const int target_tab_id, const GURL& url); Please factor these out into a separate Navigation feature. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:190: std::unique_ptr<EngineSettingsFeature> settings_feature_; Why is this a unique_ptr now? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:194: std::unique_ptr<EngineRenderWidgetFeature> render_widget_feature_; Ditto https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:40: // have client IO info. Can this be rephrased to be more clear? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:49: web_contents_->Close(); Is this redundant, given that web_contents_ is about to be destroyed? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:52: void TabBlimp::Resize(float device_pixel_ratio, const gfx::Size& size_in_dips) { DVLOG this event? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:92: void TabBlimp::RenderViewCreated(content::RenderViewHost* render_view_host) { DCHECK(rvh)? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:99: // Informs client that WebContents swaps its visible RenderViewHost with nit: since this is a one-liner method, this comment belongs in the .h instead. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:112: nit: remove newline to keep these as a single semantic chunk https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:126: NavigationStateChangeMessage* details = No need to assign this to a named variable; just inline this with the next line. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:23: // Owns WebContents, handles operations in the tab, and has one-to-one mapping This comment is kind of vague. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:25: class TabBlimp : public content::WebContentsObserver, How about just "Tab"? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:30: explicit TabBlimp(std::unique_ptr<content::WebContents> web_contents, What about having this class responsible for creating the WebContents? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:31: const int tab_id, nit: we generally don't const value parameters across the blimp/ codebase, so I recommend not using them occasionally. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:39: void Close(); Can you make these into NavigationFeature delegate methods so that we enforce interface symmetry across the navigable components? So you'd have something like this: NavigationFeature => TabNavigationDispatcher (implements NavigationFeature::Delegate) => TabBlimp (implements NavigationFeature::Delegate)
PTAL https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:237: // Ensure that all WebContents are torn down first, since teardown will On 2016/06/02 18:38:50, Kevin M wrote: > Revise this comment? Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:321: bool BlimpEngineSession::CreateWebContents(const int target_tab_id) { On 2016/06/02 18:38:50, Kevin M wrote: > Why is this called CreateWebContents, when it creates a tab? Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:356: tab_->LoadUrl(url); On 2016/06/02 18:38:50, Kevin M wrote: > Won't this segfault if tab_ is closed (and nulled) beforehand? Ditto for all > functions that reference tab_ that originate from network data. added check. we have been depending on client does not send in invalid command. But it's better to check first. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:579: void BlimpEngineSession::PlatformSetContents( On 2016/06/02 18:38:50, Kevin M wrote: > I don't see any other callers of this besides CreateWebContents. Can we inline > it? This seems to be chopping up the execution flow arbitrarily. AddNewContents will use it. But we have not implemented AddNewContents. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.cc:586: if (!parent->Contains(content)) On 2016/06/02 18:38:50, Kevin M wrote: > The WebContents and its view are brand new, is it even possible that the parent > contains it? that is the view hierarchy. WindowTreeHost is the root. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:114: void LoadUrl(const int target_tab_id, const GURL& url); On 2016/06/02 18:38:50, Kevin M wrote: > Please factor these out into a separate Navigation feature. Not sure if we need separate nav feature since it is very simple. If we have a Nav feature class, BlimpEngineSession would need to implement its delegate, and delegate methods are these methods. We can not delegate Nav feature to tab class directly because BlimpEngineSession need to find a tab first. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:190: std::unique_ptr<EngineSettingsFeature> settings_feature_; On 2016/06/02 18:38:50, Kevin M wrote: > Why is this a unique_ptr now? reverted. I was thinking about the flexibility of deleting it at any time rather than relying on the deconstruction order. It turns out there is no such need. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/blimp_... blimp/engine/session/blimp_engine_session.h:194: std::unique_ptr<EngineRenderWidgetFeature> render_widget_feature_; On 2016/06/02 18:38:50, Kevin M wrote: > Ditto reverted. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:40: // have client IO info. On 2016/06/02 18:38:50, Kevin M wrote: > Can this be rephrased to be more clear? Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:49: web_contents_->Close(); On 2016/06/02 18:38:50, Kevin M wrote: > Is this redundant, given that web_contents_ is about to be destroyed? right. we do not really need this Close method. removed. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:52: void TabBlimp::Resize(float device_pixel_ratio, const gfx::Size& size_in_dips) { On 2016/06/02 18:38:50, Kevin M wrote: > DVLOG this event? Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:92: void TabBlimp::RenderViewCreated(content::RenderViewHost* render_view_host) { On 2016/06/02 18:38:50, Kevin M wrote: > DCHECK(rvh)? Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:99: // Informs client that WebContents swaps its visible RenderViewHost with On 2016/06/02 18:38:50, Kevin M wrote: > nit: since this is a one-liner method, this comment belongs in the .h instead. removed comment. OnRenderWidgetInitialized already has enough comment there. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:112: On 2016/06/02 18:38:50, Kevin M wrote: > nit: remove newline to keep these as a single semantic chunk Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.cc:126: NavigationStateChangeMessage* details = On 2016/06/02 18:38:50, Kevin M wrote: > No need to assign this to a named variable; just inline this with the next line. Done. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:23: // Owns WebContents, handles operations in the tab, and has one-to-one mapping On 2016/06/02 18:38:50, Kevin M wrote: > This comment is kind of vague. any suggestion? https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:25: class TabBlimp : public content::WebContentsObserver, On 2016/06/02 18:38:50, Kevin M wrote: > How about just "Tab"? naming is similar to chrome/browser/android/tab_android.h I am ok with Tab or TabBlimp. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:30: explicit TabBlimp(std::unique_ptr<content::WebContents> web_contents, On 2016/06/02 18:38:50, Kevin M wrote: > What about having this class responsible for creating the WebContents? creating a webcontent needs a reference to browser_context. see BlimpEngineSession::CreateTab. We could have a static method here which takes browser_content as input. https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:31: const int tab_id, On 2016/06/02 18:38:51, Kevin M wrote: > nit: we generally don't const value parameters across the blimp/ codebase, so I > recommend not using them occasionally. Shall we use it consistently across blimp codebase? per c++ style guideline, "we strongly recommend that you use const whenever it makes sense to do so". https://codereview.chromium.org/2035543002/diff/1/blimp/engine/session/tab_bl... blimp/engine/session/tab_blimp.h:39: void Close(); On 2016/06/02 18:38:50, Kevin M wrote: > Can you make these into NavigationFeature delegate methods so that we enforce > interface symmetry across the navigable components? > > So you'd have something like this: > > NavigationFeature => TabNavigationDispatcher (implements > NavigationFeature::Delegate) => TabBlimp (implements > NavigationFeature::Delegate) NavFeature delegate will contain tab_id as its parameter, thus, TabBlimp here can not implement its delegate.
Can we pull the tab management "feature" handlers out of BlimpEngineSession into a separate feature class (e.g. TabManagementFeature)? Streamlining the client and engine session classeses is high-priority refactoring effort. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:355: if (tab_) I see that tab_ is already checked in the conditional that's executed beforehand, so this should only break if there is a regresssion somewhere. DCHECK(tab_)? https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:405: if (!tab_ || !tab_->web_contents()->GetRenderWidgetHostView()) This is pretty severe; should we silently drop this error? Ditto for other checks on tab_, tab_->web_contents() Either we should DCHECK it or crash/fail the connection, but we shouldn't silently drop the error. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:533: if (source == tab_->web_contents()) { Is it normal behavior for this to not be true? If not, can we DCHECK this? https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:109: void CloseTab(const int target_tab_id); Comment? https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:110: void HandleResize(float device_pixel_ratio, const gfx::Size& size); Comment? https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:113: // Navigates the target tab to the |url|. Comment doesn't apply equally across all four methods - add separate comments for those, or remove this comment. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:40: // Transfer over the user agent override. The default user agent does not Do we need lines 40 and 41 anymore? https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:29: // Caller ensures |render_widget_feature| and |navigation_message_sender| Document ctor parameters. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:62: // Tracks the page load status for a tab. Each PageLoadTracker is tied to a What does "tied to" mean in this case? Must be deleted before a WebContents? The second part of this comment isn't really relevant - this is implied by the Tab object owning both the tracker and the WebContents.
PTAL. per offline discussion, we will create TabManagementFeature for managing multiple tabs in separate CL. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:355: if (tab_) On 2016/06/02 21:54:05, Kevin M wrote: > I see that tab_ is already checked in the conditional that's executed > beforehand, so this should only break if there is a regresssion somewhere. > DCHECK(tab_)? removed check in ProcessMessage. When multiple tab is enabled, this will lookup tab first and check if tab is not null. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:405: if (!tab_ || !tab_->web_contents()->GetRenderWidgetHostView()) On 2016/06/02 21:54:05, Kevin M wrote: > This is pretty severe; should we silently drop this error? Ditto for other > checks on tab_, tab_->web_contents() > > Either we should DCHECK it or crash/fail the connection, but we shouldn't > silently drop the error. right. checking tab_ is not necessary. if it is null, it will crash. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:533: if (source == tab_->web_contents()) { On 2016/06/02 21:54:05, Kevin M wrote: > Is it normal behavior for this to not be true? If not, can we DCHECK this? it is possible, BlimpEngineSession (future TabManagementFeature) will be the delegate of all webcontents. thus, |source| varies. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:109: void CloseTab(const int target_tab_id); On 2016/06/02 21:54:05, Kevin M wrote: > Comment? Done. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:110: void HandleResize(float device_pixel_ratio, const gfx::Size& size); On 2016/06/02 21:54:05, Kevin M wrote: > Comment? Done. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:113: // Navigates the target tab to the |url|. On 2016/06/02 21:54:05, Kevin M wrote: > Comment doesn't apply equally across all four methods - add separate comments > for those, or remove this comment. Done. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:40: // Transfer over the user agent override. The default user agent does not On 2016/06/02 21:54:05, Kevin M wrote: > Do we need lines 40 and 41 anymore? Done. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:29: // Caller ensures |render_widget_feature| and |navigation_message_sender| On 2016/06/02 21:54:05, Kevin M wrote: > Document ctor parameters. Done. https://codereview.chromium.org/2035543002/diff/20001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:62: // Tracks the page load status for a tab. Each PageLoadTracker is tied to a On 2016/06/02 21:54:05, Kevin M wrote: > What does "tied to" mean in this case? Must be deleted before a WebContents? > > The second part of this comment isn't really relevant - this is implied by the > Tab object owning both the tracker and the WebContents. Updated comments. page_load_tracker_ takes webcontents as parameter. thus it is constructed after.
https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:237: // Ensure that all tabs are torn down first, since teardown will Not needed; placing the tab at the end of the field list ensures that it will be torn down first. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:323: // TODO(haibinlu): Support more than one tab (crbug/547231). We specify this issue multiple times, can we just use the .h comment for that? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:325: DLOG(WARNING) << "Tab " << target_tab_id << " already existed"; This warning doesn't apply - the issue is the pre-existence of *any* tab. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:356: tab_->LoadUrl(url); Why are we silently failing here? This is invalid input; we should drop the connection and log an error code. Ditto for this and other navigation commands https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:464: } else if (message->has_navigation()) { Check if the tab_ exists here? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:484: DVLOG(1) << "No tab for navigation control"; This error seems N/A for the current logic https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:558: void BlimpEngineSession::PlatformSetContents( What's the reason for having this be separate from CreateTab() ? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:210: // Only one tab is supported for blimp 0.5. Reference a bug? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/pa... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can we fold PageLoadTracker into the new Tab class, now? The decoupling of the functionality seems like unnecessary overhead. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:110: if (!changed_flags) Should this ever happen? Shall we DCHECK it? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:139: bool page_load_completed = false; Replace switch with bool page_load_completed = load_status == PageLoadStatus::LOADED; https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:26: class TabBlimp : public content::WebContentsObserver, "Tab" ? https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:35: explicit TabBlimp(std::unique_ptr<content::WebContents> web_contents, "explicit" not needed for this many ctor arguments.
https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:210: // Only one tab is supported for blimp 0.5. On 2016/06/06 21:44:38, Kevin M wrote: > Reference a bug? Also, we should probably not specify release milestones in code comments.
On 2016/06/06 21:54:58, Kevin M wrote: > https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.h (right): > > https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.h:210: // Only one tab is supported > for blimp 0.5. > On 2016/06/06 21:44:38, Kevin M wrote: > > Reference a bug? > > Also, we should probably not specify release milestones in code comments. Done
On 2016/06/06 21:54:58, Kevin M wrote: > https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.h (right): > > https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.h:210: // Only one tab is supported > for blimp 0.5. > On 2016/06/06 21:44:38, Kevin M wrote: > > Reference a bug? > > Also, we should probably not specify release milestones in code comments. Done
PTAL https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:237: // Ensure that all tabs are torn down first, since teardown will On 2016/06/06 21:44:38, Kevin M wrote: > Not needed; placing the tab at the end of the field list ensures that it will be > torn down first. Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:323: // TODO(haibinlu): Support more than one tab (crbug/547231). On 2016/06/06 21:44:38, Kevin M wrote: > We specify this issue multiple times, can we just use the .h comment for that? Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:325: DLOG(WARNING) << "Tab " << target_tab_id << " already existed"; On 2016/06/06 21:44:38, Kevin M wrote: > This warning doesn't apply - the issue is the pre-existence of *any* tab. Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:356: tab_->LoadUrl(url); On 2016/06/06 21:44:38, Kevin M wrote: > Why are we silently failing here? This is invalid input; we should drop the > connection and log an error code. Ditto for this and other navigation commands log a info instead. I argue we should not drop the connection. Due to the latency, it is possible that when load-url command from client arrives, a tab has been closed by the engine. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:464: } else if (message->has_navigation()) { On 2016/06/06 21:44:38, Kevin M wrote: > Check if the tab_ exists here? Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:484: DVLOG(1) << "No tab for navigation control"; On 2016/06/06 21:44:38, Kevin M wrote: > This error seems N/A for the current logic Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:558: void BlimpEngineSession::PlatformSetContents( On 2016/06/06 21:44:38, Kevin M wrote: > What's the reason for having this be separate from CreateTab() ? When BlimpEngineSession::AddNewContents is implemented, it will not create a new webcontents (just use new_contents) but will invoke this method to set it up. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:210: // Only one tab is supported for blimp 0.5. On 2016/06/06 21:44:38, Kevin M wrote: > Reference a bug? Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/pa... File blimp/engine/session/page_load_tracker.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/pa... blimp/engine/session/page_load_tracker.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/06 21:44:38, Kevin M wrote: > Can we fold PageLoadTracker into the new Tab class, now? The decoupling of the > functionality seems like unnecessary overhead. Created crbug/617783. I think it is fine to have this tracker as a separate class. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.cc (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:110: if (!changed_flags) On 2016/06/06 21:44:39, Kevin M wrote: > Should this ever happen? Shall we DCHECK it? InvalidateTypes has no type at value 0. but chrome/browser/ui Browser::NavigationStateChanged also does "if (changed_flags)". Added DCHECK. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.cc:139: bool page_load_completed = false; On 2016/06/06 21:44:38, Kevin M wrote: > Replace switch with > > bool page_load_completed = load_status == PageLoadStatus::LOADED; Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab_blimp.h (right): https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:26: class TabBlimp : public content::WebContentsObserver, On 2016/06/06 21:44:39, Kevin M wrote: > "Tab" ? Done. https://codereview.chromium.org/2035543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab_blimp.h:35: explicit TabBlimp(std::unique_ptr<content::WebContents> web_contents, On 2016/06/06 21:44:39, Kevin M wrote: > "explicit" not needed for this many ctor arguments. Done.
https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: const ui::TextInputClient* client) { Also check that tab_ exists https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:434: default: Replace default with TAB_CONTROL_NOT_SET https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:453: default: Remove "default" for compile time enum coverage checks https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:509: void BlimpEngineSession::CloseContents(content::WebContents* source) { Is it possible to have this closed during tab deletion? So: tab_ reset() => tab object destroyed => web contents destroyed => CloseContents observer method invoked => tab dereferenced => segfault https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:155: // Sets up and owns |new_contents|. Method name and comment seem outdated. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Unit tests? :)
PTAL https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:378: const ui::TextInputClient* client) { On 2016/06/07 22:43:42, Kevin M wrote: > Also check that tab_ exists Done. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:434: default: On 2016/06/07 22:43:42, Kevin M wrote: > Replace default with TAB_CONTROL_NOT_SET good idea, but tab control message can be client <==> engine, client ==> engine, or engine ==> client. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:453: default: On 2016/06/07 22:43:42, Kevin M wrote: > Remove "default" for compile time enum coverage checks ditto. navigation message type is not client==>engine only. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:509: void BlimpEngineSession::CloseContents(content::WebContents* source) { On 2016/06/07 22:43:42, Kevin M wrote: > Is it possible to have this closed during tab deletion? So: > > tab_ reset() => tab object destroyed => web contents destroyed => CloseContents > observer method invoked => tab dereferenced => segfault I donot think the case is possible. CloseContents is part of content::WebContentsDelegate. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:155: // Sets up and owns |new_contents|. On 2016/06/07 22:43:42, Kevin M wrote: > Method name and comment seem outdated. Updated comment. The method name is fine. https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/07 22:43:43, Kevin M wrote: > Unit tests? :) per offline discussion, we should add some browser test cases for tab controls instead.
maniscalco@chromium.org changed reviewers: + maniscalco@chromium.org
https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/08 17:57:22, haibinlu wrote: > On 2016/06/07 22:43:43, Kevin M wrote: > > Unit tests? :) > > per offline discussion, we should add some browser test cases for tab controls > instead. Not that I necessarily disagree, but could you elaborate on why we're not adding unit test for this new class?
On 2016/06/08 19:59:39, maniscalco wrote: > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h > File blimp/engine/session/tab.h (right): > > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... > blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All rights > reserved. > On 2016/06/08 17:57:22, haibinlu wrote: > > On 2016/06/07 22:43:43, Kevin M wrote: > > > Unit tests? :) > > > > per offline discussion, we should add some browser test cases for tab controls > > instead. > > Not that I necessarily disagree, but could you elaborate on why we're not adding > unit test for this new class? We were discussing browser test cases vs. unit test cases for this class. Most methods of this class are simple enough; unit test cases do not verify much but require quite a few mocks and setup; browser test case is much better, e.g., verifying go_back/forward actually work, we want these browser test cases anyway.
On 2016/06/08 20:09:18, haibinlu wrote: > On 2016/06/08 19:59:39, maniscalco wrote: > > > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h > > File blimp/engine/session/tab.h (right): > > > > > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... > > blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All > rights > > reserved. > > On 2016/06/08 17:57:22, haibinlu wrote: > > > On 2016/06/07 22:43:43, Kevin M wrote: > > > > Unit tests? :) > > > > > > per offline discussion, we should add some browser test cases for tab > controls > > > instead. > > > > Not that I necessarily disagree, but could you elaborate on why we're not > adding > > unit test for this new class? > > We were discussing browser test cases vs. unit test cases for this class. Most > methods of this class are simple enough; unit test cases do not verify much but > require quite a few mocks and setup; browser test case is much better, e.g., > verifying go_back/forward actually work, we want these browser test cases > anyway. created crbug.com/618436 to track test effort.
On 2016/06/08 20:22:46, haibinlu wrote: > On 2016/06/08 20:09:18, haibinlu wrote: > > On 2016/06/08 19:59:39, maniscalco wrote: > > > > > > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/tab.h > > > File blimp/engine/session/tab.h (right): > > > > > > > > > https://codereview.chromium.org/2035543002/diff/60001/blimp/engine/session/ta... > > > blimp/engine/session/tab.h:1: // Copyright 2016 The Chromium Authors. All > > rights > > > reserved. > > > On 2016/06/08 17:57:22, haibinlu wrote: > > > > On 2016/06/07 22:43:43, Kevin M wrote: > > > > > Unit tests? :) > > > > > > > > per offline discussion, we should add some browser test cases for tab > > controls > > > > instead. > > > > > > Not that I necessarily disagree, but could you elaborate on why we're not > > adding > > > unit test for this new class? > > > > We were discussing browser test cases vs. unit test cases for this class. Most > > methods of this class are simple enough; unit test cases do not verify much > but > > require quite a few mocks and setup; browser test case is much better, e.g., > > verifying go_back/forward actually work, we want these browser test cases > > anyway. > > created crbug.com/618436 to track test effort. Thanks for creating the bug! I'm fine with tests being added in a future CL (assuming Kevin agrees). I'll comment in that bug about the test strategy.
https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.cc:143: ->set_page_load_completed(load_status == PageLoadStatus::LOADED); Dumb question: how is "page_load_completed_load_status" different than "loading" ? https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:18: namespace blimp { add newline https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:21: namespace engine { add newline https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:24: // Owns WebContents, handles operations in the tab, and has one-to-one mapping Clarify what "operations" means. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:43: void Resize(float device_pixel_ratio, const gfx::Size& size_in_dips); Comment on the function of these methods. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:66: std::unique_ptr<PageLoadTracker> page_load_tracker_; Make this a regular field instead of a unique_ptr?
lgtm % nits
https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.cc:143: ->set_page_load_completed(load_status == PageLoadStatus::LOADED); On 2016/06/09 17:27:23, Kevin M wrote: > Dumb question: how is "page_load_completed_load_status" different than "loading" > ? See comments in navigation.proto https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... File blimp/engine/session/tab.h (right): https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:18: namespace blimp { On 2016/06/09 17:27:23, Kevin M wrote: > add newline Done. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:21: namespace engine { On 2016/06/09 17:27:23, Kevin M wrote: > add newline Done. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:24: // Owns WebContents, handles operations in the tab, and has one-to-one mapping On 2016/06/09 17:27:23, Kevin M wrote: > Clarify what "operations" means. Done. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:43: void Resize(float device_pixel_ratio, const gfx::Size& size_in_dips); On 2016/06/09 17:27:23, Kevin M wrote: > Comment on the function of these methods. Done. https://codereview.chromium.org/2035543002/diff/100001/blimp/engine/session/t... blimp/engine/session/tab.h:66: std::unique_ptr<PageLoadTracker> page_load_tracker_; On 2016/06/09 17:27:23, Kevin M wrote: > Make this a regular field instead of a unique_ptr? Done.
The CQ bit was checked by haibinlu@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/2035543002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035543002/120001
Message was sent while issue was closed.
Description was changed from ========== Refactoring. This is the first step towards multiple tab support in Engine. BUG=547231 ========== to ========== Refactoring. This is the first step towards multiple tab support in Engine. BUG=547231 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Refactoring. This is the first step towards multiple tab support in Engine. BUG=547231 ========== to ========== Refactoring. This is the first step towards multiple tab support in Engine. BUG=547231 Committed: https://crrev.com/bea8a2012f97b8491c8895885a89cacf86aeb6bd Cr-Commit-Position: refs/heads/master@{#398975} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bea8a2012f97b8491c8895885a89cacf86aeb6bd Cr-Commit-Position: refs/heads/master@{#398975} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
