Tabs being screen-captured will fullscreen within their tab contents area.

Tabs being screen-captured will fullscreen within their tab contents area.

Note: This change in behavior only applies when the --embed-flash-fullscreen experimental flag is enabled.

This change adds additional logic to FullscreenController to prevent the browser window from entering fullscreen if a tab is being screen-captured.  This allows the user to retain control of their desktop to work in other browser tabs or applications while the fullscreen view is displayed on a remote screen (e.g., a Chromecast receiver).

Furthermore, if a user switches to a different tab, FullscreenController will set the size of the hidden view to the video resolution of the capture.  This is both a quality and performance improvement as harmful scaling and letterboxing can be skipped.

BUG=256870
TEST=New FullscreenController state tests.  Ran all disabled FullscreenControllerInteractiveTests (in interactive_ui_tests).  Also confirmed all new functionality manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250872

[miu, 2014-02-08 03:59:47]

scheib: Could you please review this change?

Change description explains most of this.  Bug 256870 has more history/context,
and demo video here:
http://code.google.com/p/chromium/issues/detail?id=256870#c8

Thanks in advance!  :)

[scheib, 2014-02-12 01:41:16]

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:77: return web_contents ==
fullscreened_tab_ ||
Why drop the DCHECK when !IsCapturedFullscreenTab?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:225: if
(IsCapturedFullscreenTab(web_contents)) {
Won't this break the use case of casting play of fullscreen pointer lock
content? Such as http://media.tojicode.com/q3bsp/
At least file an issue to support pointer locked fullscreen casting.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:285: void
FullscreenController::OnTabDetachedFromView(WebContents* old_contents) {
So far I haven't noticed when we resize content back to the browser window size
when it is again activated. Perhaps comment here where that would happen?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:745: if
(std::find(captured_tabs_.begin(), captured_tabs_.end(),
why not just std::set::insert()?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:754: if (it !=
captured_tabs_.end()) {
captured_tabs_.find(web_contents) != captured_tabs_.end()

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:40: // Special behavior for
screen-captured tabs: When embedded fullscreen widgets
Consider anchoring/naming this note with e.g. 
"""
EmbeddedFullscreen Note: 
Special behavior ...
"""

And then referencing elsewhere with "See 'EmbeddedFullscreen Note'"

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:68: // NOTE: The
zero-argument version returns true iff a fullscreen tab will
For clarity (presuming I have it right): "iff a fullscreen tab and its browser
window is(or will be) fullscreen."

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:72: // case accounts for
the special behavior employed during screen capture of a
And then reference "See 'EmbeddedFullscreen Note'"

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:74: bool
IsFullscreenForTabOrPending() const;
Consider renaming the method to differentiate the meaning between the two,
overloading is getting dicey here.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:197: // |browser_|.
And then reference "See 'EmbeddedFullscreen Note'" for these.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:198: bool
IsEmbeddedFullscreenEnabled() const;
Use a consistent terminology, always 'EmbeddedFullscreen' or
'FullscreenWithinTab', or 'CapturedFullscreen' or ScreenCapture'... but pick
one.

Also, IsEmbeddedFullscreenEnabled and IsCapturedFullscreenTab are similar, and a
tad confusing.

Here's a proposal for these methods in order:
IsEmbeddedFullscreenPossible
MaybeToggleEmbeddedFullscreen
IsEmbeddedFullscreen

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc:43: State
transition_table_data[][NUM_EVENTS] = {
I think it prudent to add states and expected outcomes for tab capture being
enabled and the various sub states. Fullscreen was complex, got worse, and
that's why we built up this exhaustive test suite. This change is adding a
additional complexity, but only two one-off tests.

Please also run the FullscreenControllerStateInteractiveTest locally enabling
the disabled tests from fullscreen_controller_state_interactive_browsertest.cc
if you haven't already.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:480:
TEST_F(FullscreenControllerStateUnitTest, OneCapturedFullscreenedTab) {
Please comment the intent of each test and reference the definition of
'CapturedFullscreenedTab' (can this be consistent also, e.g.
'EmbeddedFullscreen'?).

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:500:
ASSERT_FALSE(wc_delegate->IsFullscreenForTabOrPending(second_tab));
Add assert for "IsFullscreenForTabOrPending()"?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:517: //
Switch back to the first tab and exit fullscreen.
What size should it be now?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:573: }
Some other test scenarios:
- Tab already fullscreen starts to be captured
- Captured tab in background exits fullscreen while a non captured tab in
forground remains fullscreen
- Captured tab is in fullscreen, then user attempts to enter browser fullscreen
(not sure what happens here? Are we still showing UI for browser fullscreen?)

[miu, 2014-02-12 08:25:19]

PTAL.  I've addressed all your comments.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:77: return web_contents ==
fullscreened_tab_ ||
On 2014/02/12 01:41:16, scheib wrote:
> Why drop the DCHECK when !IsCapturedFullscreenTab?

Good point.  I rearranged this to put it back.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:225: if
(IsCapturedFullscreenTab(web_contents)) {
On 2014/02/12 01:41:16, scheib wrote:
> Won't this break the use case of casting play of fullscreen pointer lock
> content? Such as http://media.tojicode.com/q3bsp/
> At least file an issue to support pointer locked fullscreen casting. 

Resolved.  I took this code out and mouse lock works perfectly during tab
capture.  ;-)

Originally, I was thinking mouse lock only happens in fullscreen mode, but I now
see mouse lock can happen without fullscreen.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:285: void
FullscreenController::OnTabDetachedFromView(WebContents* old_contents) {
On 2014/02/12 01:41:16, scheib wrote:
> So far I haven't noticed when we resize content back to the browser window
size
> when it is again activated. Perhaps comment here where that would happen?

Done.  (Reworked this into the comment a few lines down.)

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:745: if
(std::find(captured_tabs_.begin(), captured_tabs_.end(),
On 2014/02/12 01:41:16, scheib wrote:
> why not just std::set::insert()?

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:754: if (it !=
captured_tabs_.end()) {
On 2014/02/12 01:41:16, scheib wrote:
> captured_tabs_.find(web_contents) != captured_tabs_.end()

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:769:
DCHECK_NE(mouse_lock_tab_, web_contents);
I removed this since it was wrong (i.e., mouse lock is valid on screen-captured
tabs).

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:40: // Special behavior for
screen-captured tabs: When embedded fullscreen widgets
On 2014/02/12 01:41:16, scheib wrote:
> Consider anchoring/naming this note with e.g. 
> """
> EmbeddedFullscreen Note: 
> Special behavior ...
> """
> 
> And then referencing elsewhere with "See 'EmbeddedFullscreen Note'"

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:68: // NOTE: The
zero-argument version returns true iff a fullscreen tab will
On 2014/02/12 01:41:16, scheib wrote:
> For clarity (presuming I have it right): "iff a fullscreen tab and its browser
> window is(or will be) fullscreen."

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:72: // case accounts for
the special behavior employed during screen capture of a
On 2014/02/12 01:41:16, scheib wrote:
> And then reference "See 'EmbeddedFullscreen Note'"

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:74: bool
IsFullscreenForTabOrPending() const;
On 2014/02/12 01:41:16, scheib wrote:
> Consider renaming the method to differentiate the meaning between the two,
> overloading is getting dicey here.

Agreed.  There are quite a few callers of it, so I'd prefer to follow this up in
a separate clean-up CL.  SGTY?

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:197: // |browser_|.
On 2014/02/12 01:41:16, scheib wrote:
> And then reference "See 'EmbeddedFullscreen Note'" for these.

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:198: bool
IsEmbeddedFullscreenEnabled() const;
On 2014/02/12 01:41:16, scheib wrote:
> Use a consistent terminology, always 'EmbeddedFullscreen' or
> 'FullscreenWithinTab', or 'CapturedFullscreen' or ScreenCapture'... but pick
> one.

I see your point, and agree the naming was confusing.  Please take a look at how
I rewrote the 'FullscreenWithinTab Note' at the top of this file, since it
points out how there are two related pieces of functionality at play here
(http://crbug.com/290403 versus http://crbug.com/256870).  Both involve showing
fullscreen widgets within the tab contents area.  The special-case (introduced
by this CL) is when a tab is being screen-captured: We do not fullscreen the
browser window.

I've reworked the naming of the methods (and terminology in comments) to be
consistent and also clarify differences.  I decided to stop using the "embedded
fullscreen" terminology altogether since it is overused throughout Chromium code
and could be easily confused with unrelated NPAPI special-case foo in
src/content.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc:43: State
transition_table_data[][NUM_EVENTS] = {
On 2014/02/12 01:41:16, scheib wrote:
> I think it prudent to add states and expected outcomes for tab capture being
> enabled and the various sub states. Fullscreen was complex, got worse, and
> that's why we built up this exhaustive test suite. This change is adding a
> additional complexity, but only two one-off tests.

Regarding the additional complexity to FullscreenController: The way to think
about this CL is that I'm just adding a small amount of logic to
FullscreenController that sits on the side.  This new logic pretty much coexists
alongside the existing logic, and doesn't alter nor interfere with its behavior.
 The two sets of logic are pretty much orthogonal.

Regarding this test code: I originally started down the route you suggest, but
quickly found out it wouldn't work: All the test infrastructure here assumes
only one tab can be fullscreened at a time.  And, that makes sense, since all
the state transitions here only apply to the control logic that will manipulate
the browser window fullscreen state.

Overall, IMHO, the tests I've added do show that the screen-captured-tab
fullscreen toggles coexist harmoniously with the normal fullscreen toggles, and
that neither can interfere with each other.

> Please also run the FullscreenControllerStateInteractiveTest locally enabling
> the disabled tests from fullscreen_controller_state_interactive_browsertest.cc
> if you haven't already.

Done, all passing once I fixed one of the tests.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:480:
TEST_F(FullscreenControllerStateUnitTest, OneCapturedFullscreenedTab) {
On 2014/02/12 01:41:16, scheib wrote:
> Please comment the intent of each test and reference the definition of
> 'CapturedFullscreenedTab' (can this be consistent also, e.g.
> 'EmbeddedFullscreen'?).

Done.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:500:
ASSERT_FALSE(wc_delegate->IsFullscreenForTabOrPending(second_tab));
On 2014/02/12 01:41:16, scheib wrote:
> Add assert for "IsFullscreenForTabOrPending()"?

Done, and throughout all these new tests.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:517: //
Switch back to the first tab and exit fullscreen.
On 2014/02/12 01:41:16, scheib wrote:
> What size should it be now?

It can't be determined in these tests since we've mocked the Browser window.  As
explained in fullscreen_controller.cc, when the tab is selected, the attaching
of the widget to the UI will force a resize.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:573: }
On 2014/02/12 01:41:16, scheib wrote:
> Some other test scenarios:
> - Tab already fullscreen starts to be captured
> - Captured tab in background exits fullscreen while a non captured tab in
> forground remains fullscreen
> - Captured tab is in fullscreen, then user attempts to enter browser
fullscreen
> (not sure what happens here? Are we still showing UI for browser fullscreen?)

Done, except for the first scenario because there's no event mechanism to signal
FullscreenController that tab capture has started.  This is actually okay, since
tab capture can only be started via extension APIs, and only in response to a
user input event in a browser action widget.  Therefore, it shouldn't be
possible to start capture after a tab enters fullscreen.

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/fulls...
File
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc
(right):

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/fulls...
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc:651:
ASSERT_FALSE(IsFullscreenBubbleDisplayed());
This test, that I originally authored, was broken in a previous CL.  By me. 
Because I forgot to manually run these tests for
https://codereview.chromium.org/26803002.  I hang my head in shame.

BTW--The correct behavior *is* for the bubble not to be displayed.  This is the
test for Flash widgets that are Fullscreened-Within-Tab.  Flash is responsible
for displaying the "Press Esc to exit fullscreen" message, not the bubble.

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/fulls...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/fulls...
chrome/browser/ui/fullscreen/fullscreen_controller_state_unittest.cc:525: //
TODO(miu): Need to make an adjustment to content::WebContentsViewMac for
Try bots caught this.  I'll need to implement
WebContentsViewMac::GetViewBounds().  I'm kind of surprised they didn't
implement it.

[scheib, 2014-02-12 18:01:36]

lgtm with a small nit:

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:74: bool
IsFullscreenForTabOrPending() const;
On 2014/02/12 08:25:19, miu wrote:
> On 2014/02/12 01:41:16, scheib wrote:
> > Consider renaming the method to differentiate the meaning between the two,
> > overloading is getting dicey here.
> 
> Agreed.  There are quite a few callers of it, so I'd prefer to follow this up
in
> a separate clean-up CL.  SGTY?

Yes.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:198: bool
IsEmbeddedFullscreenEnabled() const;
On 2014/02/12 08:25:19, miu wrote:
> On 2014/02/12 01:41:16, scheib wrote:
> > Use a consistent terminology, always 'EmbeddedFullscreen' or
> > 'FullscreenWithinTab', or 'CapturedFullscreen' or ScreenCapture'... but pick
> > one.
> 
> I see your point, and agree the naming was confusing.  Please take a look at
how
> I rewrote the 'FullscreenWithinTab Note' at the top of this file, since it
> points out how there are two related pieces of functionality at play here
> (http://crbug.com/290403 versus http://crbug.com/256870).  Both involve
showing
> fullscreen widgets within the tab contents area.  The special-case (introduced
> by this CL) is when a tab is being screen-captured: We do not fullscreen the
> browser window.
> 
> I've reworked the naming of the methods (and terminology in comments) to be
> consistent and also clarify differences.  I decided to stop using the
"embedded
> fullscreen" terminology altogether since it is overused throughout Chromium
code
> and could be easily confused with unrelated NPAPI special-case foo in
> src/content.
> 

Thanks - I think it's clearer now - though still subtle that
'FullscreenWithinTab' is a super set of 'FullscreenForCapturedTab'.

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc (right):

https://codereview.chromium.org/158253002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller_state_test.cc:43: State
transition_table_data[][NUM_EVENTS] = {
On 2014/02/12 08:25:19, miu wrote:
> On 2014/02/12 01:41:16, scheib wrote:
> > I think it prudent to add states and expected outcomes for tab capture being
> > enabled and the various sub states. Fullscreen was complex, got worse, and
> > that's why we built up this exhaustive test suite. This change is adding a
> > additional complexity, but only two one-off tests.
> 
> Regarding the additional complexity to FullscreenController: The way to think
> about this CL is that I'm just adding a small amount of logic to
> FullscreenController that sits on the side.  This new logic pretty much
coexists
> alongside the existing logic, and doesn't alter nor interfere with its
behavior.
>  The two sets of logic are pretty much orthogonal.
> 
> Regarding this test code: I originally started down the route you suggest, but
> quickly found out it wouldn't work: All the test infrastructure here assumes
> only one tab can be fullscreened at a time.  And, that makes sense, since all
> the state transitions here only apply to the control logic that will
manipulate
> the browser window fullscreen state.
> 
> Overall, IMHO, the tests I've added do show that the screen-captured-tab
> fullscreen toggles coexist harmoniously with the normal fullscreen toggles,
and
> that neither can interfere with each other.
> 
> > Please also run the FullscreenControllerStateInteractiveTest locally
enabling
> > the disabled tests from
fullscreen_controller_state_interactive_browsertest.cc
> > if you haven't already.
> 
> Done, all passing once I fixed one of the tests.

The controller unfortunately has several 'orthogonal' bits of logic now... mouse
lock, metro snap, (somewhat more integral mac with-chrome fullscreen), and in
tab & while capturing FS. ;) It adds up, and it's hard to be confident when
making changes to the FSC that there isn't some subtle connection.

I understand the lack of suitability for the infrastructure, so off we go
without trying to bake this in. I think your additional tests should help.

REQUEST: Please add a DCHECK (somewhere?) to notify a developer if we break the
assumption that we won't start a capture session in an already fullscreened tab.

[miu, 2014-02-12 20:17:17]

sky: Need OWNERS approval for one-line addition to browser.cc.

[sky, 2014-02-12 21:30:12]

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:984:
fullscreen_controller_->OnTabDetachedFromView(old_contents);
Is it possible to make fullscreencontroller observer the TabStripModel directly?
Or are the ordering dependencies if we do that?

[miu, 2014-02-12 21:49:14]

scheib: Added DCHECK as you requested, to notify a developer if we break the
assumption that we won't start a capture session in an already fullscreened tab.

sky: Responding to your question:

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/158253002/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:984:
fullscreen_controller_->OnTabDetachedFromView(old_contents);
On 2014/02/12 21:30:13, sky wrote:
> Is it possible to make fullscreencontroller observer the TabStripModel
directly?
> Or are the ordering dependencies if we do that?

Yes, there are definitely ordering dependencies, both for existing code and the
new code introduced in this CL.  In this CL, FullscreenController resizes a
detached fullscreen widget to the video resolution of the tab capture.  The
widget needs to be detached first (via the statement above this one) for this to
work correctly.

[sky, 2014-02-12 21:51:15]

Ok, LGTM

[miu, 2014-02-12 21:53:02]

The CQ bit was checked by miu@chromium.org

[miu, 2014-02-12 21:54:25]

Thanks everyone for the quick turnaround!  The team will be very happy to have
this in M34 for usability testing/feedback.  :)

[commit-bot: I haz the power, 2014-02-12 21:56:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/158253002/290001

[commit-bot: I haz the power, 2014-02-13 00:53:12]

Change committed as 250872