Make window.screen property non replaceable

Make window.screen property non replaceable

Make window.screen property non replaceable for consistency with other
browsers (tested with Firefox 28 and IE11).

This CL also adds an internals.setOverrideScreenData() utility function for
Layout tests as the fast/forms/page-popup/page-popup-adjust-rect.html test
needs to be able to override the screen object.

BUG=364937
TEST=fast/dom/Window/get-set-properties.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172134

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172495

[abarth-chromium, 2014-04-18 20:58:58]

Why does that test need to override the screen data?

[Inactive, 2014-04-18 21:08:40]

+keishi in cc.

https://codereview.chromium.org/243173004/diff/40001/LayoutTests/fast/forms/p...
File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (left):

https://codereview.chromium.org/243173004/diff/40001/LayoutTests/fast/forms/p...
LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:30:
popupWindow.screen = {
This test was added in https://bugs.webkit.org/show_bug.cgi?id=98499 for more
info. Looks like keishi is the author and tkent reviewed it.

The test overrides the screen property here for the popup window so that
screen.* have consistent values across machines / bots.

The idea is that this test should make sure that (quoted from original patch):
    1. Page popup should reposition itself so it won't get clipped by
screen(Win/Linux) or rootview(Mac) bounds.
    2. Page popup should resize itself when it doesn't fit.

https://codereview.chromium.org/243173004/diff/40001/LayoutTests/fast/forms/p...
File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):

https://codereview.chromium.org/243173004/diff/40001/LayoutTests/fast/forms/p...
LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:7: <script
src="../calendar-picker/resources/calendar-picker-common.js"></script>
The test is basically testing this library (which is a blink resource).

[abarth-chromium, 2014-04-18 21:11:44]

On 2014/04/18 21:08:40, Chris Dumez wrote:
> The test overrides the screen property here for the popup window so that
> screen.* have consistent values across machines / bots.

Can we actually make the popup window actually have a consistent size in tests? 
It seems very indirect to fake-out the screen property.

[abarth-chromium, 2014-04-18 21:12:38]

I'd rather not add yet-another-internals API that's only going to be used by one
test in order to test a JavaScript library.  Perhaps we need a JavaScript unit
testing framework for testing JavaScript libraries?

[Inactive, 2014-04-18 21:17:09]

On 2014/04/18 21:12:38, abarth wrote:
> I'd rather not add yet-another-internals API that's only going to be used by
one
> test in order to test a JavaScript library.  Perhaps we need a JavaScript unit
> testing framework for testing JavaScript libraries?

Yes, I understand. I will try to find a better way, maybe by rewriting the test.
I want to make sure we don't lose test coverage.

[Inactive, 2014-04-18 21:18:30]

On 2014/04/18 21:11:44, abarth wrote:
> On 2014/04/18 21:08:40, Chris Dumez wrote:
> > The test overrides the screen property here for the popup window so that
> > screen.* have consistent values across machines / bots.
> 
> Can we actually make the popup window actually have a consistent size in
tests? 
> It seems very indirect to fake-out the screen property.

I guess you mean having a consistent screen size, since what we want to check is
that the popup window resizes itself to fit the screen.

[Inactive, 2014-04-18 21:26:08]

For the record, here is what the diff looks like on my machine:
http://pastebin.com/Cs3hTbMx

1920 is my screen resolution width.

[abarth-chromium, 2014-04-18 21:46:37]

Yeah, maybe content_shell should lie about the screen size when run with
--dump-render-tree to make it consistent.

[Inactive, 2014-04-21 18:15:23]

keishi@, by any chance, would you be willing to help updating your layout test
(fast/forms/page-popup/page-popup-adjust-rect.html) so that it no longer relies
on overriding the screen properties? This is the only test causing trouble. I
have tried to take a look but it is still not obvious to me what the expected
results are if we change the dimensions.

[Inactive, 2014-04-21 18:32:07]

I found a way to update fast/forms/page-popup/page-popup-adjust-rect.html so
that is still works even if window.screen property is no longer replaceable (and
without adding a new Internals API). Could you please take another look?

[arv (Not doing code reviews), 2014-04-21 19:18:16]

LGTM

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:30: delete
w.screen['width']
delete w.screen.width;

[arv (Not doing code reviews), 2014-04-21 19:23:42]

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:29: function
overridePopupWindowScreenObject(w) {
You should also be able to do:

Object.defineProperty(window, 'screen', {
  width: {value: 80},
  height: {value: 60},
  availLeft: {value: 0},
  availTop: {value: 0},
  availWidth: {value: 80},
  availHeight: {value: 60},
});

[Inactive, 2014-04-21 19:51:52]

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):

