Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7036019:
Enabling page visibility api in the chrome browser. (Closed)
|
Created:
9 years, 7 months ago by Shishir Modified:
9 years, 7 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org, jam, Avi (use Gerrit), brettw-cc_chromium.org Visibility:
Public. |
DescriptionEnabling page visibility api in the chrome browser.
BUG=83286
TEST=None.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86569
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86881
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressing Brett's comments. Moving visiblity to TabContentsView. #Patch Set 3 : Using the RenderViews's hidden state only to drive page visibility. #Patch Set 4 : Synced for commit. #
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/7036019/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/7036019/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:564: // Always in a hidden state, comma -> period. http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_view_host.cc:253: DCHECK(delegate_); This assertion isn't really helping since if it fails, the code will immediately crash with an obvious null pointer exception. I usually just tell people to not bother DCHECKing something that will immediately crash in a predictable way. http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_view_host_delegate.h:489: // Returns ture iff the render view host is hidden. ture -> true. iff -> if ("only if" is meaningless in this context and makes it more difficult to read). http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... File content/browser/tab_contents/tab_contents_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... content/browser/tab_contents/tab_contents_delegate.h:291: // Returns true iff the tab contents is not showing. iff -> if http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... content/browser/tab_contents/tab_contents_delegate.h:292: virtual bool IsHidden(const TabContents* source) const; Sorry if I led you astray earlier, but it looks like it will be cleaner to ask the view for its visibility rather than the browser. This will also allow the delegate to be implemented more cleanly in more places (it should come for free for things like HTML dialogs that would otherwise need to implement this). In this case, you can add it to TabContentDelegate::View and even skip the entire TabContents layer. I'd probably call the function IsVisible which is a little bit more natural than "is hidden." http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:721: if (params.is_hidden) { Prefer no {} for single-line conditionals (prevailing style for this file). How does the is_hidden_ variable in RenderWidget get updated? It seems like it would be out of sync with the webkit visibility state in this case? I realized this because I was wondering whether this call could be simplified to webview()->setVisibilityState(visibilityState(), true); http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:4110: if (is_hidden()) { Ditto remove the {}
http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... File content/browser/tab_contents/tab_contents_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... content/browser/tab_contents/tab_contents_delegate.h:292: virtual bool IsHidden(const TabContents* source) const; On 2011/05/18 22:11:38, brettw wrote: > Sorry if I led you astray earlier, but it looks like it will be cleaner to ask > the view for its visibility rather than the browser. This will also allow the > delegate to be implemented more cleanly in more places (it should come for free > for things like HTML dialogs that would otherwise need to implement this). > > In this case, you can add it to TabContentDelegate::View and even skip the > entire TabContents layer. > > I'd probably call the function IsVisible which is a little bit more natural than > "is hidden." By TabContentDelegate::View, do you mean RenderWidgetHostDelegate::View? I tried doing it using the RenderWidgetHostDelegate::View, but it suffers from the problem that when a tab is created hidden (say using a middle click) the WasHidden() call from the browser::Navigate() to the the TabContents happens when the view has not yet been created and so the initial value of the visibility is incorrect.
http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... File content/browser/tab_contents/tab_contents_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... content/browser/tab_contents/tab_contents_delegate.h:292: virtual bool IsHidden(const TabContents* source) const; Is there the opposite problem? i.e. what if we say when there's no view it's invisible? This is certainly the behavior I'd expect if there was no view. Are there problems creating a foreground tab in this case?
http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... File content/browser/tab_contents/tab_contents_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/tab_contents/ta... content/browser/tab_contents/tab_contents_delegate.h:292: virtual bool IsHidden(const TabContents* source) const; The forground tab likely works because we always start off with the is_hidden_ value set to false in all the RenderViewHostDelegate::View implementations, so the default value works out. Even for a hidden tab, the view does get created before the Navigate call to the renderer but its gets created late enough that it does not trap the WasHidden call from the browser::Navigate(). On 2011/05/19 04:45:09, brettw wrote: > Is there the opposite problem? i.e. what if we say when there's no view it's > invisible? This is certainly the behavior I'd expect if there was no view. Are > there problems creating a foreground tab in this case?
The way that this is currently hooked up is messed up. Adding another layer of complexity on top of this seems like the wrong approach. Ideally, the visible/hidden flag should be driven by the view side, which knows based on the native system whether that window is visible or not. Currently, this isn't the case, which is why we're having problems figuring this out. In this case, the TabContents.WasHidden call would be moved to the TabContentsView and would get called as a result of the native OS widget being shown or hidden as a result of the user changing things. It looks like the TabContentsView is already created before the navigate, and the view should always know its visibility. If the WasHidden messages came from the view, and the IsVisible queries that the RenderViewHost.Navigate function calls came from the view, everything should be in sync without having to add extra flags to TabContents.
http://codereview.chromium.org/7036019/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_contents.cc (right): http://codereview.chromium.org/7036019/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_contents.cc:564: // Always in a hidden state, On 2011/05/18 22:11:38, brettw wrote: > comma -> period. Done. http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_view_host.cc:253: DCHECK(delegate_); On 2011/05/18 22:11:38, brettw wrote: > This assertion isn't really helping since if it fails, the code will immediately > crash with an obvious null pointer exception. I usually just tell people to not > bother DCHECKing something that will immediately crash in a predictable way. Removed. http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/7036019/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/render_view_host_delegate.h:489: // Returns ture iff the render view host is hidden. On 2011/05/18 22:11:38, brettw wrote: > ture -> true. > iff -> if ("only if" is meaningless in this context and makes it more difficult > to read). Done. http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc File content/renderer/render_view.cc (right): http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:721: if (params.is_hidden) { On 2011/05/18 22:11:38, brettw wrote: > Prefer no {} for single-line conditionals (prevailing style for this file). > > How does the is_hidden_ variable in RenderWidget get updated? It seems like it > would be out of sync with the webkit visibility state in this case? > > I realized this because I was wondering whether this call could be simplified to > webview()->setVisibilityState(visibilityState(), true); Removed {} around condition. At this point the is_hidden_ property in the renderer and the internal webkit visibility do go out of sync. This happens because we always starts of renderers in the visible state and later update them to the correct state. If the navigation happens before the renderer has been updated, this will be out of sync till its been updated with the right state. http://codereview.chromium.org/7036019/diff/1/content/renderer/render_view.cc... content/renderer/render_view.cc:4110: if (is_hidden()) { On 2011/05/18 22:11:38, brettw wrote: > Ditto remove the {} Done.
A bunch of discussion about this happened off thread and in person, summarized below: - Ideally we want TabContentsView to drive the visibility and dont want such calls in TabContentsDelegate - We have a handle on the mac and win platforms, but might need to make changes to how gtk windows behave to fix it in linux. - Using TabContents is active did not seem like the right semantics to Ben. - The current visibility states implementation in chrome are a bit hairy. Alternate proposal: The visibility api spec currently says that we will always err on the side of visibile, i.e. whenever the state says that we are hidden, it will always be true. Based on this it looks like it might be possible to use the is_hidden variable in the render views and do this entirely in the render view. The two cases where we will be inaccurate will be: 1) When a new page is opened in a hidden tab i.e. using a middle click - Here the render view starts of as visible initially and then become invisible, so the javascript might see the visible state although for a very small period of time. This is also the case I was trying to fix with the changes in TabContentsView. 2) When a new window is opened with an opener set - here again if the window opens in a hidden tab, initially it starts off as visible and then gets the correct state. Making the change in the render view only is very clean, has no additional states and is consistent with the current spec. We can then figure out how to fix the hidden states across chrome to try and be consistent always instead of just trying to fix the visibility code. Thoughts? Thanks Shishir
I think a lower risk, lower complexity change in the RenderView sounds like a good way to get this feature in more quickly.
Uploaded changes to only use the RenderView hidden state as earlier described. Please take a look.
LGTM Having this tied to the interval timer changes makes for a consistent experience for web developers.
In fact, looking at this updated CL I am convinced it is the right one regardless. If there is any brittleness to RenderView::is_hidden() (and I believe that there is) then that should be corrected rather than adding new signals. -Ben On Tue, May 24, 2011 at 12:42 PM, <brettw@chromium.org> wrote: > I think a lower risk, lower complexity change in the RenderView sounds like > a > good way to get this feature in more quickly. > > > http://codereview.chromium.org/7036019/ >
LGTM
Change committed as 86569
Try job failure for 7036019-11002 on win: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Change committed as 86881 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
