Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2394663002:
Blimp: Propagating page loading status to tab (Closed)
|
Created:
4 years, 2 months ago by shaktisahu Modified:
4 years, 2 months ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: Propagating page loading status to tab
This CL adds support on the Blimp to propagate loading status and page
load status to Tab. This enables the UI to display the loading spinner
on the tablets and page load progress bar on phones.
The page load progress was set to 50 percent for now.
BUG=651473
Committed: https://crrev.com/8975b80a50bb91c4723c8fc29a871a940b7fdb2f
Cr-Commit-Position: refs/heads/master@{#426827}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Fixed progress bar to go upto 100 #
Total comments: 8
Patch Set 3 : Removed unused params #
Total comments: 6
Patch Set 4 : dtrainor@ comments #Messages
Total messages: 25 (6 generated)
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org
https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); Shout out to david for the same thing. Nice to have comments on why it makes sense for us to signal this to the Tab at this point. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( I'm super scared of doing this. Ideally, the Chrome layer shouldn't have to care about these values. If they are, then we need those call sites to handle the blimp case separately. Doing a quick look, these values don't seem to be used anywhere in Chrome. Would it be possible to remove them from the observer interface instead? https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:61: mTab.didFinishPageLoad(); Why is this here and now in onLoadingStateChanged? Are you trying to match this with how events are dispatched by the content layer? It would be nice to have comments on what events this is trying to match with respect to the content layer and how it makes sense for us to dispatch them at this point instead.
https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( On 2016/10/06 03:13:27, Khushal wrote: > I'm super scared of doing this. Ideally, the Chrome layer shouldn't have to care > about these values. If they are, then we need those call sites to handle the > blimp case separately. > > Doing a quick look, these values don't seem to be used anywhere in Chrome. Would > it be possible to remove them from the observer interface instead? It is possible to fix these values at the engine by listening to a WebContentsObserver. But, so far they are not used by any TabObserver. IMO, it is good to pass these values in case some observer needs it in future, but in that case blimp values needs to be fixed at that point https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:61: mTab.didFinishPageLoad(); On 2016/10/06 03:13:27, Khushal wrote: > Why is this here and now in onLoadingStateChanged? Are you trying to match this > with how events are dispatched by the content layer? It would be nice to have > comments on what events this is trying to match with respect to the content > layer and how it makes sense for us to dispatch them at this point instead. Yes, this is triggered on didFinishLoad, didFailLoad and didFirstPaintAfterLoad methods of WebContentsObserver. I wanted to make sure that didStartPageLoad and didFinishPageLoad are called correctly.
https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:39: mTab.onLoadStarted(true); Can you put a comment stating that we're setting toDifferentDocument as true here? Do we want to pipe this through from the engine? If not, add a TODO and file a bug and assign it to yourself to investigate later. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:40: mTab.notifyLoadProgress(50); Add a comment explaining why we're using 50 here. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); What's the typical call flow for an existing tab? Do we see handleDidCommitProvisionalLoadForFrame after the onLoadStarted/Stopped/Progress changed? We should make sure we're calling the methods in the same order. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( On 2016/10/07 00:41:46, shaktisahu wrote: > On 2016/10/06 03:13:27, Khushal wrote: > > I'm super scared of doing this. Ideally, the Chrome layer shouldn't have to > care > > about these values. If they are, then we need those call sites to handle the > > blimp case separately. > > > > Doing a quick look, these values don't seem to be used anywhere in Chrome. > Would > > it be possible to remove them from the observer interface instead? > > It is possible to fix these values at the engine by listening to a > WebContentsObserver. But, so far they are not used by any TabObserver. > IMO, it is good to pass these values in case some observer needs it in future, > but in that case blimp values needs to be fixed at that point Hmm interesting. We might have used them in the past. If we aren't using them anymore I'd be ok with removing them as well.
https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:40: mTab.notifyLoadProgress(50); Also, reminder that this should probably be in onPageLoadingStateChanged. I'm sorry about the confusing names but that's the hook up we ended up with for the progress bar. onLoadingStateChanged will correspond to WebContents::IsLoading, which will toggle whenever the WebContents starts loading any resource afaik. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:42: mTab.onLoadStopped(); Also nice to comment on why LoadStarted and LoadStopped is here and not in PageLoadStateChange for instance. I'm assuming its because this matches what WebContents does on the engine and those are the calls we pipe from the engine using these messages? Another reason for documenting this is because I think going forward, we should really make these calls using the state of content fetched on the client, rather than replaying the calls from the engine. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); On 2016/10/07 06:45:24, David Trainor wrote: > What's the typical call flow for an existing tab? Do we see > handleDidCommitProvisionalLoadForFrame after the onLoadStarted/Stopped/Progress > changed? We should make sure we're calling the methods in the same order. I tried doing some quick digging but I didn't find a definite order. Naive understanding, I would guess you would have a DidStartLoad followed by DidCommitProvisionalLoad. And then stop based on when the load finishes or it is interrupted. Though since the client will get this message whenever the loading state changes on the engine, I'm not sure if we want to do a commit call every time. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( On 2016/10/07 06:45:24, David Trainor wrote: > On 2016/10/07 00:41:46, shaktisahu wrote: > > On 2016/10/06 03:13:27, Khushal wrote: > > > I'm super scared of doing this. Ideally, the Chrome layer shouldn't have to > > care > > > about these values. If they are, then we need those call sites to handle the > > > blimp case separately. > > > > > > Doing a quick look, these values don't seem to be used anywhere in Chrome. > > Would > > > it be possible to remove them from the observer interface instead? > > > > It is possible to fix these values at the engine by listening to a > > WebContentsObserver. But, so far they are not used by any TabObserver. > > IMO, it is good to pass these values in case some observer needs it in future, > > but in that case blimp values needs to be fixed at that point There is no point sending those values from the engine, that frame id is going to be useless on the client, because we don't have a frame tree which the content layer does. If someone needs these values, then they most probably need for accessing a content object (RenderFrameHost/RenderWidgetHost..), which will be wrong in blimp mode. I would be happier removing them and dealing with someone needing these explicitly when that happens, rather than things failing in a weird way somewhere down the stack. > > Hmm interesting. We might have used them in the past. If we aren't using them > anymore I'd be ok with removing them as well. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( Another thing, is it okay to do to onDidStartProvisionalLoadForFrame here and call handleDidCommitProvisionalLoadForFrame in the method above? I don't think there is any guarantees for the order in which these would happen. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:61: mTab.didFinishPageLoad(); On 2016/10/07 00:41:46, shaktisahu wrote: > On 2016/10/06 03:13:27, Khushal wrote: > > Why is this here and now in onLoadingStateChanged? Are you trying to match > this > > with how events are dispatched by the content layer? It would be nice to have > > comments on what events this is trying to match with respect to the content > > layer and how it makes sense for us to dispatch them at this point instead. > > Yes, this is triggered on didFinishLoad, didFailLoad and didFirstPaintAfterLoad > methods of WebContentsObserver. I wanted to make sure that didStartPageLoad and > didFinishPageLoad are called correctly. Okay, thanks! Comment in code please. :)
Patchset #2 (id:20001) has been deleted
shaktisahu@chromium.org changed reviewers: + mdjones@chromium.org
Adding mdjones@ https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:39: mTab.onLoadStarted(true); On 2016/10/07 06:45:24, David Trainor wrote: > Can you put a comment stating that we're setting toDifferentDocument as true > here? Do we want to pipe this through from the engine? If not, add a TODO and > file a bug and assign it to yourself to investigate later. In clank, this method is triggered by WebContentsDelegate::LoadingStateChanged But in blimp engine, we are only listening to WebContentsDelegate::NavigationStateChanged and querying the loading state from there. So there is a difference where these events are generated. The value of toDifferentDocument=true is used in ToolbarManager to update the button status, reload state, bookmark status etc, which are updated only when it is true. So it looks fine to me to pass this boolean as true. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:40: mTab.notifyLoadProgress(50); On 2016/10/08 01:56:59, Khushal wrote: > Also, reminder that this should probably be in onPageLoadingStateChanged. I'm > sorry about the confusing names but that's the hook up we ended up with for the > progress bar. onLoadingStateChanged will correspond to WebContents::IsLoading, > which will toggle whenever the WebContents starts loading any resource afaik. Ack. I am moving this to the next method. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:40: mTab.notifyLoadProgress(50); On 2016/10/07 06:45:24, David Trainor wrote: > Add a comment explaining why we're using 50 here. Done. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); On 2016/10/07 06:45:24, David Trainor wrote: > What's the typical call flow for an existing tab? Do we see > handleDidCommitProvisionalLoadForFrame after the onLoadStarted/Stopped/Progress > changed? We should make sure we're calling the methods in the same order. As I see from the logs, handleDidCommitProvisionalLoadForFrame is called some time after onLoadStarted. I fixed the order. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( On 2016/10/08 01:56:59, Khushal wrote: > Another thing, is it okay to do to onDidStartProvisionalLoadForFrame here and > call handleDidCommitProvisionalLoadForFrame in the method above? I don't think > there is any guarantees for the order in which these would happen. Actually I checked that this is in the same order as the calls are received in clank world. ToolbarManager would start the progress bar on this signal. https://codereview.chromium.org/2394663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:57: observers.next().onDidStartProvisionalLoadForFrame( On 2016/10/08 01:56:59, Khushal wrote: > On 2016/10/07 06:45:24, David Trainor wrote: > > On 2016/10/07 00:41:46, shaktisahu wrote: > > > On 2016/10/06 03:13:27, Khushal wrote: > > > > I'm super scared of doing this. Ideally, the Chrome layer shouldn't have > to > > > care > > > > about these values. If they are, then we need those call sites to handle > the > > > > blimp case separately. > > > > > > > > Doing a quick look, these values don't seem to be used anywhere in Chrome. > > > Would > > > > it be possible to remove them from the observer interface instead? > > > > > > It is possible to fix these values at the engine by listening to a > > > WebContentsObserver. But, so far they are not used by any TabObserver. > > > IMO, it is good to pass these values in case some observer needs it in > future, > > > but in that case blimp values needs to be fixed at that point > > There is no point sending those values from the engine, that frame id is going > to be useless on the client, because we don't have a frame tree which the > content layer does. If someone needs these values, then they most probably need > for accessing a content object (RenderFrameHost/RenderWidgetHost..), which will > be wrong in blimp mode. I would be happier removing them and dealing with > someone needing these explicitly when that happens, rather than things failing > in a weird way somewhere down the stack. > > > > > Hmm interesting. We might have used them in the past. If we aren't using > them > > anymore I'd be ok with removing them as well. Sure. I will remove these params. I don't want to do in this CL. I will make a new CL for this.
My comments are a result of me possibly not understanding the blimp api. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:27: public void onNavigationStateChanged() { Stop and reset progress in this function? https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:37: * Also sends signals for displaying the page load spinner. nit: s/page load spinner/progress bar/ if that's what you are using. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:49: mTab.onLoadStopped(); Does this ever coincide with onPageLoadingStateChanged? Will this ever be called if the progress is 100% or is it only called when the load is interrupted?
mdjones@ - I have replied to the comments. Since blimp client doesn't have WebContents, we are trying to match up the TabObserver calls and make those calls from BlimpContents. Not all the calls are available right now. I am trying to add only the essential ones here for the progress bar and loading spinner. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:27: public void onNavigationStateChanged() { On 2016/10/13 00:20:35, mdjones wrote: > Stop and reset progress in this function? onPageLoadingStateChanged and hence mTab.didFinishPageLoad is always called even if the page is interrupted. So we don't need to stop/reset progress in this function. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:37: * Also sends signals for displaying the page load spinner. On 2016/10/13 00:20:35, mdjones wrote: > nit: s/page load spinner/progress bar/ if that's what you are using. Sorry, I misjudged. However this sets the Tab's mIsLoading flag correctly. I will update the comments. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:49: mTab.onLoadStopped(); I think this is always called after the page is done loading.
TBH, if its not urgent to hook up to the progress bar yet, it might be better to defer doing this till there is a clearer plan for what the navigation protocol is going to look like. https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java (right): https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:27: public void onNavigationStateChanged() { On 2016/10/13 02:00:02, shaktisahu wrote: > On 2016/10/13 00:20:35, mdjones wrote: > > Stop and reset progress in this function? > > onPageLoadingStateChanged and hence mTab.didFinishPageLoad is always called even > if the page is interrupted. So we don't need to stop/reset progress in this > function. This function corresponds to WebContentsDelegate::NavigationStateChanged. So the code here is effectively trying to do what this does: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); Shakti and I did some digging on this, and looks like onLoadingStateChanged is going to be invoked each time a resource on the page starts/stops loading. At the same time, I *think* it would report the loading to be true if there is a pending navigation that is being loaded. The value here is picked up from WebContents::IsLoading, and following the code we reached here, https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... So I'm not sure if this is the correct place to call navigation committed?
On 2016/10/13 16:00:00, Khushal wrote: > TBH, if its not urgent to hook up to the progress bar yet, it might be better to > defer doing this till there is a clearer plan for what the navigation protocol > is going to look like. > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java > (right): > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:27: > public void onNavigationStateChanged() { > On 2016/10/13 02:00:02, shaktisahu wrote: > > On 2016/10/13 00:20:35, mdjones wrote: > > > Stop and reset progress in this function? > > > > onPageLoadingStateChanged and hence mTab.didFinishPageLoad is always called > even > > if the page is interrupted. So we don't need to stop/reset progress in this > > function. > > This function corresponds to WebContentsDelegate::NavigationStateChanged. So the > code here is effectively trying to do what this does: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: > mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); > Shakti and I did some digging on this, and looks like onLoadingStateChanged is > going to be invoked each time a resource on the page starts/stops loading. At > the same time, I *think* it would report the loading to be true if there is a > pending navigation that is being loaded. The value here is picked up from > WebContents::IsLoading, and following the code we reached here, > https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... > > So I'm not sure if this is the correct place to call navigation committed? If we do want to land this, can the doc be updated to explain when each method is supposed to be called? The current API is incredibly unclear.
On 2016/10/13 16:00:00, Khushal wrote: > TBH, if its not urgent to hook up to the progress bar yet, it might be better to > defer doing this till there is a clearer plan for what the navigation protocol > is going to look like. I think I'd be fine with landing this, but we should rework how the methods in tab look. I don't want to call methods that happen to produce the right result, we should rework tab to be a little more decoupled from the WebContents API. Then we can leverage TabWebContentsDelegate/Observer and TabBlimpContentsObserver to do the custom heavy lifting and figure out what to call on Tab. Right now, as Matt said, it's unclear to me how it's supposed to work. Let's talk later today and maybe dig into the methods a bit. > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java > (right): > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:27: > public void onNavigationStateChanged() { > On 2016/10/13 02:00:02, shaktisahu wrote: > > On 2016/10/13 00:20:35, mdjones wrote: > > > Stop and reset progress in this function? > > > > onPageLoadingStateChanged and hence mTab.didFinishPageLoad is always called > even > > if the page is interrupted. So we don't need to stop/reset progress in this > > function. > > This function corresponds to WebContentsDelegate::NavigationStateChanged. So the > code here is effectively trying to do what this does: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > https://codereview.chromium.org/2394663002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java:47: > mTab.handleDidCommitProvisonalLoadForFrame(mTab.getUrl(), PageTransition.TYPED); > Shakti and I did some digging on this, and looks like onLoadingStateChanged is > going to be invoked each time a resource on the page starts/stops loading. At > the same time, I *think* it would report the loading to be true if there is a > pending navigation that is being loaded. The value here is picked up from > WebContents::IsLoading, and following the code we reached here, > https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... > > So I'm not sure if this is the correct place to call navigation committed?
dtrainor@ - PTAL
lgtm. Thanks!
https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1031: return mBlimpContentsObserver.getProgress(); "getMostRecentProgress makes sense I think. https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1036: return isLoading() ? delegate.getMostRecentProgress() : 100; Do we want to keep this isLoading logic? Should this be something like: if (!isLoading()) return 100; if (mBlimp) { return mBlimpContentsObserver != null ? mBlimpContentsObserver.getMostRecentProgress() : 100; } else { <Existing Delegate Code>. } https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1480: getBlimpContents().addObserver(mBlimpContentsObserver); mBlimpContents.? We use it directly above might as well use it here.
dtrainor@ - PTAL https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1031: return mBlimpContentsObserver.getProgress(); On 2016/10/18 16:43:33, David Trainor wrote: > "getMostRecentProgress makes sense I think. Done. https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1036: return isLoading() ? delegate.getMostRecentProgress() : 100; On 2016/10/18 16:43:33, David Trainor wrote: > Do we want to keep this isLoading logic? Should this be something like: > > if (!isLoading()) return 100; > > if (mBlimp) { > return mBlimpContentsObserver != null ? > mBlimpContentsObserver.getMostRecentProgress() : 100; > } else { > <Existing Delegate Code>. > } Done. https://codereview.chromium.org/2394663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1480: getBlimpContents().addObserver(mBlimpContentsObserver); On 2016/10/18 16:43:33, David Trainor wrote: > mBlimpContents.? We use it directly above might as well use it here. Done.
lgtm
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/2394663002/#ps80001 (title: "dtrainor@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: Propagating page loading status to tab This CL adds support on the Blimp to propagate loading status and page load status to Tab. This enables the UI to display the loading spinner on the tablets and page load progress bar on phones. The page load progress was set to 50 percent for now. BUG=651473 ========== to ========== Blimp: Propagating page loading status to tab This CL adds support on the Blimp to propagate loading status and page load status to Tab. This enables the UI to display the loading spinner on the tablets and page load progress bar on phones. The page load progress was set to 50 percent for now. BUG=651473 Committed: https://crrev.com/8975b80a50bb91c4723c8fc29a871a940b7fdb2f Cr-Commit-Position: refs/heads/master@{#426827} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8975b80a50bb91c4723c8fc29a871a940b7fdb2f Cr-Commit-Position: refs/heads/master@{#426827} |
