Make it possible to set the display mode from Chromium

Make it possible to set the display mode from Chromium

This CL allows setting of blink::WebView display mode with the value obtainted from the content::WebContentsDelegate implementations (so far there is a preliminary implementation at content_shell).
The display mode value is send along with ViewMsg_Resize_Params IPC message.

The follow-up layout test: https://codereview.chromium.org/1066213003/

BUG=471703
Committed: https://crrev.com/c0e251b8eedbeec65e9d0cafc3c270f072fc6724
Cr-Commit-Position: refs/heads/master@{#325200}

[Mikhail, 2015-03-02 16:15:10]

mikhail.pozdnyakov@intel.com changed reviewers:
+ kenneth.r.christiansen@intel.com, mkwst@chromium.org

[Mikhail, 2015-03-02 16:23:08]

WIP (tests are missing yet)
This CL is basically following https://codereview.chromium.org/870933002#msg6, I
decided though not to integrate the ViewMsg_Resize_Params 'fullscreen' and
'display_mode' fields as those have different origins and can be set
independently.

CL is not yet complete however, could you please have a look and say whether the
approach is OK (does the content::WebContentsDelegate look like as an
appropriate place to store the display mode values?)

[mlamouri (slow - plz ping), 2015-03-02 19:02:53]

Do we expect the display mode to change during the lifetime of the widget except
for fullscreen?

[Mikhail, 2015-03-03 09:40:47]

On 2015/03/02 19:02:53, Mounir Lamouri wrote:
> Do we expect the display mode to change during the lifetime of the widget
except
> for fullscreen?

I would not expect display mode changes other than going to fullscreen and back

[kenneth.christiansen, 2015-03-06 12:21:28]

kenneth.christiansen@gmail.com changed reviewers:
+ kenneth.christiansen@gmail.com

[kenneth.christiansen, 2015-03-06 12:21:29]

https://codereview.chromium.org/964403003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/964403003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:590:
resize_params->display_mode = GetDisplayMode();
isn't Isfullscreen included in the GetDisplayMode?

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

https://codereview.chromium.org/964403003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:559: // FIXME : Should
it be renamed to 'IsHTMLFullscreen'?
What does it represent?

* An element in fullscreen?
* The browser in kiosk or fullscreen mode (like on Mac)

In any case I don't see how HTML makes it more understandable of which of the
two it is

https://codereview.chromium.org/964403003/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/964403003/diff/1/content/common/view_messages...
content/common/view_messages.h:450: // Indicates whether a page is set
fullscreen via HTML API.
you dont set a page to fullscreen using the JS API, you make an element
fullscreen

https://codereview.chromium.org/964403003/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/964403003/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:3186: // TODO: Shouldn't it be checked
inside Blink instead?
I guess if this saves IPC then it is better

[Mikhail, 2015-03-06 15:42:36]

Should not land before https://codereview.chromium.org/984023003/

[Mikhail, 2015-03-13 09:17:28]

On 2015/03/06 15:42:36, Mikhail wrote:
> Should not land before https://codereview.chromium.org/984023003/

Now dependent fix has landed. Could you please review?

[Mike West, 2015-03-18 08:43:33]

A few nits and questions. IPC LGTM, but you'll need a content/ OWNER to take a
look.

https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:646: class
WebDisplayModeDelegate : public WebContentsDelegate {
Nit: Don't define classes inside tests, please. I'd suggest pulling this out
into an anonymous namespace.

https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:663: "
minimal-ui)').matches"));
Can you add tests that verify changes to the display mode are propagated
correctly?

https://codereview.chromium.org/964403003/diff/20001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/964403003/diff/20001/content/common/view_mess...
content/common/view_messages.h:86:
blink::WebDisplayMode::WebDisplayModeFullscreen)
Nit: You'll need to add WebDisplayModeLast to match the rest of the enums here.
I don't mind if you add a TODO here and do it in two more CLs.

https://codereview.chromium.org/964403003/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/964403003/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:128: display_mode_ ==
blink::WebDisplayModeFullscreen; }
When will these be out of sync?

[Mikhail, 2015-03-24 13:33:06]

mikhail.pozdnyakov@intel.com changed reviewers:
+ jochen@chromium.org

