Fullscreen: make fullscreen requests come from RenderFrame

Fullscreen: make fullscreen requests come from RenderFrame.

This is implementing WebFrameClient::enterFullscreen and
::exitFullscreen in RenderFrameImpl which allows to associate
a fullscreen request to a specific frame.

This is keeping track of the origin by getting it from the
RenderFrameHost then forwards the call all the way to the
WebContentsDelegate.

This is also changing the permission handling by properly differentiate
requesting and embedding origins. If permission is given to a
top frame, it will always be allowed if that origin is embedded.

In addition, it allows OOPIF to request to go fullscreen. It does
not make them fullscreen but the top frame is because of the widget
not yet working correctly for OOPIF.

This part of a multi-sided CL:
Part 1: https://codereview.chromium.org/790543003/
Part 2: <this>
Part 3: https://codereview.chromium.org/782243003/

BUG=374854
TEST=Requires MANUAL testing because of fullscreen-related tests being all disabled for flakyness. Steps are:
- Remove all exceptions for fullscreen:
  - Go to chrome://settings/content
  - Click on "Manage Exceptions" in Fullscreen section;
  - Remove all exceptions;
- Go to http://blog.chromium.org/2014/12/chrome-40-beta-powerful-offline-and.html
  - Start the video and fullscreen it;
  - The UI prompt should ask for fullscreen permission for youtube.com
  - Click ALLOW
- Go to https://www.youtube.com, play any video and go fullscreen;
  - You should see the same message as before, asking for permission.
  - Press ESC;
- Remove all fullscreen exceptions as in the first step;
- Go back to http://blog.chromium.org/2014/12/chrome-40-beta-powerful-offline-and.html
  - Play the video, go fullscreen
  - It should ask again for permission, don't give it, press ESC;
- Go to https://www.youtube.com, play any video and go fullscreen;
  - Press ALLOW and leave the video;
- Go back to http://blog.chromium.org/2014/12/chrome-40-beta-powerful-offline-and.html
  - Play the video, go fullscreen
  - The message should no longer ask for permission.