https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:29: function
overridePopupWindowScreenObject(w) {
On 2014/04/21 19:23:43, arv wrote:
> You should also be able to do:
> 
> Object.defineProperty(window, 'screen', {
>   width: {value: 80},
>   height: {value: 60},
>   availLeft: {value: 0},
>   availTop: {value: 0},
>   availWidth: {value: 80},
>   availHeight: {value: 60},
> });
> 

This did not seem to work so I used your other proposal.

[arv (Not doing code reviews), 2014-04-21 19:53:36]

On 2014/04/21 19:51:52, Chris Dumez wrote:
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):
> 
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:29: function
> overridePopupWindowScreenObject(w) {
> On 2014/04/21 19:23:43, arv wrote:
> > You should also be able to do:
> > 
> > Object.defineProperty(window, 'screen', {
> >   width: {value: 80},
> >   height: {value: 60},
> >   availLeft: {value: 0},
> >   availTop: {value: 0},
> >   availWidth: {value: 80},
> >   availHeight: {value: 60},
> > });
> > 
> 
> This did not seem to work so I used your other proposal.

That should have been defineProperties (plural)

[Inactive, 2014-04-21 20:24:31]

On 2014/04/21 19:53:36, arv wrote:
> On 2014/04/21 19:51:52, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> > File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html (right):
> > 
> >
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> > LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:29: function
> > overridePopupWindowScreenObject(w) {
> > On 2014/04/21 19:23:43, arv wrote:
> > > You should also be able to do:
> > > 
> > > Object.defineProperty(window, 'screen', {
> > >   width: {value: 80},
> > >   height: {value: 60},
> > >   availLeft: {value: 0},
> > >   availTop: {value: 0},
> > >   availWidth: {value: 80},
> > >   availHeight: {value: 60},
> > > });
> > > 
> > 
> > This did not seem to work so I used your other proposal.
> 
> That should have been defineProperties (plural)

Hmm,

// No error but does not change the value of window.screen properties
Object.defineProperty(window, 'screen', {
   width: {value: 80},
   height: {value: 60},
   availLeft: {value: 0},
   availTop: {value: 0},
   availWidth: {value: 80},
   availHeight: {value: 60},
 });

// Gives an error: TypeError: Property description must be an object: s
Object.defineProperties(window, 'screen', {
   width: {value: 80},
   height: {value: 60},
   availLeft: {value: 0},
   availTop: {value: 0},
   availWidth: {value: 80},
   availHeight: {value: 60},
 });

// No error but does not change the value of window.screen properties
Object.defineProperties(window.screen, {
   'width': {value: 80},
   'height': {value: 60},
   'availLeft': {value: 0},
   'availTop': {value: 0},
   'availWidth': {value: 80},
   'availHeight': {value: 60},
 });

I am not familiar with those, I am still trying to find out to make it work
using defineProperty / defineProperties.

[arv (Not doing code reviews), 2014-04-21 20:28:50]

On 2014/04/21 20:24:31, Chris Dumez wrote:
> On 2014/04/21 19:53:36, arv wrote:
> > On 2014/04/21 19:51:52, Chris Dumez wrote:
> > >
> >
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> > > File LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/243173004/diff/60001/LayoutTests/fast/forms/p...
> > > LayoutTests/fast/forms/page-popup/page-popup-adjust-rect.html:29: function
> > > overridePopupWindowScreenObject(w) {
> > > On 2014/04/21 19:23:43, arv wrote:
> > > > You should also be able to do:
> > > > 
> > > > Object.defineProperty(window, 'screen', {
> > > >   width: {value: 80},
> > > >   height: {value: 60},
> > > >   availLeft: {value: 0},
> > > >   availTop: {value: 0},
> > > >   availWidth: {value: 80},
> > > >   availHeight: {value: 60},
> > > > });
> > > > 
> > > 
> > > This did not seem to work so I used your other proposal.
> > 
> > That should have been defineProperties (plural)
> 
> Hmm,
> 
> // No error but does not change the value of window.screen properties
> Object.defineProperty(window, 'screen', {
>    width: {value: 80},
>    height: {value: 60},
>    availLeft: {value: 0},
>    availTop: {value: 0},
>    availWidth: {value: 80},
>    availHeight: {value: 60},
>  });
> 
> // Gives an error: TypeError: Property description must be an object: s
> Object.defineProperties(window, 'screen', {
>    width: {value: 80},
>    height: {value: 60},
>    availLeft: {value: 0},
>    availTop: {value: 0},
>    availWidth: {value: 80},
>    availHeight: {value: 60},
>  });
> 
> // No error but does not change the value of window.screen properties
> Object.defineProperties(window.screen, {
>    'width': {value: 80},
>    'height': {value: 60},
>    'availLeft': {value: 0},
>    'availTop': {value: 0},
>    'availWidth': {value: 80},
>    'availHeight': {value: 60},
>  });
> 
> I am not familiar with those, I am still trying to find out to make it work
> using defineProperty / defineProperties.

I'm sorry for providing bad example code :'(

The following works in Firefox and old Blink and should work with this patch
since screen is configurable.

Object.defineProperty(window, 'screen', {
  value: {
    width: 80,
    height: 60,
    availLeft: 0,
    availTop: 0,
    availWidth: 80,
    availHeight: 60
  }
});

[Inactive, 2014-04-21 20:33:01]

> I'm sorry for providing bad example code :'(
> 
> The following works in Firefox and old Blink and should work with this patch
> since screen is configurable.
> 
> Object.defineProperty(window, 'screen', {
>   value: {
>     width: 80,
>     height: 60,
>     availLeft: 0,
>     availTop: 0,
>     availWidth: 80,
>     availHeight: 60
>   }
> });

Thanks for finding out the exact syntax for me arv! This worked. I updated the
CL accordingly.

[tkent, 2014-04-22 04:49:34]

public API change LGTM

[Inactive, 2014-04-22 12:58:48]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-22 12:58:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/243173004/120001

[commit-bot: I haz the power, 2014-04-22 13:06:41]

Change committed as 172134

[Inactive, 2014-04-23 21:15:05]

The fix has landed on Chromium side
(https://src.chromium.org/viewvc/chrome?view=rev&revision=265738), I am
relanding this.

[Inactive, 2014-04-23 21:15:10]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-23 21:15:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/243173004/140001

[commit-bot: I haz the power, 2014-04-23 22:23:35]

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

[commit-bot: I haz the power, 2014-04-23 22:23:35]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Inactive, 2014-04-24 12:06:21]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-24 12:06:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/243173004/140001

[commit-bot: I haz the power, 2014-04-24 13:12:39]

Change committed as 172495