[Mikhail, 2015-03-24 13:50:10]

On 2015/03/18 08:43:33, Mike West wrote:
> A few nits and questions. IPC LGTM, but you'll need a content/ OWNER to take a
> look.
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl_browsertest.cc:646: class
> WebDisplayModeDelegate : public WebContentsDelegate {
> Nit: Don't define classes inside tests, please. I'd suggest pulling this out
> into an anonymous namespace.
Done.
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl_browsertest.cc:663: "
> minimal-ui)').matches"));
> Can you add tests that verify changes to the display mode are propagated
> correctly?
Done.
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/common/view_mess...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/common/view_mess...
> content/common/view_messages.h:86:
> blink::WebDisplayMode::WebDisplayModeFullscreen)
> Nit: You'll need to add WebDisplayModeLast to match the rest of the enums
here.
> I don't mind if you add a TODO here and do it in two more CLs.
So far I uploaded a CL for blink (https://codereview.chromium.org/1029833002/)
I'll update this CL after it lands.
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/renderer/render_...
> File content/renderer/render_widget.h (right):
> 
>
https://codereview.chromium.org/964403003/diff/20001/content/renderer/render_...
> content/renderer/render_widget.h:128: display_mode_ ==
> blink::WebDisplayModeFullscreen; }
> When will these be out of sync?
Those two have different origins: 'display_mode_' comes from
http://w3c.github.io/manifest/#the-display-mode-media-feature, and 'fullscreen_'
is used in the fullscreen API implementation code path.

Thanks!

[jochen (gone - plz use gerrit), 2015-03-25 12:10:24]

jochen@chromium.org changed reviewers:
+ danakj@chromium.org, falken@chromium.org

[jochen (gone - plz use gerrit), 2015-03-25 12:10:24]

+falken for fullscreen
+danakj for resize message

[danakj, 2015-03-25 16:21:04]

On 2015/03/25 12:10:24, jochen wrote:
> +danakj for resize message

Sorry I'm not sure what parts of this you want me to review..

[jochen (gone - plz use gerrit), 2015-03-25 16:30:43]

On 2015/03/25 at 16:21:04, danakj wrote:
> On 2015/03/25 12:10:24, jochen wrote:
> > +danakj for resize message
> 
> Sorry I'm not sure what parts of this you want me to review..

ah, I just saw resize IPC related, and thought I'd cc you fyi

if a quick glance at the CL doesn't make you worry about anything, feel free to
pass

[danakj, 2015-03-25 16:56:38]

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

https://codereview.chromium.org/964403003/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:806: bool will_be_fullscreen = is_fullscreen
||
I find it kinda weird that there is both is_fullscreen_ and display_mode_, and
it feels like maybe those should be consolidated. But I don't think I have the
context enough to really judge.

[falken, 2015-03-26 02:13:52]

The CL description should explain more the motivation for the change. There's no
bug for this work?

But my knowledge of fullscreen is just Blink's implementation, so I'm not sure I
can meaningfully review this.

[jochen (gone - plz use gerrit), 2015-03-26 15:37:58]

On 2015/03/26 at 02:13:52, falken wrote:
> The CL description should explain more the motivation for the change. There's
no bug for this work?
> 
> But my knowledge of fullscreen is just Blink's implementation, so I'm not sure
I can meaningfully review this.

who's a good content fullscreen reviewer?

[falken, 2015-03-27 00:47:36]

falken@chromium.org changed reviewers:
+ miu@chromium.org, scheib@chromium.org

[falken, 2015-03-27 00:47:37]

Maybe +scheib?

The callsites of is_fullscreen() are just pepper_plugin_instance_impl.cc and
resizing_mode_selector.cc, adding +miu@ also who's done some pepper/fullscreen
work.

[scheib, 2015-03-27 16:50:52]

Patch definitely needs more explanation in the change comment, needs a BUG=, and
most importantly the code comments should resolve the ambiguity questions folks
have raised.

Fullscreen is pretty hairy, with HTML fullscreen, casting only fullscreen,
browser fullscreen, mac presentation mode, windows metro snap, App window
fullscreen, and now   WebDisplayModeFullscreen as well.

Can we keep items as orthogonal as possible?