Committed: https://crrev.com/7a78d6fd7dc3241a91a614a09f9c59a0e4454135
Cr-Commit-Position: refs/heads/master@{#312038}

[mlamouri (slow - plz ping), 2014-12-08 20:20:25]

mlamouri@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org

[mlamouri (slow - plz ping), 2014-12-08 20:20:26]

dcheng@chromium.org: Please review changes in:
 *_messages.h

creis@chromium.org: Please review changes in:
 * content/

[mlamouri (slow - plz ping), 2014-12-08 20:21:57]

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.h:295: void
EnterFullscreen(const GURL& origin);
It would probably have been a better idea to go trough RenderFrameHostDelegate
but it would have made that CL even bigger because the change would have been
more disruptive.

I think it could be interesting to investigate that in a follow-up.

[Charlie Reis, 2014-12-10 00:51:21]

Thanks-- looks pretty close.  

I debated suggesting url::Origin instead of GURL for |origin|, but it looks like
that's more pain than it's worth.  (Alex points out that url::Origin and
GURL::GetOrigin aren't actually compatible, so we would have to change
everything that touches it.)  Let's leave it as a GURL.

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

https://codereview.chromium.org/789533002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:98: const GURL& origin,
nit: Document what |origin| means and is used for.

https://codereview.chromium.org/789533002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1116:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
nit: This whole class is on the UI thread, so we don't usually include these
checks here.  (Looks like it snuck in during
https://codereview.chromium.org/7461059, but that's the only occurrence of it in
RenderViewHostImpl.)

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.h:295: void
EnterFullscreen(const GURL& origin);
On 2014/12/08 20:21:57, Mounir Lamouri wrote:
> It would probably have been a better idea to go trough RenderFrameHostDelegate
> but it would have made that CL even bigger because the change would have been
> more disruptive.
> 
> I think it could be interesting to investigate that in a follow-up.

Hmm, it does seem like that would be a better option.

To do that, we would need RenderFrameHostImpl to be able to call WasResized and
RejectMouseLockOrUnlockIfNecessary, right?  Those are both widget methods, and
it makes sense for RenderFrameHostImpl to be able to call them (on its
render_view_host_ for now, and on its widget after RVH goes away).

Technically, we could already do it.  WasResized is public, and
RejectMouseLockOrUnlockIfNecessary is protected but RenderFrameHostImpl is a
friend of RenderViewHostImpl while we move code between the two.  That's kind of
violating the spirit of the friend declaration, though, since the method
actually lives in RenderWidgetHostImpl.  It would be better to make it public on
RWHI.

I wouldn't be opposed to doing that in this CL, but feel free to send a followup
CL for that if you prefer.

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:593: virtual bool
IsFullscreenForCurrentTab() const = 0;
This doesn't look like it's used outside of content/, so we shouldn't put it in
the public interface.

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:375: // |origin| will always be
empty when |enter_fullscreen| is false.
These comments help, but please first say what |origin| is for (i.e., what it
means when it isn't empty).

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:376: // |origin| can be empty
when |enter_fullscreen| in which case the
when |enter_fullscreen| is true, in which case...

[dcheng, 2014-12-10 06:58:01]

https://codereview.chromium.org/789533002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1431: DCHECK(enter_fullscreen
|| origin.is_empty());
I'm not fond of this pattern. It seems that things would read much more clearly
as:

r->EnterFullscreenMode(origin);

and

r->ExitFullscreen();

Clearly this isn't actually a toggling function, since you have to specify
whether you're entering or exiting fullscreen mode anyway. I realize that this
pattern is older than this patch. However, IMO, all the new calls to
ToggleFullscreen(..., GURL(), false) are a lot less readable, so it would be
nice to fix it.

If having one entry point for this really is that much simpler, then it should
be called SetFullscreen() or something, not ToggleFullscreen().

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3417: bool
RenderFrameImpl::enterFullscreen()
What's the point of having a bool return value if we always return true?

[mlamouri (slow - plz ping), 2014-12-10 15:47:02]

PTAL

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

https://codereview.chromium.org/789533002/diff/1/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen/fullscreen_controller.h:98: const GURL& origin,
On 2014/12/10 00:51:21, Charlie Reis wrote:
> nit: Document what |origin| means and is used for.

Done.

https://codereview.chromium.org/789533002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1116:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/12/10 00:51:21, Charlie Reis wrote:
> nit: This whole class is on the UI thread, so we don't usually include these
> checks here.  (Looks like it snuck in during
> https://codereview.chromium.org/7461059, but that's the only occurrence of it
in
> RenderViewHostImpl.)

Done.

https://codereview.chromium.org/789533002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1116:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/12/10 00:51:21, Charlie Reis wrote:
> nit: This whole class is on the UI thread, so we don't usually include these
> checks here.  (Looks like it snuck in during
> https://codereview.chromium.org/7461059, but that's the only occurrence of it
in
> RenderViewHostImpl.)

Done.

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.h:295: void
EnterFullscreen(const GURL& origin);
On 2014/12/10 00:51:21, Charlie Reis wrote:
> On 2014/12/08 20:21:57, Mounir Lamouri wrote:
> > It would probably have been a better idea to go trough
RenderFrameHostDelegate
> > but it would have made that CL even bigger because the change would have
been
> > more disruptive.
> > 
> > I think it could be interesting to investigate that in a follow-up.
> 
> Hmm, it does seem like that would be a better option.
> 
> To do that, we would need RenderFrameHostImpl to be able to call WasResized
and
> RejectMouseLockOrUnlockIfNecessary, right?  Those are both widget methods, and
> it makes sense for RenderFrameHostImpl to be able to call them (on its
> render_view_host_ for now, and on its widget after RVH goes away).
> 
> Technically, we could already do it.  WasResized is public, and
> RejectMouseLockOrUnlockIfNecessary is protected but RenderFrameHostImpl is a
> friend of RenderViewHostImpl while we move code between the two.  That's kind
of
> violating the spirit of the friend declaration, though, since the method
> actually lives in RenderWidgetHostImpl.  It would be better to make it public
on
> RWHI.
> 
> I wouldn't be opposed to doing that in this CL, but feel free to send a
followup
> CL for that if you prefer.

Done. But I had to move some things around. RenderViewHost had a
ExitFullscreen() method that was called by FullscreenController and
WebContentsImpl. I moved this to WebContents. I think it makes more sense
because FullscreenController is mostly working with WebContents and was using
RVH mostly for that. However, it's a bit odd to have WebContentsImpl
implementing WebContents::ExitFullscreen and
RenderFrameHostDelegate::ExitFullscreenMode...

https://codereview.chromium.org/789533002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1431: DCHECK(enter_fullscreen
|| origin.is_empty());
On 2014/12/10 06:58:00, dcheng wrote:
> I'm not fond of this pattern. It seems that things would read much more
clearly
> as:
> 
> r->EnterFullscreenMode(origin);
> 
> and
> 
> r->ExitFullscreen();
> 
> Clearly this isn't actually a toggling function, since you have to specify
> whether you're entering or exiting fullscreen mode anyway. I realize that this
> pattern is older than this patch. However, IMO, all the new calls to
> ToggleFullscreen(..., GURL(), false) are a lot less readable, so it would be
> nice to fix it.
> 
> If having one entry point for this really is that much simpler, then it should
> be called SetFullscreen() or something, not ToggleFullscreen().

I've change WebContentsDelegate to have EnterFullscreenModeForTab and
ExitFullscreenModeForTab. In most places I redirected to the previous
ToggleFullscreenModeForTab method but it should be better.

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:593: virtual bool
IsFullscreenForCurrentTab() const = 0;
On 2014/12/10 00:51:21, Charlie Reis wrote:
> This doesn't look like it's used outside of content/, so we shouldn't put it
in
> the public interface.

Hmm, I did not meant to have this in this CL. It's a remaining of trying to move
IsFullscreenForCurrentTab out of RVHDelegate and directly to the  WebContents.

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:375: // |origin| will always be
empty when |enter_fullscreen| is false.
On 2014/12/10 00:51:21, Charlie Reis wrote:
> These comments help, but please first say what |origin| is for (i.e., what it
> means when it isn't empty).

Done.

https://codereview.chromium.org/789533002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:376: // |origin| can be empty
when |enter_fullscreen| in which case the
On 2014/12/10 00:51:21, Charlie Reis wrote:
> when |enter_fullscreen| is true, in which case...

No longer exists.

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3417: bool
RenderFrameImpl::enterFullscreen()
On 2014/12/10 06:58:00, dcheng wrote:
> What's the point of having a bool return value if we always return true?

It's only in order to do the transition in Blink. WebFrameClient default
implementation returns false so I can fallback to WebWidgetClient.

[Charlie Reis, 2014-12-10 19:22:57]

Thanks!  LGTM with nits.

(Side note: Uploading rebases in their own patch set would have made the diff a
bit easier to review.  Not a big deal, though.)

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/789533002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.h:295: void
EnterFullscreen(const GURL& origin);
On 2014/12/10 15:47:02, Mounir Lamouri wrote:
> On 2014/12/10 00:51:21, Charlie Reis wrote:
> > On 2014/12/08 20:21:57, Mounir Lamouri wrote:
> > > It would probably have been a better idea to go trough
> RenderFrameHostDelegate
> > > but it would have made that CL even bigger because the change would have
> been
> > > more disruptive.
> > > 
> > > I think it could be interesting to investigate that in a follow-up.
> > 
> > Hmm, it does seem like that would be a better option.
> > 
> > To do that, we would need RenderFrameHostImpl to be able to call WasResized
> and
> > RejectMouseLockOrUnlockIfNecessary, right?  Those are both widget methods,
and
> > it makes sense for RenderFrameHostImpl to be able to call them (on its
> > render_view_host_ for now, and on its widget after RVH goes away).
> > 
> > Technically, we could already do it.  WasResized is public, and
> > RejectMouseLockOrUnlockIfNecessary is protected but RenderFrameHostImpl is a
> > friend of RenderViewHostImpl while we move code between the two.  That's
kind
> of
> > violating the spirit of the friend declaration, though, since the method
> > actually lives in RenderWidgetHostImpl.  It would be better to make it
public
> on
> > RWHI.
> > 
> > I wouldn't be opposed to doing that in this CL, but feel free to send a
> followup
> > CL for that if you prefer.
> 
> Done. But I had to move some things around. RenderViewHost had a
> ExitFullscreen() method that was called by FullscreenController and
> WebContentsImpl. I moved this to WebContents. I think it makes more sense
> because FullscreenController is mostly working with WebContents and was using
> RVH mostly for that. However, it's a bit odd to have WebContentsImpl
> implementing WebContents::ExitFullscreen and
> RenderFrameHostDelegate::ExitFullscreenMode...

I agree, this is much better.  And it's not a big deal to have two versions (one
called from below, one from above).

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (left):

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:143: // We need to update
the fullscreen exit bubble, e.g., going from browser
nit: Please preserve this comment.

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1421: // The callers of this
method seem to call it when they expect |WebContents|
This seems a bit hand-wavy.  Let's just say "Clean up related state and initiate
the fullscreen exit."

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1428: // This method is being
called to enter or leave renderer-initiated fullscreen
nit: Remove "or leave"

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:592: // Requests the renderer to exit
fullscreen.
nit: "Asks the renderer to exit..." or "Requests that the renderer exit..."

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:376: // |origin| can be empty in
which case, the |web_contents| last committed
nit: no comma