https://codereview.chromium.org/964403003/diff/40001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/964403003/diff/40001/content/common/view_mess...
content/common/view_messages.h:89:
blink::WebDisplayMode::WebDisplayModeFullscreen)
Enum should have a ___Last member.

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

https://codereview.chromium.org/964403003/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:3905: params.is_fullscreen =
is_fullscreen_;
Dropping the is_fullscreen() accessor should come with a note for why the
encapsulation is being worked around.

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

https://codereview.chromium.org/964403003/diff/40001/content/renderer/render_...
content/renderer/render_widget.cc:806: bool will_be_fullscreen = is_fullscreen
||
On 2015/03/25 16:56:38, danakj wrote:
> I find it kinda weird that there is both is_fullscreen_ and display_mode_, and
> it feels like maybe those should be consolidated. But I don't think I have the
> context enough to really judge.

Similar to above with is_fullscreen(), it seems that if the window actually is
fullscreen then is_fullscreen should be true.

https://codereview.chromium.org/964403003/diff/40001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/964403003/diff/40001/content/renderer/render_...
content/renderer/render_widget.h:127: bool is_fullscreen() const { return
is_fullscreen_ ||
Yikes, if we're fullscreen why isn't is_fullscreen_ also being set to true?

[miu, 2015-03-27 21:14:55]

Sort of echoing what other reviewers have said, but I'd like to know more about
what blink::WebDisplayMode is all about.  Up until now, blink only ever cared
about "not fullscreen" versus "fullscreen" and the latter only drove blink to
either:

1. Re-layout the page so that a single fullscreened DOM element was sized to
fill the root render widget.

2. Do nothing special if there is no DOM element flagged as the designated
fullscreened element.

How would a renderer behave in response to each of the possible WebDisplayModes?
 Are we giving the renderer too much information about the browser UI state? 
Perhaps an eng review is warranted here, if there hasn't been one already?

[Mikhail, 2015-03-30 15:47:17]

Thanks for having a look!

I've added a bug to the CL description. 
In general what I'm willing to achieve is to enable
http://w3c.github.io/manifest/#the-display-mode-media-feature

In the latest version of patch I completely split the newly added
"RenderWidget::display_mode_" logic from the existing
"RenderWidget::is_fullscreen_" logic, reason: currently
RenderWidget::is_fullscreen_ reflects only (as I see from the code, it gets
initialized from WebContentsImpl::IsFullscreenForCurrentTab())
renderer-initiated fullscreen mode and RenderWidget::display_mode_ reflects the
display mode of the top-level browsing context (meaning that it should be equal
to blink::WebDisplayModeFullscreen wherever user puts a browser window into
fullscreen).

PTAL

[scheib, 2015-03-30 17:47:16]

Ok this is looking pretty reasonable, but it's hard to see the use in Blink.
Often patches such as this have companion patches reference in Blink that show
the utilization of the parameter.

On the blink side, WebDisplayMode should reference the specification.

I see that The 'display-mode' media feature should represent fullscreen being
true even when enabled by the user and not the application manifest.
https://w3c.github.io/manifest/#the-display-mode-media-feature and it appears
that is what this patch is doing. 

This should be captured in tests as well, LayoutTests would be the place to do
it, though I'm not sure if we're setup to automate the user/browser fullscreen
mode from LayoutTest automation.

The AppWindow functionality seems reasonable to test via API tests similar to
src/chrome/test/data/extensions/api_test/fullscreen/

[miu, 2015-03-31 01:37:49]

https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
content/renderer/render_widget.h:127: bool is_fullscreen() const { return
is_fullscreen_; }
I understand now why this is decoupled, but there's a real readability issue
here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the variable
names should be changed throughout the vertical (i.e., IPC message, and
host-side) to match.

[Mikhail, 2015-03-31 15:23:32]

On 2015/03/30 17:47:16, scheib wrote:
> Ok this is looking pretty reasonable, but it's hard to see the use in Blink.
> Often patches such as this have companion patches reference in Blink that show
> the utilization of the parameter.
These are the related blink side patches:
https://codereview.chromium.org/870933002
https://codereview.chromium.org/853483003

> 
> On the blink side, WebDisplayMode should reference the specification.
> 
> I see that The 'display-mode' media feature should represent fullscreen being
> true even when enabled by the user and not the application manifest.
> https://w3c.github.io/manifest/#the-display-mode-media-feature and it appears
> that is what this patch is doing. 
> 
> This should be captured in tests as well, LayoutTests would be the place to do
> it, though I'm not sure if we're setup to automate the user/browser fullscreen
> mode from LayoutTest automation.
> 
> The AppWindow functionality seems reasonable to test via API tests similar to
> src/chrome/test/data/extensions/api_test/fullscreen/
I've added display-mode media feature check to the existing
'extensions/api_test/fullscreen/' test, is it OK or you would prefer having a
separate test for it?

Thanks!

[Mikhail, 2015-03-31 15:24:24]

On 2015/03/31 01:37:49, miu wrote:
>
https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
> File content/renderer/render_widget.h (right):
> 
>
https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
> content/renderer/render_widget.h:127: bool is_fullscreen() const { return
> is_fullscreen_; }
> I understand now why this is decoupled, but there's a real readability issue
> here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the
variable
> names should be changed throughout the vertical (i.e., IPC message, and
> host-side) to match.

This looks like a significant change itself, could I do it within a separate CL?

[scheib, 2015-03-31 21:02:48]

On 2015/03/31 15:24:24, Mikhail wrote:
> On 2015/03/31 01:37:49, miu wrote:
> >
>
https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
> > File content/renderer/render_widget.h (right):
> > 
> >
>
https://codereview.chromium.org/964403003/diff/60001/content/renderer/render_...
> > content/renderer/render_widget.h:127: bool is_fullscreen() const { return
> > is_fullscreen_; }
> > I understand now why this is decoupled, but there's a real readability issue
> > here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the
> variable
> > names should be changed throughout the vertical (i.e., IPC message, and
> > host-side) to match.
> 
> This looks like a significant change itself, could I do it within a separate
CL?

I don't think a rename is appropriate. The browser either puts the view into
fullscreen or not, and then sets is_fullscreen. Sometimes this is in reponse to
HTML requesting fullscreen, and then I suppose fullscreen has been granted.
Sometimes it is because the user put the browser into fullscreen, and then it's
just fullscreen. Or, did I fail to understand the code correctly?

[scheib, 2015-03-31 21:03:02]

https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/fullscreen_app/check_media_feature.js (right):

https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/fullscreen_app/check_media_feature.js:1: var
fullscreen = matchMedia( '(display-mode: fullscreen)' );
This needs to be loaded by a browser test and verified, right?

[scheib, 2015-03-31 21:05:35]

On 2015/03/31 15:23:32, Mikhail wrote:
> These are the related blink side patches:
> https://codereview.chromium.org/870933002
> https://codereview.chromium.org/853483003

I think you need a future change that shows layout tests verifying the behavior
of HTML5 fullscreen causing the media query to report Fullscreen.

[miu, 2015-03-31 21:22:12]

On 2015/03/31 21:02:48, scheib wrote:
> On 2015/03/31 15:24:24, Mikhail wrote:
> > On 2015/03/31 01:37:49, miu wrote:
> > > I understand now why this is decoupled, but there's a real readability
issue
> > > here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the
> > variable
> > > names should be changed throughout the vertical (i.e., IPC message, and
> > > host-side) to match.
> 
> I don't think a rename is appropriate. The browser either puts the view into
> fullscreen or not, and then sets is_fullscreen. Sometimes this is in reponse
to
> HTML requesting fullscreen, and then I suppose fullscreen has been granted.
> Sometimes it is because the user put the browser into fullscreen, and then
it's
> just fullscreen. Or, did I fail to understand the code correctly?

It really does only mean tab-initiated fullscreen was granted.  Follow the code
path in code search, and you'll see this boolean is exactly the result of the
call to FullscreenController::IsFullscreenForTabOrPending().  That's why I'm
worried and really do feel the rename is necessary.

If the user puts the browser into fullscreen (e.g., F11 or the via the hot-dog
menu), this boolean is false; and the renderer is not in any way aware the
browser window is fullscreened.

[miu, 2015-03-31 21:24:47]

On 2015/03/31 15:24:24, Mikhail wrote:
> On 2015/03/31 01:37:49, miu wrote:
> > I understand now why this is decoupled, but there's a real readability issue
> > here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the
> variable
> > names should be changed throughout the vertical (i.e., IPC message, and
> > host-side) to match.
> 
> This looks like a significant change itself, could I do it within a separate
CL?

Well, you're already touching most of the files and same LOC in this change. 
But, it's up to you.

At the least, in this change, please file a crbug and put a TODO with the bug
number at line 597 in render_widget_host_impl.cc mentioning that the naming is
misleading.

[scheib, 2015-03-31 21:55:02]

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

https://codereview.chromium.org/964403003/diff/80001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:384: virtual
blink::WebDisplayMode GetDisplayMode(
Comment here the specification DisplayMode represents, that that it should be
used w.r.t. the web manifest, HTML5 fullscreen, and user initiated browser
fullscreen.

[scheib, 2015-03-31 21:55:23]

OK, sorry for missing that, I've looked again and agree with miu@,
is_fullscreen is used to eventually call
FullscreenController::didEnterFullScreen(), which is only desired when HTML
requests fullscreen. I'd suggest a follow up CL as I believe it will be
easier to review - but agree with commenting and citing an issue in
render_widget*.

For this patch I still think we should see a plan  for how to test "if the
end-user puts the whole user agent into fullscreen, then the user agent
would reflect this change to CSS and scripts via the 'display-mode' media
feature." in the https://w3c.github.io/manifest/

On Tue, Mar 31, 2015 at 2:24 PM, <miu@chromium.org> wrote:

> On 2015/03/31 15:24:24, Mikhail wrote:
>
>> On 2015/03/31 01:37:49, miu wrote:
>> > I understand now why this is decoupled, but there's a real readability
>> issue
>> > here w.r.t. naming now.  Suggestion: is_fullscreen_granted()  Also, the
>> variable
>> > names should be changed throughout the vertical (i.e., IPC message, and
>> > host-side) to match.
>>
>
>  This looks like a significant change itself, could I do it within a
>> separate
>>
> CL?
>
> Well, you're already touching most of the files and same LOC in this
> change.
> But, it's up to you.
>
> At the least, in this change, please file a crbug and put a TODO with the
> bug
> number at line 597 in render_widget_host_impl.cc mentioning that the
> naming is
> misleading.
>
>
> https://codereview.chromium.org/964403003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mikhail, 2015-04-07 22:14:58]

On 2015/03/31 21:05:35, scheib wrote:
> On 2015/03/31 15:23:32, Mikhail wrote:
> > These are the related blink side patches:
> > https://codereview.chromium.org/870933002
> > https://codereview.chromium.org/853483003
> 
> I think you need a future change that shows layout tests verifying the
behavior
> of HTML5 fullscreen causing the media query to report Fullscreen.

Here I made a CL with the layout test which passes after this patch is applied:
https://codereview.chromium.org/1066213003/

[Mikhail, 2015-04-07 22:15:52]

On 2015/03/31 21:24:47, miu wrote:
> At the least, in this change, please file a crbug and put a TODO with the bug
> number at line 597 in render_widget_host_impl.cc mentioning that the naming is
> misleading.
Done. Thanks.

[miu, 2015-04-08 01:25:03]

lgtm

[jochen (gone - plz use gerrit), 2015-04-08 07:54:37]

https://codereview.chromium.org/964403003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/964403003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:659: };
nit. DISALLOW_COPY_AND_ASSIGN() also please add a virtual dtor

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:482: is_fullscreen_(false),
why did you drop this?

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:783: ResizeAck resize_ack) {
why did you drop the const?

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:1850: if (is_fullscreen()) {
this change is not needed anymore?

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:1861: if (is_fullscreen()) {
same here?

[Mikhail, 2015-04-08 09:16:01]

https://codereview.chromium.org/964403003/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/964403003/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:659: };
On 2015/04/08 07:54:36, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN() also please add a virtual dtor

Done.

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:482: is_fullscreen_(false),
On 2015/04/08 07:54:36, jochen wrote:
> why did you drop this?

it is a rudiment from the previous versions of the patch. Thanks for catching!

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:783: ResizeAck resize_ack) {
On 2015/04/08 07:54:36, jochen wrote:
> why did you drop the const?
'const' is missing in the method declaration, anyway it's out of this CL scope.

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:1850: if (is_fullscreen()) {
On 2015/04/08 07:54:36, jochen wrote:
> this change is not needed anymore?
yep, fixed.

https://codereview.chromium.org/964403003/diff/100001/content/renderer/render...
content/renderer/render_widget.cc:1861: if (is_fullscreen()) {
On 2015/04/08 07:54:36, jochen wrote:
> same here?
Ditto.

[jochen (gone - plz use gerrit), 2015-04-08 09:21:04]

lgtm

[Mikhail, 2015-04-08 11:09:00]

On 2015/03/31 21:03:02, scheib wrote:
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> File chrome/test/data/extensions/fullscreen_app/check_media_feature.js
(right):
> 
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> chrome/test/data/extensions/fullscreen_app/check_media_feature.js:1: var
> fullscreen = matchMedia( '(display-mode: fullscreen)' );
> This needs to be loaded by a browser test and verified, right?

(WebContentsImplBrowserTest, ChangeDisplayMode) is a browser test for it,
however AppWindow is a bit different code path, would you like me to remove the
check from here?

[Mikhail, 2015-04-08 11:09:37]

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

https://codereview.chromium.org/964403003/diff/80001/content/public/browser/w...
content/public/browser/web_contents_delegate.h:384: virtual
blink::WebDisplayMode GetDisplayMode(
On 2015/03/31 21:55:02, scheib wrote:
> Comment here the specification DisplayMode represents, that that it should be
> used w.r.t. the web manifest, HTML5 fullscreen, and user initiated browser
> fullscreen.

Done.

[Mikhail, 2015-04-09 20:55:38]

On 2015/04/08 11:09:00, Mikhail wrote:
> On 2015/03/31 21:03:02, scheib wrote:
> >
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> > File chrome/test/data/extensions/fullscreen_app/check_media_feature.js
> (right):
> > 
> >
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> > chrome/test/data/extensions/fullscreen_app/check_media_feature.js:1: var
> > fullscreen = matchMedia( '(display-mode: fullscreen)' );
> > This needs to be loaded by a browser test and verified, right?
> 
> (WebContentsImplBrowserTest, ChangeDisplayMode) is a browser test for it,
> however AppWindow is a bit different code path, would you like me to remove
the
> check from here?

friendly ping @scheib

[Mikhail, 2015-04-10 10:24:27]

On 2015/04/09 20:55:38, Mikhail wrote:
> On 2015/04/08 11:09:00, Mikhail wrote:
> > On 2015/03/31 21:03:02, scheib wrote:
> > >
> >
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> > > File chrome/test/data/extensions/fullscreen_app/check_media_feature.js
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/964403003/diff/80001/chrome/test/data/extensi...
> > > chrome/test/data/extensions/fullscreen_app/check_media_feature.js:1: var
> > > fullscreen = matchMedia( '(display-mode: fullscreen)' );
> > > This needs to be loaded by a browser test and verified, right?
> > 
> > (WebContentsImplBrowserTest, ChangeDisplayMode) is a browser test for it,
> > however AppWindow is a bit different code path, would you like me to remove
> the
> > check from here?
> 
> friendly ping @scheib

I've extracted extensions::AppWindow changes to
https://codereview.chromium.org/l1070423003/, so that the LGTM'ed code (which
already contains comprehensive functionality for this CL) can land and I can
progress further with this feature in Chrome.
Let's continue extensions::AppWindow implementation work at
https://codereview.chromium.org/1070423003/.

[Mikhail, 2015-04-10 10:27:34]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2015-04-10 10:27:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
miu@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/964403003/#ps160001
(title: "Omit extensions/ changes (extract those to a separate CL)")

[commit-bot: I haz the power, 2015-04-10 10:28:02]

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

[commit-bot: I haz the power, 2015-04-10 10:37:32]

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

[commit-bot: I haz the power, 2015-04-10 10:37:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mikhail, 2015-04-10 12:29:01]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2015-04-10 12:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/964403003/#ps180001
(title: "Allowed include of
"third_party/WebKit/public/platform/WebDisplayMode.h"")

[commit-bot: I haz the power, 2015-04-10 12:29:27]

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

[commit-bot: I haz the power, 2015-04-10 18:31:33]

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

[commit-bot: I haz the power, 2015-04-10 18:31:35]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Mikhail, 2015-04-13 07:54:36]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[commit-bot: I haz the power, 2015-04-13 07:55:15]

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

[commit-bot: I haz the power, 2015-04-13 13:55:11]

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

[commit-bot: I haz the power, 2015-04-13 13:55:12]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[scheib, 2015-04-13 18:30:22]

LGTM

Small comment change requested.

Cite the layout test in this change description
https://codereview.chromium.org/1066213003/

https://codereview.chromium.org/964403003/diff/180001/content/shell/browser/s...
File content/shell/browser/shell.cc (right):

https://codereview.chromium.org/964403003/diff/180001/content/shell/browser/s...
content/shell/browser/shell.cc:318: // FIXME : should return
blink::WebDisplayModeFullscreen wherever user puts a
TODO comments. Cite a follow up issue.

[Mikhail, 2015-04-14 10:37:23]

The newly added test cannot pass on Android because of http://crbug.com/129998:

[ERROR:shell_android.cc(70)] Not implemented reached in void
content::Shell::PlatformSetTitle(const string16&): true
[ERROR:shell_android.cc(70)] Not implemented reached in void
content::Shell::PlatformSetTitle(const string16&): false
../../content/browser/web_contents/web_contents_impl_browsertest.cc:689: Failure
Value of: shell()->web_contents()->GetTitle()
  Actual: false
Expected: base::ASCIIToUTF16("true")
Which is: true

have to disable the test on Android.

[Mikhail, 2015-04-14 12:33:07]

On 2015/04/14 10:37:23, Mikhail wrote:
> The newly added test cannot pass on Android because of
http://crbug.com/129998:
> 
> [ERROR:shell_android.cc(70)] Not implemented reached in void
> content::Shell::PlatformSetTitle(const string16&): true
> [ERROR:shell_android.cc(70)] Not implemented reached in void
> content::Shell::PlatformSetTitle(const string16&): false
> ../../content/browser/web_contents/web_contents_impl_browsertest.cc:689:
Failure
> Value of: shell()->web_contents()->GetTitle()
>   Actual: false
> Expected: base::ASCIIToUTF16("true")
> Which is: true
> 
> have to disable the test on Android.

This assumption was wrong, I've modified the browser test so that it passes at
linux_android_rel_ng

[Mikhail, 2015-04-14 13:22:18]

On 2015/04/14 12:33:07, Mikhail wrote:
> On 2015/04/14 10:37:23, Mikhail wrote:
> > The newly added test cannot pass on Android because of
> http://crbug.com/129998:
> > 
> > [ERROR:shell_android.cc(70)] Not implemented reached in void
> > content::Shell::PlatformSetTitle(const string16&): true
> > [ERROR:shell_android.cc(70)] Not implemented reached in void
> > content::Shell::PlatformSetTitle(const string16&): false
> > ../../content/browser/web_contents/web_contents_impl_browsertest.cc:689:
> Failure
> > Value of: shell()->web_contents()->GetTitle()
> >   Actual: false
> > Expected: base::ASCIIToUTF16("true")
> > Which is: true
> > 
> > have to disable the test on Android.
> 
> This assumption was wrong, I've modified the browser test so that it passes at
> linux_android_rel_ng

OK, all is green. Not sure whether it needs another review now (since the
browser test code has been modified).

[scheib, 2015-04-14 15:06:32]

LGTM
For smaller changes you should use judgement, especially if they're test only
fix ups it's often OK to move forward without additional round of L G T M.

You included a merge of tip of tree in the last few patches which also makes
them harder to see what you changed in the patches vs what changed in the tree.
Sometimes it's hard to avoid this, but if you think of it please upload patches
with changes, and upload a separate patch for just the 'merge tip of tree'.

[Mikhail, 2015-04-15 06:46:41]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2015-04-15 06:46:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/964403003/#ps240001
(title: "Re-enable test for Android, force "was resized" IPC message there.")

[commit-bot: I haz the power, 2015-04-15 06:47:33]

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

[commit-bot: I haz the power, 2015-04-15 06:51:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-04-15 06:52:28]

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