https://codereview.chromium.org/789533002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/789533002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3414: {
Style nit: Please put open brace on previous line to match Chromium style rather
than Blink style.  (Might as well fix didChangeManifest as well.)

[Charlie Reis, 2014-12-10 19:23:37]

[+site-isolation-reviews for FYI: making fullscreen frame specific for OOPIF]

[mlamouri (slow - plz ping), 2014-12-10 20:36:53]

mlamouri@chromium.org changed reviewers:
+ scheib@chromium.org

[mlamouri (slow - plz ping), 2014-12-10 20:36:53]

scheib@chromium.org: Please review changes in 
- chrome/browser/ui/fullscreen/
- extensions/

[mlamouri (slow - plz ping), 2014-12-10 20:38:15]

mlamouri@chromium.org changed reviewers:
+ blundell@chromium.org

[mlamouri (slow - plz ping), 2014-12-10 20:38:16]

blundell@chromium.org: Please review changes in:
- components/web_contents_delegate_android/

[mlamouri (slow - plz ping), 2014-12-10 20:39:53]

mlamouri@chromium.org changed reviewers:
+ jhawkins@chromium.org

[mlamouri (slow - plz ping), 2014-12-10 20:39:54]

jhawkins@chromium.org: Please review changes in 
- chrome/browser/ui/browser.*
- chrome/browser/ui/views/

(the fullscreen bits are being reviewed by scheib@)

[dcheng, 2014-12-10 20:45:01]

dcheng@chromium.org changed reviewers:
- blundell@chromium.org, jhawkins@chromium.org

[dcheng, 2014-12-10 20:45:02]

IPC changes lgtm

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/789533002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3417: bool
RenderFrameImpl::enterFullscreen()
On 2014/12/10 at 15:47:02, Mounir Lamouri wrote:
> On 2014/12/10 06:58:00, dcheng wrote:
> > What's the point of having a bool return value if we always return true?
> 
> It's only in order to do the transition in Blink. WebFrameClient default
implementation returns false so I can fallback to WebWidgetClient.

Let's make sure to clean this up afterwards.

[blundell, 2014-12-10 20:47:15]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2014-12-10 20:47:16]

https://codereview.chromium.org/789533002/diff/40001/components/web_contents_...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/789533002/diff/40001/components/web_contents_...
components/web_contents_delegate_android/web_contents_delegate_android.cc:384:
env, obj.obj(), true);
Isn't this change a no-op? I would expect it be doing something with |origin|.

[mlamouri (slow - plz ping), 2014-12-10 20:51:40]

mlamouri@chromium.org changed reviewers:
+ jhawkins@chromium.org

[mlamouri (slow - plz ping), 2014-12-10 20:52:35]

https://codereview.chromium.org/789533002/diff/40001/components/web_contents_...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/789533002/diff/40001/components/web_contents_...
components/web_contents_delegate_android/web_contents_delegate_android.cc:384:
env, obj.obj(), true);
On 2014/12/10 20:47:15, blundell wrote:
> Isn't this change a no-op? I would expect it be doing something with |origin|.

It is indeed a no-op. We don't use the origin on Android so there is no point in
passing in trough for now.

[blundell, 2014-12-10 20:53:24]

I see, didn't see the forest for the trees.

lgtm

[scheib, 2014-12-10 21:36:30]

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:422:
settings_map->SetContentSetting(
Should be updated to match GetContentSetting?

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:707: GetEmbeddingOrigin(),
GetContentSetting documentation could be clearer here, I don't understand the
implication of the ordering between primary and secondary URLs, it seems rules
match both, but I'm not clear on the ramifications.

How are these URLs displayed in content settings?

Presume a.com has an iframe of b.com. What is the outcome of this call, and if
the user presses accept button on fullscreen dialog:
old content setting:                      new content setting: 
   a.com  , b.com  : Fullscreen happens?,         a.com  , b.com
   ---------------------------------------------------------------
   ask    , ask    : prompted           ,         ?      , ?
   allowed, ask    : ?                  ,         allowed, ?
   ask    , allowed: ?                  ,         ?      , allowed

Code/comments here should make this clear and reference updated
GetContentSetting comment that should make the difference between primary and
secondary clear.

Manual QA should be informed of any fullscreen changes, and the difference of
iframed content behavior should be called out with a change comment TEST= line.

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1420: void
WebContentsImpl::ExitFullscreen() {
declaration & definition order should match:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

[mlamouri (slow - plz ping), 2014-12-11 16:03:20]

PTAL

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (left):

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:143: // We need to update
the fullscreen exit bubble, e.g., going from browser
On 2014/12/10 19:22:56, Charlie Reis wrote:
> nit: Please preserve this comment.

Done.

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:422:
settings_map->SetContentSetting(
On 2014/12/10 21:36:30, scheib wrote:
> Should be updated to match GetContentSetting?

Oh! Good catch. I really thought I updated that. It's done now.

https://codereview.chromium.org/789533002/diff/40001/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:707: GetEmbeddingOrigin(),
On 2014/12/10 21:36:30, scheib wrote:
> GetContentSetting documentation could be clearer here, I don't understand the
> implication of the ordering between primary and secondary URLs, it seems rules
> match both, but I'm not clear on the ramifications.
> 
> How are these URLs displayed in content settings?
> 
> Presume a.com has an iframe of b.com. What is the outcome of this call, and if
> the user presses accept button on fullscreen dialog:
> old content setting:                      new content setting: 
>    a.com  , b.com  : Fullscreen happens?,         a.com  , b.com
>    ---------------------------------------------------------------
>    ask    , ask    : prompted           ,         ?      , ?
>    allowed, ask    : ?                  ,         allowed, ?
>    ask    , allowed: ?                  ,         ?      , allowed
> 
> Code/comments here should make this clear and reference updated
> GetContentSetting comment that should make the difference between primary and
> secondary clear.
> 
> Manual QA should be informed of any fullscreen changes, and the difference of
> iframed content behavior should be called out with a change comment TEST=
line.

I'm using PermissionContextBase (chrome/browser/content_settings/) as my
documentation mostly. I agree that content settings documentation could be
improved but it sounds orthogonal to that CL. I think we might want to have a
FullscreenPermissionContext. I will add a comment to explain the implication of
that change though.

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1420: void
WebContentsImpl::ExitFullscreen() {
On 2014/12/10 21:36:30, scheib wrote:
> declaration & definition order should match:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1421: // The callers of this
method seem to call it when they expect |WebContents|
On 2014/12/10 19:22:57, Charlie Reis wrote:
> This seems a bit hand-wavy.  Let's just say "Clean up related state and
initiate
> the fullscreen exit."

Done.

https://codereview.chromium.org/789533002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1428: // This method is being
called to enter or leave renderer-initiated fullscreen
On 2014/12/10 19:22:57, Charlie Reis wrote:
> nit: Remove "or leave"

Done. Updated the comment for both ::EnterFullscreenMode and
::ExitFullscreenMode

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/789533002/diff/40001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:376: // |origin| can be empty in
which case, the |web_contents| last committed
On 2014/12/10 19:22:57, Charlie Reis wrote:
> nit: no comma

Done.

https://codereview.chromium.org/789533002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/789533002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3414: {
On 2014/12/10 19:22:57, Charlie Reis wrote:
> Style nit: Please put open brace on previous line to match Chromium style
rather
> than Blink style.  (Might as well fix didChangeManifest as well.)

Done.

[scheib, 2014-12-11 17:56:56]

extensions/* lgtm
c/b/ui/fullscreen lgtm with improved comment

Also TEST line (as discussed in person) should be even more explicit as a test
plan any 'chrome user' could follow.

https://codereview.chromium.org/789533002/diff/60002/chrome/browser/ui/fullsc...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/789533002/diff/60002/chrome/browser/ui/fullsc...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:429: // use fullscreen,
regardless of the embedder.
This was still difficult for me to understand. Here's a shot at a rewrite:

Origins can enter fullscreen even when embedded in other origins. Permission is
tracked based on the combinations of requester and embedder. Thus, even if a
requesting origin has been previously approved for embedder A, it will not be
approved when embedded in a different origin B.

However, an exception is made when a requester and an embedder are the same
origin. If that combination is ALLOWED, then future requests from that origin
will succeed no matter what the embedder is. E.g. youtube.com is visited and
user selects ALLOW. Later user visits example.com which embeds youtube in an
iframe, which is then ALLOWed.


If other areas of the code base (PermissionContextBase) explain this behavior
well then add a reference.

[mlamouri (slow - plz ping), 2014-12-12 10:43:34]

mlamouri@chromium.org changed reviewers:
+ felt@chromium.org

[mlamouri (slow - plz ping), 2014-12-12 10:43:34]

Comments applied, TEST steps written. I've also changed the top frame/embedded
frame rule. I checked with Adrienne from Chrome Security that it was fine (added
here as reviewer).

I have to wait for the Blink CLs to land and roll before landing this.

[felt, 2014-12-12 15:34:53]

On 2014/12/12 10:43:34, Mounir Lamouri wrote:
> Comments applied, TEST steps written. I've also changed the top frame/embedded
> frame rule. I checked with Adrienne from Chrome Security that it was fine
(added
> here as reviewer).
> 
> I have to wait for the Blink CLs to land and roll before landing this.

top/embedded frame rule change lgtm

[felt, 2014-12-12 15:37:56]

On 2014/12/12 15:34:53, felt wrote:
> On 2014/12/12 10:43:34, Mounir Lamouri wrote:
> > Comments applied, TEST steps written. I've also changed the top
frame/embedded
> > frame rule. I checked with Adrienne from Chrome Security that it was fine
> (added
> > here as reviewer).
> > 
> > I have to wait for the Blink CLs to land and roll before landing this.
> 
> top/embedded frame rule change lgtm

actually: perhaps you should do that as a separate CL? it seems like a
conceptually
different thing; and if for some reason it needs to be changed or reverted,
would be
better to have it as its own CL.

[scheib, 2014-12-12 16:03:51]

Thanks, much clearer now.

[mlamouri (slow - plz ping), 2014-12-15 10:30:02]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-15 10:31:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789533002/110001

[commit-bot: I haz the power, 2014-12-15 10:43:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 10:43:46]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2015-01-16 14:48:36]

mlamouri@chromium.org changed reviewers:
+ torne@chromium.org

[mlamouri (slow - plz ping), 2015-01-16 14:48:37]

torne@, could you have a look at the changes in android_webview/, should be
mostly boilerplate.

[mlamouri (slow - plz ping), 2015-01-16 14:52:53]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2015-01-16 14:52:55]

Jochen, I am missing an OWNER review for the following files:
- chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc
- chrome/browser/ui/browser.cc
- chrome/browser/ui/browser.h
- chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller_unittest.mm
- chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
- chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc
- chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc

It is strictly boilerplate, updating the files to match the new content/public/
API that has been reviewed already.

[Torne, 2015-01-16 14:55:40]

android_webview LGTM

[jochen (gone - plz use gerrit), 2015-01-16 15:04:15]

lgtm.

[mlamouri (slow - plz ping), 2015-01-16 15:19:19]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-16 15:19:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789533002/220001

[commit-bot: I haz the power, 2015-01-16 16:31:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:31:57]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[mlamouri (slow - plz ping), 2015-01-16 16:32:42]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:33:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789533002/220001

[commit-bot: I haz the power, 2015-01-16 16:34:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:34:16]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[mlamouri (slow - plz ping), 2015-01-17 13:22:28]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-17 13:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789533002/220001

[commit-bot: I haz the power, 2015-01-17 13:24:23]

Committed patchset #13 (id:220001)

[commit-bot: I haz the power, 2015-01-17 13:25:11]

Patchset 13 (id:??) landed as
https://crrev.com/7a78d6fd7dc3241a91a614a09f9c59a0e4454135
Cr-Commit-Position: refs/heads/master@{#312038}